handle EINTR in the stdlib

@gvanrossum

BPO	18885
Nosy	@gvanrossum, @arigo, @gpshead, @pitrou, @vstinner, @larryhastings, @giampaolo, @vadmium, @koobs
Files	select_eintr.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 = None
closed_at = <Date 2015-06-09.10:04:36.339>
created_at = <Date 2013-08-30.15:02:35.265>
labels = ['type-feature']
title = 'handle EINTR in the stdlib'
updated_at = <Date 2015-06-09.10:04:36.338>
user = 'https://bugs.python.org/neologix'

bugs.python.org fields:

activity = <Date 2015-06-09.10:04:36.338>
actor = 'vstinner'
assignee = 'none'
closed = True
closed_date = <Date 2015-06-09.10:04:36.339>
closer = 'vstinner'
components = []
creation = <Date 2013-08-30.15:02:35.265>
creator = 'neologix'
dependencies = []
files = ['32907']
hgrepos = []
issue_num = 18885
keywords = ['patch', 'needs review']
message_count = 28.0
messages = ['196555', '196646', '196647', '196648', '196653', '196661', '198681', '204816', '204855', '204858', '204863', '204865', '204868', '204872', '204875', '204878', '204890', '204906', '204907', '204912', '204913', '204949', '204953', '204962', '224339', '235543', '244892', '245055']
nosy_count = 13.0
nosy_names = ['gvanrossum', 'arigo', 'gregory.p.smith', 'pitrou', 'vstinner', 'larry', 'giampaolo.rodola', 'fossilet', 'neologix', 'sbt', 'martin.panter', 'piotr.dobrogost', 'koobs']
pr_nums = []
priority = 'normal'
resolution = 'fixed'
stage = 'patch review'
status = 'closed'
superseder = None
type = 'enhancement'
url = 'https://bugs.python.org/issue18885'
versions = ['Python 3.5']

As discussed in http://mail.python.org/pipermail/python-dev/2013-August/128204.html, I think that we shouldn't let EINTR leak to Python code: it should be handled properly by the C code, so that users (and the Python part of the stdlib) don't have to worry about this low-level historical nuisance.

For code that doesn't release the GIL, we could simply use this glibc macro:
# define TEMP_FAILURE_RETRY(expression) \
(extension \
({ long int __result; \
do __result = (long int) (expression); \
while (__result == -1L && errno == EINTR); \
__result; }))
#endif

Now, I'm not sure about how to best handle this for code that releases the GIL.

Basically:

Py_BEGIN_ALLOW_THREADS
pid = waitpid(pid, &status, options);
Py_END_ALLOW_THREADS

should become

begin_handle_eintr:
Py_BEGIN_ALLOW_THREADS
pid = waitpid(pid, &status, options);
Py_END_ALLOW_THREADS

        if (pid < 0 && errno == EINTR) {
            if (PyErr_CheckSignals())
                return NULL;
            goto begin_handle_eintr;
        }

Should we do this with a macro?

If yes, should it be a new one that should be placed around Py_BEGIN_ALLOW_THREADS/Py_END_ALLOW_THREADS (like BEGIN_SELECT_LOOP in selectmodule.c) or could we have a single macro that would do both (i.e. release the GIL / reacquire the GIL, and try again in case of EINTR, unless a signal handler raised an exception)?

From a cursory look, the main files affected would be:
Modules/fcntlmodule.c
Modules/ossaudiodev.c
Modules/posixmodule.c
Modules/selectmodule.c
Modules/selectmodule.c
Modules/signalmodule.c
Modules/socketmodule.c
Modules/syslogmodule.c

FYI - use the changes made in http://bugs.python.org/issue12268 as a guide for how to deal with EINTR properly at the C level. See the _PyIO_trap_eintr() function for example.

See also _eintr_retry_call() in Lib/subprocess.py.

FWIW, there are times when we *want* the interrupted system call to return control to Python rather than retrying the call. If someone is making a Python equivalent of the low level system call such as select() or poll(), the EINTR should be exposed for Python code to handle.

Things like time.sleep() are documented as sleeping for less time when a signal has arrived even though an exception may not be raised. People have written code which depends on this behavior so adding an EINTR retry for the remaining sleep time would break some programs.

Getting an EINTR errno does *not* mean you can simply retry the system calls with the exact same arguments. ie: If you did that with the select() call within time.sleep it'd be trivial to make the process sleep forever by sending it signals with a frequency less than the sleep time.

Gregory, thanks, that's what I was planning to do.

But since the recent discussions (mainly on selectors), there are points I obviously don't - and won't - agree with (such as select() returning EINTR or returning early, same for sleep()), I'm not interested in this anymore.
Anyone interested can pick this up, though.

(BTW, as for applications relying on EINTR being returned, I'm positive *way more applications* will break because of the recent change making file descriptors close-on-exec by default).

FWIW, there are times when we *want* the interrupted system call to
return control to Python rather than retrying the call.

I'm a bit curious, do you know of any use cases?

If someone is making a Python equivalent of the low level system call
such as select() or poll(), the EINTR should be exposed for Python
code to handle.

As mentioned in another issue, you would use a special wakeup fd to
wakeup select() or poll() calls.

Getting an EINTR errno does *not* mean you can simply retry the system
calls with the exact same arguments. ie: If you did that with the
select() call within time.sleep it'd be trivial to make the process
sleep forever by sending it signals with a frequency less than the
sleep time.

Indeed. That's already done in e.g. socketmodule.c : take a look at the
BEGIN_SELECT_LOOP / END_SELECT_LOOP macros.

On Sat, Aug 31, 2013 at 9:56 AM, Charles-François Natali
<report@bugs.python.org> wrote:

Charles-François Natali added the comment:

Gregory, thanks, that's what I was planning to do.

But since the recent discussions (mainly on selectors), there are points I obviously don't - and won't - agree with (such as select() returning EINTR or returning early, same for sleep()), I'm not interested in this anymore.

Whoa. Maybe you're overreacting a bit? I personally see a big divide
here between system calls whose functionality includes sleeping (e.g.
sleep(), poll(), select()) and those that just want some I/O to
complete (e.g. recv(), send(), read(), write()). The former are almost
always used in a context that can handle premature returns just fine,
since the return value for a premature return is the same as for
hitting the deadline, and the timeout is often used just as a hint
anyway. It's the latter category (recv() etc.) where the EINTR return
is problematic, and I think for many of those the automatic retry
(after the Python-level signal handler has been run and conditional on
it not raising an exception) will be a big improvement.

Anyone interested can pick this up, though.

(BTW, as for applications relying on EINTR being returned, I'm positive *way more applications* will break because of the recent change making file descriptors close-on-exec by default).

Again, I'd make a distinction: I agree for send(), recv() etc., but I
don't think there are many buggy uses of select()/poll() timeouts
around. (And even if there are, I still think it's better to fix these
by correcting the retry logic in the framework or the application,
since it may have other considerations.)

I wrote too many words. In short:

time.sleep()'s behavior should remain as it is today given how it is documented to behave. If you disagree, consider adding an optional interruptable=False parameter so that both behavior options exist.

ALL IO calls and wait* should handle EINTR transparently for the user and never expose it to the Python application.

select(), poll() and equivalents. If you want to transparently handle EINTR on these, just make sure you deal with the timeouts properly. While I suspect a few people wanted to see the signal interruption on those I agree: very uncommon and undesirable for most.

If people need a specific signal interruption they should define a signal handler that raises.

(replying to Guido's post in another thread)

Charles-Francois, sorry to add you back to the bug, but (a) I thought you had agreed to a compromise patch that restarts signals in most cases but not for select(), poll() etc.; (b) I may have found a flaw in the idea.
The flaw (if it is one) is related to Py_AddPendingCall(). This "schedules" a pending callback, mostly for signals, but doesn't AFAICT interrupt the mainthread in any way. (TBH, I only understand the code for Python 2.7, and in that version I'm sure it doesn't.)

So is this a flaw? I'm nor sure. Can you think about it?

I don't think that's a problem: the way I was planning to tackle signals is to call PyErr_CheckSignals() before retrying upon EINTR: this runs signal handlers, and returns a non 0 value if an exception occured (e.g. KeyboardInterrupt): if that's the case, then we simply break out of the loop, and let the exception bubble up.
See e.g. http://hg.python.org/cpython/file/default/Modules/socketmodule.c#l3397

Alright, here's a first step: select/poll/epoll/etc now return empty
lists/tuples upon EINTR. This comes with tests (note that all those tests
could probably be factored, but that's another story).

Am I correct in thinking that you're simply replacing the OSError(EINTR) with returning empty lists? This is bound to subtly break code, e.g. the code that expects reasonably that a return value of three empty lists means the timeout really ran out (i.e. the version of the code that is already the most careful). Shouldn't you restart the poll with the remaining time until timeout?

I wouldn't call that "being the most careful". I've always had an implicit understanding that calls with timeouts may, for whatever reason, return sooner than requested (or later!), and the most careful approach is to re-check the clock again.

I've always had an implicit understanding that calls with timeouts may,
for whatever reason, return sooner than requested (or later!), and the
most careful approach is to re-check the clock again.

I've always had the implicit understanding that if I use an *infinite* timeout then the call will not timeout.

> I've always had an implicit understanding that calls with timeouts may,
> for whatever reason, return sooner than requested (or later!), and the
> most careful approach is to re-check the clock again.

I've always had the implicit understanding that if I use an *infinite*
timeout then the call will not timeout.

Wow, that's a good point. select() and friends are not documented to
exhibit successful spurious wakeups. It would be a pretty strong
compatibility breach if they started doing so.

If we don't want select() to silently retry on EINTR, then I think we
should leave it alone.

Speaking of which, I see that SelectSelector.select() returns an empty
list when interrupted, but this is nowhere documented.

I've always had an implicit understanding that calls with timeouts may, for whatever reason, return sooner than requested (or later!), and the most careful approach is to re-check the clock again.

exactly. at the system call level you can be interrupted. re-checking the clock is the right thing to do if the elapsed time actually matters.

If we don't want select() to silently retry on EINTR, then I think we
should leave it alone.

We should go ahead and retry for the user for select/poll/epoll/kqueue. If they care about being able to break out of that low level call due to a signal, they should set a signal handler which raises an exception. I have *never* seen code intentionally get an EINTR exception from a select or poll call and have often seen code tripped up because it or a library it was using forgot to handle it.

We're a high level language: Lets be sane by default and do the most desirable thing for the user. Retry the call internally with a safely adjusted timeout:
new_timeout = min(original_timeout, time_now-start_time)
if new_timeout <= 0:
return an empty list # ie: the system clock changed
retry the call with new_timeout

We went through this whole discussion before. Returning immediately with three empty lists is better than raising InterruptedError. Retrying is not always better.

Guido's point was that it is already a bug in code to not check the elapsed
time after a select call returns rather than assuming the full timeout time
has elapsed. Correct code today already needs to deal with both situations
(OSError(EINTR) and select returning an empty set before the desired time
has elapsed) because both can happen on existing systems today. So correct
code in the future wishing to be compatible with older Pythons will need to
continue to do so.

As for "presumably have been annoyed by the occasional OSError(EINTR) and
fix that bug" that isn't always true. EINTRs are not guaranteed to happen
and are likely to crop up on different systems (production systems) long
after you've deployed and successfully run your code as they are something
that happens due to things _outside_ of the control of your deployed
program: signals.

That's what has gotten me on a kick to hide EINTR from python developers
when at all possible.

For the record: I am perfectly fine with select and friends returning an
empty set early on EINTR (as Guido seems to prefer). If this worries some
people lets at least highlight this in the documentation as part of this
change.

What I don't want is to ever see OSError(EINTR) in the future.

Just for the record, I was initially in favor of recomputing the
timeout and retrying upon EINTR, but Guido prefers to return empty
lists, and since that's a better compromise than the current situation
(I've seen *many* people complaining on EINTR popping-up at random
points in the code, including myself), I went ahead and implemented
it.

AFAICT, an early return for calls such as poll()/epoll() etc is
something which is definitely acceptable: if you have a look at e.g.
Tornado, Twisted & Co, they all return empty lists on EINTR.

I've always had the implicit understanding that if I use an *infinite* timeout then > the call will not timeout.

Well, I've always assumed that time.sleep(n) would sleep n seconds, but:
"""
static int
floatsleep(double secs)
[...]
    Py_BEGIN_ALLOW_THREADS
    err = select(0, (fd_set *)0, (fd_set *)0, (fd_set *)0, &t);
    Py_END_ALLOW_THREADS
    if (err != 0) {
#ifdef EINTR
        if (errno == EINTR) {
            if (PyErr_CheckSignals())
                return -1;
        }
        else
#endif
        {
            PyErr_SetFromErrno(PyExc_IOError);
            return -1;
        }
    }
[...]
"""

So really, I'm like Gregory: I don't care which solution we chose, but
I just don't want to have to let the user handle EINTR.

Guido's point was that it is already a bug in code to not check the elapsed
time after a select call returns rather than assuming the full timeout time
has elapsed.

I don't understand how it's a bug. You're assuming select() has
unreliable timing, but it doesn't (if you are using the same clock).

On dim., 2013-12-01 at 08:14 +0000, Charles-François Natali wrote:

So really, I'm like Gregory: I don't care which solution we chose, but
I just don't want to have to let the user handle EINTR.

Well this is wishing thinking, since by returning an empty list you
force the user to handle EINTR - just in a different way.

Well this is wishing thinking, since by returning an empty list you
force the user to handle EINTR - just in a different way.

I know that returning an empty list changes the semantics: I just
think that's better - or not as bad - than the current possibility of
having any single piece of code possibly die upon EINTR.

If you want to implement retry with timeout re-computation, I'm not
the one to who must be convinced :-)

(BTW, if we go this way, then time.sleep() should probably also be
fixed to retry upon EINTR).

I know that returning an empty list changes the semantics: I just
think that's better - or not as bad - than the current possibility of
having any single piece of code possibly die upon EINTR.

If you want to implement retry with timeout re-computation, I'm not
the one to who must be convinced :-)

Or, since we now have the selectors module, we could let select() live
with the current semantics.

By the way, it's already too late for 3.4, which is in feature freeze.

I do not consider this a feature; that EINTR is exposed as an exception from the API is a bug. But Larry is the only one who can actually make that decision as the 3.4 release manager (+nosy'd).

by returning an empty list you force the user to handle EINTR -
just in a different way.

The user now only has one thing to deal with instead of two: an empty list being returned; something they should already have been dealing with. Gone will be the OSError(EINTR) exception as a rare, often never tested for, alternate form of the same retry needed indication.

I never see code intentionally wanting to receive and handle an OSError(EINTR) exception but I constantly run into code that is buggy due to some library it is using not getting this right... Where it isn't up to the code exhibiting the problem because the only place to fix it is within the library they use that is outside of that code's control.

We've got the opportunity to fix this nit once and for all here, lets do it.

I do not consider this a feature; that EINTR is exposed as an
exception from the API is a bug.

select() currently works as specified; you are proposing a
compatibility-breaking change to the API, not a bugfix.

We're left with the fact that the API is inconvenient: but we now have
the selectors module and can advocate that instead of breaking existing
code during a feature freeze period.

(or we can retry on EINTR, which has the benefit of not creating new
situations to deal with in existing code)

The user now only has one thing to deal with instead of two: an empty
list being returned; something they should already have been dealing
with.

Returning an empty list when no timeout has been passed has never been a
feature of select(), which is why users are not expected to be dealing
with it.

I don't want this checked in to 3.4.

(Congratulations, this is my first "no" as a release manager!)

FYI Charles-François and me are working on a PEP to address this issue: the PEP-475. The PEP is not ready yet for a review.

Navigation Menu

Repository navigation

handle EINTR in the stdlib #63085

1 remaining item

Metadata

Assignees

Labels

Projects

Milestone

Relationships

Development

handle EINTR in the stdlib #63085

Description

Activity

neologix commented on Aug 30, 2013

gpshead commented on Aug 31, 2013

neologix commented on Aug 31, 2013

pitrou commented on Aug 31, 2013

gvanrossum commented on Aug 31, 2013

gpshead commented on Aug 31, 2013

neologix commented on Sep 30, 2013

neologix commented on Nov 30, 2013

arigo commented on Nov 30, 2013

gvanrossum commented on Nov 30, 2013

sbt commented on Dec 1, 2013

pitrou commented on Dec 1, 2013

gpshead commented on Dec 1, 2013

gvanrossum commented on Dec 1, 2013

1 remaining item

gpshead commented on Dec 1, 2013

neologix commented on Dec 1, 2013

pitrou commented on Dec 1, 2013

pitrou commented on Dec 1, 2013

neologix commented on Dec 1, 2013

pitrou commented on Dec 1, 2013

gpshead commented on Dec 1, 2013

pitrou commented on Dec 1, 2013

larryhastings commented on Dec 1, 2013

vstinner commented on Jul 30, 2014

vadmium commented on Feb 8, 2015

vadmium commented on Jun 6, 2015

vstinner commented on Jun 9, 2015

Metadata

Metadata

Assignees

Labels

Projects

Milestone

Relationships

Development

Issue actions