selectors (and asyncio?): document behaviour on closed files/sockets

[vstinner]

BPO	19876
Nosy	@gvanrossum, @vstinner
Files	unregister.diff unregister2.diff unregister3.diff unregister4.diff unregister5.diff unregister6.diff unregister7.diff unregister8.diff epoll_except.patch nodup2.diff

^{Note: these values reflect the state of the issue at the time it was migrated and might not reflect the current state.}

Show more details

GitHub fields:

assignee = 'https://github.com/gvanrossum'
closed_at = <Date 2013-12-09.18:28:31.125>
created_at = <Date 2013-12-03.14:11:19.380>
labels = ['type-bug', 'docs']
title = 'selectors (and asyncio?): document behaviour on closed files/sockets'
updated_at = <Date 2013-12-09.18:28:31.124>
user = 'https://github.com/vstinner'

bugs.python.org fields:

activity = <Date 2013-12-09.18:28:31.124>
actor = 'gvanrossum'
assignee = 'gvanrossum'
closed = True
closed_date = <Date 2013-12-09.18:28:31.125>
closer = 'gvanrossum'
components = ['Documentation']
creation = <Date 2013-12-03.14:11:19.380>
creator = 'vstinner'
dependencies = []
files = ['32999', '33013', '33014', '33015', '33020', '33025', '33028', '33029', '33045', '33048']
hgrepos = []
issue_num = 19876
keywords = ['patch']
message_count = 44.0
messages = ['205118', '205123', '205134', '205135', '205136', '205142', '205160', '205162', '205165', '205166', '205230', '205233', '205333', '205382', '205385', '205406', '205408', '205409', '205410', '205411', '205432', '205433', '205434', '205438', '205468', '205478', '205483', '205496', '205499', '205500', '205501', '205502', '205529', '205546', '205551', '205606', '205607', '205608', '205631', '205632', '205633', '205724', '205725', '205726']
nosy_count = 5.0
nosy_names = ['gvanrossum', 'vstinner', 'neologix', 'docs@python', 'python-dev']
pr_nums = []
priority = 'normal'
resolution = 'fixed'
stage = 'resolved'
status = 'closed'
superseder = None
type = 'behavior'
url = 'https://bugs.python.org/issue19876'
versions = ['Python 3.4']

[vstinner]

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 documented. I would accept an "undefined behaviour" if it's documented :-)

[gvanrossum]

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.

[neologix]

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 what else to say (I don' like the idea of documenting
possible behaviors, because it's non-portable, and really might change
in a future version).

[gvanrossum]

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 standard but documentation for users. Maybe we can add a separate section of caveats?

[vstinner]

"(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 selector.select() is polling may be ignored or raise an exception". Which can of exception is raised? It is possible to catch it and find the closed file descriptor?

[gvanrossum]

I think you're looking for the discussion in bpo-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 times register() of a bad FD fails; when the FD is initially good and then gets closed, sometimes select() may fail, sometimes select() will silently ignore the FD. Sometimes unregister() of a closed FD will return False, sometimes True.

Another consequence is that registering an FD, then closing it, then calling select(), then reopening it may keep reporting events for the FD or not.

I think these are all things to call out in a section on caveats or common bugs.

[neologix]

Guido van Rossum added the comment:

I think you're looking for the discussion in bpo-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 times register() of a bad FD fails; when the FD is initially good and then gets closed, sometimes select() may fail, sometimes select() will silently ignore the FD. Sometimes unregister() of a closed FD will return False, sometimes True.

Another consequence is that registering an FD, then closing it, then calling select(), then reopening it may keep reporting events for the FD or not.

Exactly, it's a mess.

I think these are all things to call out in a section on caveats or common bugs.

What I don't remember was the conclusion: do we want to keep the
current OS-specific behavior, or do we want to try to be tolerant with
misuse?
For example, one possibility would be to ignore errors when
unregistering a file descriptor from epoll: for example, the
selectmodule currently ignore EBADF when unregistering a FD:
"""
        case EPOLL_CTL_DEL:
        /* In kernel versions before 2.6.9, the EPOLL_CTL_DEL
         * operation required a non-NULL pointer in event, even
         * though this argument is ignored. */
        Py_BEGIN_ALLOW_THREADS
        result = epoll_ctl(epfd, op, fd, &ev);
        if (errno == EBADF) {
            /* fd already closed */
            result = 0;
            errno = 0;
        }
"""

IIRC libev and libevent both ignore those errors.

We have to settle on a solution before documenting it.

[vstinner]

>>> 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)

SelectSelector.unregister(<closed fd>) doesn't raise any error, so it makes sense to ignore EBADF in EpollSelector.unregister(). What's the point of raising an error here?

If you want a portable behaviour, the file descriptor should be tested (ex: call os.fstat or os.dup). For the "normal" use case, I would not expect a syscall on unregister(), because unregister() may be called just before closing the file descriptor.

Maybe you can explain that in the "caveats" section? Suggestion: "unregister(fd) does not check if fd is closed or not (to get a portable behaviour), os.fstat() or os.dup() can be used to check if the fd is closed or not". Or "unregister(fd) ignores error if the file descriptor is closed".

[gvanrossum]

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 chose not to raise from the remove_{reader,writer}() methods -- they return True if something was removed, False if not. This currently has to be implemented by explicitly asking the selector for the key first. I.e.:

    def remove_reader(self, fd):
        """Remove a reader callback."""
        try:
            key = self._selector.get_key(fd)
        except KeyError:
            return False
        else:
            mask, (reader, writer) = key.events, key.data
            mask &= ~selectors.EVENT_READ
            if not mask:
                self._selector.unregister(fd)
            else:
                self._selector.modify(fd, mask, (None, writer))

            if reader is not None:
                reader.cancel()
                return True
            else:
                return False

[gvanrossum]

(What I meant to add was, I'd be happy if unregister() also just used a true/false return.)

[gvanrossum]

I just ran into a live case of the platform differences here. Check out http://bugs.python.org/review/19509/ (bpo-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 the unregister() call for the closed FD had been silent instead of raising.

I think that the proper fix might have been not to close the socket, but nevertheless this failure confused everyone -- the author of the patch thought it had to do with the SSL version, I was initially confused by the first half of the traceback (which turned out to be expected, this was in an assertRaises() call), and I spent half an hour in pdb to track down the real cause.

[gvanrossum]

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).

I believe that once this side effect has happened the unregister() call should return with success even if the kqueue syscall fails with OSError.

A further refinement could be to skip the kqueue syscall *if* the registered object is in fact an object with a fileno() method and not a bare FD, and the object is closed -- we should be able to tell that by calling its fileno() method, which should return -1 or None if it is closed. (But this would be mostly an optimization -- and a safety guard in case the FD has been reused for a different object.)

I don't know how poll and epoll behave under these circumstances, but given that only the Kqueue-based asyncio test failed I think those don't raise when the FD has been closed.

If you are not amenable to this fix I will have to catch the OSError in Tulip's remove_reader(), e.g. like this:

        try:
            if not mask:
                self._selector.unregister(fd)
            else:
                self._selector.modify(fd, mask, (None, writer))
        except OSError:
            # unregister()/modify() may or may not raise this if
            # the FD is closed -- it depends on what type of
            # selector is used.
            pass

(and repeated in remove_writer()).

[gvanrossum]

Here's a tentative change to selectors.py that ignores the OSError in various unregister() methods (but not in register()).