Repository navigation
Fix EINTR Socket Module issues in 2.7 #68051
Description
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 2017-09-06.22:45:57.341>
created_at = <Date 2015-04-04.02:51:25.263>
labels = ['type-bug']
title = 'Fix EINTR Socket Module issues in 2.7'
updated_at = <Date 2017-09-06.22:45:57.340>
user = 'https://bugs.python.org/mcjeff'bugs.python.org fields:
activity = <Date 2017-09-06.22:45:57.340>
actor = 'vstinner'
assignee = 'none'
closed = True
closed_date = <Date 2017-09-06.22:45:57.341>
closer = 'vstinner'
components = []
creation = <Date 2015-04-04.02:51:25.263>
creator = 'mcjeff'
dependencies = []
files = ['38825', '38826', '38832', '38865', '38873', '38874', '38883', '38891', '38944']
hgrepos = []
issue_num = 23863
keywords = ['patch', 'needs review']
message_count = 30.0
messages = ['240044', '240045', '240083', '240093', '240094', '240234', '240272', '240273', '240310', '240313', '240347', '240382', '240407', '240441', '240597', '240692', '240698', '240860', '240879', '241646', '241658', '241742', '241766', '241854', '241905', '241986', '241987', '243760', '301517', '301534']
nosy_count = 7.0
nosy_names = ['gregory.p.smith', 'vstinner', 'benjamin.peterson', 'r.david.murray', 'mcjeff', 'koobs', 'rpcope1']
pr_nums = []
priority = 'normal'
resolution = 'wont fix'
stage = 'resolved'
status = 'closed'
superseder = None
type = 'behavior'
url = 'https://bugs.python.org/issue23863'
versions = ['Python 2.7']
Activity
mcjeff commentedon Apr 4, 2015
There are a collection of places in the socket module that do not correctly retry on EINTR. Updated to wrap those calls in a retry loop. However, when fixing connect calls, I noticed that when EINTR is retried on a socket with a timeout specified, the retry fails with EALREADY.. so I fixed that.
I was going to shy away from primitive calls on sockets as one expects these things when working at a lower level, however, due to the way socket timeouts were implemented, I handled it differently in internal_connect. The create_connection calls probably ought to shield users from retry.
Python 2.7.6.
mcjeff commentedon Apr 4, 2015
mcjeff@mcjeff:~/cpython_clean$ hg summary
parent: 95416:fe34dfea16b0
Escaped backslashes in docstrings.
branch: 2.7
commit: 3 modified, 3 unknown
update: (current)
mcjeff commentedon Apr 4, 2015
Whoops. Accidentally attached the wrong patch that I generated during testing.
vstinner commentedon Apr 4, 2015
I have a very good news for you: this issue and more generally all EINTR
issues will be solved in Python 3.5. See the PEP-475.
I'm not really interested to fix Python 2.7.
gpshead commentedon Apr 4, 2015
You may not be, but I am. :). Jeff is aware of PEP-475.
Thanks for the awesome work on the real cleanup of this stuff in 3.5.
Sanity at last.
gpshead commentedon Apr 7, 2015
I like the socketmodule.c part of socket_eintr.1.patch, but it appears to still have the issue haypo describes in https://bugs.python.org/issue20611#msg240194 where connect() cannot be called more than once. The kernel carries on with the connect without our process waiting for it, we have to monitor it to know when it has succeeded or failed.
See https://hg.python.org/cpython/file/85a5265909cb/Modules/socketmodule.c#l2610 and bpo-23618 from Python 3.5.
[code review done at 35,000ft, thanks chromebook gogo free wifi pass!]
mcjeff commentedon Apr 8, 2015
Missed check on _ex func.
mcjeff commentedon Apr 8, 2015
So, yeah, that's right. In the attached patch, I'm closing the file descriptor if the timeout/error happens on a non-blocking call. It fails with an EBADF on reconnect at that point, but it doesn't potentially leave an FD in the proc's file table.
Should be no more EINTR's coming out of the select call.
vstinner commentedon Apr 9, 2015
Ok, since you look to want to fix Python 2.7, I can help you to handle EINTR in socket.connect() since I fixed Python 3.5.
If Python 2.7 is fixed, Python 3.4 should also be fixed.
connect_eintr-py27.patch: Patch for Python 2.7 to handle EINTR in socket.connect() for blocking socket or socket with a timeout. The patch calls select() to wait until the connection completes or fails.
Use attached test_connect_eintr4.py to test it.
I only tested my patch on Linux yet.
vstinner commentedon Apr 9, 2015
socket_eintr.2.patch has an issue with timeout.
socket_eintr.2.patch retries a socket method when it is interrupted, but it doesn't recompute the timeout. If a program is interrupted every second by a signal, the signal handler doesn't raise an exception and the socket has a timeout longer than 1 second, the socket method will "hang".
Internally, socket method uses select() using the socket timeout. I had to modify deeply the socket module to handle correctly EINTR and respect the timeout in Python 3.5. By the way, I changed the behaviour of socket.sendall(), the timeout is now more strict (it's now the maximum total duration, the timeout is no reset each time some bytes are sent).
Since Python applications of the last 20 years already had to handle EINTR theirself, it's maybe safer to leave Python 2.7 with its bugs and let application developers workaround them? Anyway, if you have to handle EINTR in your application, you will have to handle EINTR explicitly to support Python 2.7.9 and older, no? It's hard to require an exact minor version on all platforms.
The PEP-475 is wider than just the socket module:
https://docs.python.org/dev/whatsnew/3.5.html#changes-in-the-python-api
Impacted Python modules: io, os, select, socket, time. (Python 2.7 don't have faulthandler and don't have signal.sigtimedwait() nor signal.sigwaitinfo() functions, so faulthandler and signal are not impacted in Python 2.7.)
Do you want to fix all these modules?
mcjeff commentedon Apr 9, 2015
Updated to recalculate timeout at Python level. The current module already works this way on recv() calls. See attached.
I'd be happy to churn through and fix the other modules (using the 3.5 work as a guide), though I think only addressing the higher level abstractions makes sense (I think that's been noted elsewhere). For example, the _fileobject wrappers, but not the recv from sock_recv_guts.
vstinner commentedon Apr 9, 2015
It is risky to modify so much code. The PEP-475 also has an impact on backward compatibility:
https://www.python.org/dev/peps/pep-0475/#backward-compatibility
IMO it must be discussed on python-dev.
mcjeff commentedon Apr 10, 2015
Sure, to be clear --- the intention wouldn't be to create any backwards incompatible changes. Rather, as a means to identify what needs to be addressed.
5 remaining items
mcjeff commentedon Apr 14, 2015
I'm not a big fan of the settimeout approach myself (and only did it because it was mentioned as a possible approach). I think the existing implementations of EINTR retry also suffer from the same issue in which the timeout isn't adjusted per iteration (but that's okay, see below).
The _fileobject explicitly states only to use a blocking socket (i.e. one without a timeout set), so in practice, that shouldn't be a problem. I'd like to ensure the rest of the calls in that class take the same approach (thus the retryable call function, originally without the settimeout code) as they're a higher level abstraction above recv/send.
The only other call in socket.py that also qualifies as a higher abstraction is create_connection. If we could apply the 2.7 patch you created, connect ought to be correct at that point. All that remains after that would be isolating _retryable_call to _fileobject calls -- sans the settimeout -- which requires a blocking socket anyway. In retrospect, I probably should have just placed that call in _fileobject anyway.
I think that addresses most of what I'd like to fix. Of course, I'm happy to go through and weave PEP-475 into the socketmodule.c code entirely, but if the code churn creates too much worry, I think the above is a good medium.
bitdancer commentedon Apr 20, 2015
Is there a bug being fixed here? I mean other than socket not handling EINTR, where I think we agree that handling it is a feature, given the PEP.
vstinner commentedon Apr 20, 2015
See also issue bpo-20611 "socket.create_connection() doesn't handle EINTR properly" which was closed as duplicated of this issue.
I'm not sure that bpo-20611 is a duplicate.
mcjeff commentedon Apr 21, 2015
Fixing the underlying connect call should also address EINTR failing with a "operation already in progress" when a connect+timeout fails due to a signal.
I can understand not addressing EINTR generically (though it is already partially addressed in 2.7's socket.py - this just completes it), but IMO, not handling it on connect & responding with a seemingly unrelated error is the wrong thing to do.
gpshead commentedon Apr 22, 2015
... Code review:
In socket_eintr.5.patch I don't think the thread safety concerns about the s.settimeout() calls being done from Python code should be an issue but I'll ponder that for a bit. When you've got a socket s, why would code ever be calling s.recv() in one thread while calling another method on the same socket s in another thread? That seems unwise. (For UDP it might be a valid thing to do but it still has an odd smell to it)
If you're worried about timer resolution, just check that the new timeout value "deadline - now" is less than the previous timeout value and if not, ensure the value decreases by some small amount. So long as it continually goes down, it will eventually timeout. Even if the duration isn't accurate in the face of a bunch of EINTRs, this is a degenerate case; it should still be reasonable behavior.
On POSIX systems using os.times()[4] rather than an absolute time.time(), which can be changed out from underneath the process (even though that is rare on functioning systems), could be useful. But conditionally using that seems overly complex. I wouldn't bother.
... Side discussion
Some things in 2.7 related to EINTR have already been fixed in the past few years such as data loss in readline() and IIRC writelines(). Correctness isn't a feature.
Do you consider it an API change to prevent an error that nobody relies on or even expects (as PEP-475 notes) and virtually no code is written to properly handle when it _does_ happen? I don't. It is a bug fix. It's mostly a matter of if we can do it sanely and in a maintainable manner.
Please don't WONTFIX this issue, I intend to get safe fixes in.
... Motivation
The underlying motivation here is to enable the use of a signal based sampling profiler on a program that is using Python 2.7 (entirely, or embedded as one part of a larger C/C++ application). Signals (SIGPROF) used in that scenario cause EINTR returns from syscalls that otherwise rarely (read: never) have them in most environments. Result: Bugs show up making such a sampling profiler impossible to deploy in practice until those issues are fixed. Fixing the Python standard library is high value as it is a higher up place where these can be handled properly as you cannot even use some standard library modules at all otherwise because the unhandled EINTR surfaces too late or causes other unrecoverable errors internally before you see it.
vstinner commentedon Apr 23, 2015
This is just wrong. os.times() doesn't use seconds, but an "CPU time" which is unrelated. socket.settimeout() uses seconds.
By the way, as I wrote before: using the system clock here may introduce bugs when the system clock goes forward to backward, but Python 2.7 doesn't have a monotonic clock :-( That's another reason why I didn't want to fix Python 2.7 or 3.4.
I'm concerned by backward compatibility. Trivial changes was made in Python 2.7 and then came back as regressions because it broke several applications. See recent discussions on this topic on python-dev. Handling EINTR and recompute the timeout is not a trivial changes, some applications rely heavily on the current behaviour of sockets. Network servers using Twisted, eventlet and other libraries.
When we say "threads", also think about greenthreads which are used in eventlet to handle sockets.
readline()/writelines() don't have a timeout, so it was easier to handle EINTR there.
For your information, some people already contacted me because their application behaves differently on Python 3.5 (with the PEP-475). Well, it was easy to modify their code to not rely on EINTR, but it means that "Applications relying on the fact that system calls are interrupted with InterruptedError will hang" exists in the wild.
While it's acceptable to change such thing in Python 3.5, a new major version, I asked if it's acceptable for a minor version.
By the way, Python 3.5 is not released yet, so we can expect more feedback on PEP-475 changes when Python 3.5 will be released (other regressions in applications?).
I suggest WONTFIX for Python 2.7, this issue is *already* fixed in Python 3.5. Come on, in 2015 it's time to upgrade to Python 3!
gpshead commentedon Apr 24, 2015
diverging discussion: Go re-read the documentation on os.times(). It is plural, it isn't just CPU time. (on POSIX os.times()[4] is likely to be the system uptime in seconds as a float... it cannot be changed like the absolute clock can, it is a relative monotonic clock).
vstinner commentedon Apr 24, 2015
We don't have the same definition of the unit "seconds" :-)
My watch doesn't stop when I'm sleeping.
See https://docs.python.org/dev/library/time.html#time.process_time
See also "CPU Time" in https://www.python.org/dev/peps/pep-0418/#glossary
vstinner commentedon Apr 24, 2015
Or, please forget my comment, I watched the first 4 items of os.times(). I didn't notice the difference of the 5th item, os.times()[4].
The bad news is that only the first two items of os.times() are filled on Windows :-( I don't think that Python 2.7 provides a monotonic clock on Windows.
mcjeff commentedon May 21, 2015
Do we have a decision on this yet? I'm willing to rework bits that may need it, but I'd like to know whether this is going to be a fruitful effort or not. Thanks!
gpshead commentedon Sep 6, 2017
Unassigning because i'm unlikely to get to this in 2.7, it is better to concentrate on ensuring that 3.x stays signal safe.
vstinner commentedon Sep 6, 2017
I am closing the issue as WONTFIX for all the reasons listen in my previous comments.