Guido van Rossum added the comment:
Sadly, the optimistic code doesn't work on Windows. I think it may be because
the socketpair() helper at the top of test_selectors.py uses an extra socket
('l') and the handles just don't match up (I get a failure on assert
wr2.fileno() == w). So I
STINNER Victor added the comment:
The current test using os.dup2() with a skip on Windows is fine.
--
___
Python tracker rep...@bugs.python.org
http://bugs.python.org/issue19876
___
Guido van Rossum added the comment:
OK, closed.
--
resolution: - fixed
status: open - closed
___
Python tracker rep...@bugs.python.org
http://bugs.python.org/issue19876
___
Charles-François Natali added the comment:
The test is failing on Windows buildbot:
http://buildbot.python.org/all/builders/x86%20Windows%20Server%202003%20%5BSB%5D%203.x/builds/1851/steps/test/logs/stdio
==
ERROR:
STINNER Victor added the comment:
I don't like generic except OSError: pass. Here is a first patch for epoll()
to use except FileNotFoundError: pass instead. Kqueue selector should also be
patched.
I tested to close epoll FD (os.close(epoll.fileno())): on Linux 3.11,
epoll.unregister(fd) and
Charles-François Natali added the comment:
STINNER Victor added the comment:
I don't like generic except OSError: pass. Here is a first patch for
epoll() to use except FileNotFoundError: pass instead. Kqueue selector
should also be patched.
Except that it can fail with ENOENT, but also
Guido van Rossum added the comment:
I don't think we should be more selective about the errno values, the try block
is narrow enough (just one syscall) and we really don't know what the kernel
will do on different platforms. And what would we do about it anyway?
I will look into the Windows
Charles-François Natali added the comment:
I will look into the Windows problem, but I suspect the best we can do there
is skip the test.
I already took care of that:
http://hg.python.org/cpython/rev/01676a4c16ff
--
___
Python tracker
Guido van Rossum added the comment:
Then here's a hopeful fix for the Windows situation that relies on the
socketpair() operation reusing FDs from the lowest value. I'm adding asserts to
check that this is actually the case. (These are actual assert statements to
indicate that they are
Roundup Robot added the comment:
New changeset c4c1c4bc8086 by Victor Stinner in branch 'default':
Issue #19876: Run also
test_selectors.test_unregister_after_fd_close_and_reuse() on Windows
http://hg.python.org/cpython/rev/c4c1c4bc8086
--
___
STINNER Victor added the comment:
Oops, I reverted my changeset c4c1c4bc8086, I didn't read why the test was
skipped on Windows. Sorry.
--
___
Python tracker rep...@bugs.python.org
http://bugs.python.org/issue19876
STINNER Victor added the comment:
Except that it can fail with ENOENT, but also EBADF, and EPERM if the
FD has been reused by a FD which doesn't support epoll.
Oh, I didn't know that. I ran the unit test, and I expected the two unit test
to cover any error case. So ignore my
Guido van Rossum added the comment:
Sorry, here's another version. It keeps the original _fileobj_to_fd function
and wraps it with a method that does the exhaustive search.
--
Added file: http://bugs.python.org/file33025/unregister6.diff
___
Python
Guido van Rossum added the comment:
I think I got the closing sorted out now, and through reordering the dup2()
calls are actually needed.
--
Added file: http://bugs.python.org/file33028/unregister7.diff
___
Python tracker rep...@bugs.python.org
Guido van Rossum added the comment:
OK, here's another try. I ran what you suggested for all three tests I added
and they are all clean. I realized that every single call to socketpair() is
followed by two addCleanup calls, so I added a make_socketpair() helper method
that does this.
Charles-François Natali added the comment:
LGTM!
--
___
Python tracker rep...@bugs.python.org
http://bugs.python.org/issue19876
___
___
Python-bugs-list mailing list
Roundup Robot added the comment:
New changeset 39e7995f9ad1 by Guido van Rossum in branch 'default':
Silently ignore unregistering closed files. Fixes issue 19876. With docs and
slight test refactor.
http://hg.python.org/cpython/rev/39e7995f9ad1
--
nosy: +python-dev
Guido van Rossum added the comment:
Is this worthy of a Misc/NEWS entry?
--
___
Python tracker rep...@bugs.python.org
http://bugs.python.org/issue19876
___
___
Roundup Robot added the comment:
New changeset f334dd2471e7 by Guido van Rossum in branch 'default':
News item for issue 19876.
http://hg.python.org/cpython/rev/f334dd2471e7
--
___
Python tracker rep...@bugs.python.org
Guido van Rossum added the comment:
Done.
--
assignee: neologix - gvanrossum
resolution: - fixed
stage: - committed/rejected
status: open - closed
type: - behavior
___
Python tracker rep...@bugs.python.org
http://bugs.python.org/issue19876
Guido van Rossum added the comment:
Ping? Charle-François, what do you think of my patch to ignore OSError in
unregister()?
--
assignee: docs@python - neologix
___
Python tracker rep...@bugs.python.org
http://bugs.python.org/issue19876
Charles-François Natali added the comment:
Sorry for the delay, I didn't have any free time this week.
I'll review the patch shortly, but the idea sounds fine (I just need
to check if we can't be a little more specific for errnos upon
unregister).
--
Guido van Rossum added the comment:
Here's a new patch. Note that it includes a commented-out test that
demonstrates the failure if the socket object itself is closed (rather than
just its FD).
--
Added file: http://bugs.python.org/file33013/unregister2.diff
Guido van Rossum added the comment:
Here's a variant that documents the ValueError issue and omits the
commented-out test.
--
Added file: http://bugs.python.org/file33014/unregister3.diff
___
Python tracker rep...@bugs.python.org
Guido van Rossum added the comment:
Here's an attempt at fixing the ValueError.
I don't like the exhaustive search much, but the alternative is to maintain an
inverse dict. What do you think?
--
Added file: http://bugs.python.org/file33015/unregister4.diff
Charles-François Natali added the comment:
Guido van Rossum added the comment:
Here's an attempt at fixing the ValueError.
I don't like the exhaustive search much, but the alternative is to maintain
an inverse dict. What do you think?
I was going to suggest such an exhaustive search.
I
Guido van Rossum added the comment:
OK, I'll make a new patch (maybe Monday). I want to be a little more careful
with the exhaustive search, I think it should not be attempted if we see
KeyError or AttributeError (those should not be dynamic).
I tested for the epoll error on Ubuntu and
Guido van Rossum added the comment:
So I think this is why epoll doesn't raise OSError:
http://hg.python.org/cpython/file/44dacafdd48a/Modules/selectmodule.c#l1335
The Python wrapper explicitly checks for EBADF and turns this into a non-error
result.
--
Guido van Rossum added the comment:
Well, I take it back. If you close the FD and then reuse it, you get ENOENT,
which is not caught. So we still need the try/except OSError.
I am going to experiment with a PollSelector as well.
--
___
Python
Guido van Rossum added the comment:
PollSelector doesn't seem to have this behavior.
--
___
Python tracker rep...@bugs.python.org
http://bugs.python.org/issue19876
___
Guido van Rossum added the comment:
New patch. Please review.
The error handling is a bit complicated but I like to be careful in which
errors I catch.
--
Added file: http://bugs.python.org/file33020/unregister5.diff
___
Python tracker
Guido van Rossum added the comment:
Here's a tentative change to selectors.py that ignores the OSError in various
unregister() methods (but not in register()).
--
keywords: +patch
Added file: http://bugs.python.org/file32999/unregister.diff
___
Guido van Rossum added the comment:
I just ran into a live case of the platform differences here. Check out
http://bugs.python.org/review/19509/ (issue 19509). Christian uploaded a patch
for asyncio, and when I tested it I got a double traceback and a hang. This
could have been avoided if
Guido van Rossum added the comment:
The more I think about this the more I believe unregister() should catch the
OSError (but not the KeyError).
Every unregister() implementation starts by calling super().unregister(key),
which has a side effect (it removes the key from the _fd_to_key dict).
New submission from STINNER Victor:
I remember a discussion about EBADF, but I don't remember the conclusion. The
documentation of the asyncio doesn't explain the behaviour of selectors when a
file/socket is closed, without unregistering it from the selector.
I should be explicitly
Guido van Rossum added the comment:
Yeah, the behavior is at least different for each type of polling system calls,
and possibly also for different platforms. It would be good to describe at
least all the different possible behaviors.
--
___
Charles-François Natali added the comment:
Well, unregister() documentation currently contains this:
.. method:: unregister(fileobj)
Unregister a file object from selection, removing it from monitoring. A
file object shall be unregistered prior to being closed.
I'm not sure
Guido van Rossum added the comment:
I think we should give the reader some kind of hint, since a bug in this area
may cause a lot of pain when it has to be debugged on porting from a system
where the issue is silent to one where it causes a crash. These docs (unlike a
PEP) are not a formal
STINNER Victor added the comment:
(I don' like the idea of documenting
possible behaviors, because it's non-portable, and really might change
in a future version).
The description doesn't need to be precise, you can just say depending on the
platform, closing a file descriptor while
Guido van Rossum added the comment:
I think you're looking for the discussion in issue 19017.
IIRC the conclusion is that not only do you not get the same error everywhere,
but you get it at different points -- sometimes register() of a bad FD passes
and then [Selector.]select() fails, other
Charles-François Natali added the comment:
Guido van Rossum added the comment:
I think you're looking for the discussion in issue 19017.
IIRC the conclusion is that not only do you not get the same error
everywhere, but you get it at different points -- sometimes register() of a
bad FD
STINNER Victor added the comment:
import selectors, os
r,w=os.pipe()
s=selectors.SelectSelector()
s.register(r, selectors.EVENT_READ)
SelectorKey(fileobj=3, fd=3, events=1, data=None)
os.close(r)
os.close(w)
s.unregister(r)
SelectorKey(fileobj=3, fd=3, events=1, data=None)
Guido van Rossum added the comment:
Heh, I'd forgotten the behavior of unregister(). It seems that there are two
layers to the behavior -- if this FD was never register()ed it will raise; if
it was register()ed but has since been close()d it may raise.
For the higher-level APIs in asyncio I
Guido van Rossum added the comment:
(What I meant to add was, I'd be happy if unregister() also just used a
true/false return.)
--
___
Python tracker rep...@bugs.python.org
http://bugs.python.org/issue19876
___
44 matches
Mail list logo
