Repository navigation
SocketServer doesn't handle syscall interruption #52226
Description
| BPO | 7978 |
|---|---|
| Nosy | @gpshead, @pitrou, @vstinner, @giampaolo |
| Files |
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 2012-04-08.23:25:19.590>
created_at = <Date 2010-02-21.19:14:53.658>
labels = ['type-bug', 'library']
title = "SocketServer doesn't handle syscall interruption"
updated_at = <Date 2012-04-08.23:48:11.811>
user = 'https://bugs.python.org/nvetoshkin'bugs.python.org fields:
activity = <Date 2012-04-08.23:48:11.811>
actor = 'pitrou'
assignee = 'none'
closed = True
closed_date = <Date 2012-04-08.23:25:19.590>
closer = 'pitrou'
components = ['Library (Lib)']
creation = <Date 2010-02-21.19:14:53.658>
creator = 'nvetoshkin'
dependencies = []
files = ['16755', '16813', '17120', '17314', '25144']
hgrepos = []
issue_num = 7978
keywords = ['patch']
message_count = 32.0
messages = ['99676', '100704', '102331', '102333', '102336', '102337', '102363', '102591', '102592', '102594', '102596', '102660', '102680', '102690', '102704', '104455', '104459', '105617', '120898', '125234', '137361', '137362', '137414', '157695', '157725', '157814', '157815', '157817', '157818', '157822', '157824', '157825']
nosy_count = 14.0
nosy_names = ['gregory.p.smith', 'spiv', 'exarkun', 'pitrou', 'vstinner', 'giampaolo.rodola', 'Christophe Simonis', 'nvetoshkin', 'neologix', 'Yaniv.Aknin', 'bda', 'kchen', 'Jerzy.Kozera', 'python-dev']
pr_nums = []
priority = 'normal'
resolution = 'fixed'
stage = 'resolved'
status = 'closed'
superseder = None
type = 'behavior'
url = 'https://bugs.python.org/issue7978'
versions = ['Python 2.7', 'Python 3.2', 'Python 3.3']
Activity
nvetoshkin commentedon Feb 21, 2010
SocketServer's handle_request function uses "select" call to handle io, but sending POSIX signal will result in 'Interrupted system call' exception raised. After that Paste (http://pythonpaste.org/) http server will crash.
I suppose EINTR must be handled properly (i.e. syscall must be restarted silently) on SocketServer's side. That must be pretty easy task.
nvetoshkin commentedon Mar 9, 2010
Wrapping select in (taken from twisted sources) can help:
def untilConcludes(f, *a, **kw):
while True:
try:
return f(*a, **kw)
except (IOError, OSError), e:
if e.args[0] == errno.EINTR:
continue
raise
neologix commentedon Apr 4, 2010
A patch to make select calls EINTR-safe is attached, but:
Thoughts ?
pitrou commentedon Apr 4, 2010
What kind of signals can be received in real-life?
(I'm assuming that most signals are of the kind that you only receive if someone else deliberately sends it to you, in which case you are supposed to be prepared to handle it)
Also, does your patch still allow Python to handle the signal properly (invoke a registered signal handler, or perhaps raise KeyboardInterrupt)?
You could raise the issue on the python-dev mailing-list for more advice.
neologix commentedon Apr 4, 2010
In real life, you can receive for example SIGSTOP (strace, gdb, shell), but mostly SIGCHLD (any process spawning children), etc. The attached patch just restarts calls when EINTR is received, as is done in subprocess module. The mailing list is a good idea.
exarkun commentedon Apr 4, 2010
There are lots of possible answers. Here's one. You launch a child process with os.fork() (and perhaps os.exec*() as well). Eventually you'll get a SIGCHLD in the parent when the child exits. If you've also installed a Python-level SIGCHLD handler with signal.signal (and not flagged the handler with SA_RESTART using signal.siginterrupt) then if you're in select.select() when the SIGCHLD is received, select.select() will fail with select.error EINTR.
If your Python SIGCHLD handler deals with the SIGCHLD completely before returning, then this lets you support child processes and use SocketServer at the same time.
Changing the behavior of select.select() ("a fix at select level would be better") should be *very* carefully considered, and probably rejected after such consideration. There are completely legitimate use cases which require select.select() to fail with EINTR, and these should remain supported (not to mention that existing code relying on this behavior shouldn't be broken by a Python upgrade in a quite insidious way).
nvetoshkin commentedon Apr 5, 2010
We use SIGUSR1 to reopen log files after rotation. sighandler works just fine, but after that Paste crashes.
I suppose that implementing silent syscall restart at select.select() level is a bad idea (explanation is given already), but some helper like untilConcludes in socket module might be useful.
YanivAknin commentedon Apr 8, 2010
First, let me cast my vote in favour of leaving select() itself alone, it's excessive to fix this issue with changing select().
Second, while exarkun's comments are accurate, I understand where pitrou is coming from. EINTR is not a pathological case, but it's not commonplace, either. I believe that on modern Unices (I tried Linux and OpenSolaris), the default handler for SIGCHLD will restart interrupted calls. By default, SIGUSR1 and most sane others, will kill the process anyway. So I can understand the purity of the argument that if a process sets a signal handler, it might just as well set SA_RESTART and be done with it.
That said, Imagine I write a forking server with SocketServer which sets a signal handler for SIGCHLD, and I don't set SA_RESTART because I don't think I need to. I believe this is a valid real-world scenario where (a) I don't expect an EINTER-related exception to be raised from SocketServer, (b) SocketServer can actually do something about it, and (c) I doubt the old EINTR behaviour is relied upon by anyone (now I'm /certain/ someone relying on this will pop-up...). I think its a safer bet on behalf of SocketServer (and subprocess, etc) to use something like untilConcludes.
If we agree so far, I believe that an implementation of untilConcludes *should* be added to stdlib ("signal.restartable_call", anyone?). If people agree, I'd be happy to produce the patch (trunk+py3k, doc+test). Other than SocketServer and subprocess, anything else that should use "untilConcludes"?
Finally, to answer pitrou's second question, I'm pretty sure untilConcludes as presented will not hurt other signal handlers. The interrupt handler will be executed before returning to untilConcludes' loop and manually restarting the call, so KeyboardInterrupt (or anything else signalistic) will behave as expected.
I'm attaching a toy 3.x program to play with select() and EINTR, I used it in writing this comment. Gee, it even has an OptionParser CLI to save you some commenting-out.
gpshead commentedon Apr 8, 2010
wrapping select in an eintr handler that loops like SocketServer_eintr.diff is fine.
nvetoshkin commentedon Apr 8, 2010
According to "man 7 signal" select must be explicitely restarted, regardless of the SA_RESTART flag.
14 remaining items
pitrou commentedon Apr 7, 2012
Jerzy's latest patch looks ok to me.
This is a slight behaviour change so I'm not sure it should go in 3.2/2.7.
python-dev commentedon Apr 8, 2012
New changeset 5493d44b56d8 by Antoine Pitrou in branch '3.2':
Issue bpo-7978: socketserver now restarts the select() call when EINTR is returned.
http://hg.python.org/cpython/rev/5493d44b56d8
New changeset 97a0c6230ece by Antoine Pitrou in branch 'default':
Issue bpo-7978: socketserver now restarts the select() call when EINTR is returned.
http://hg.python.org/cpython/rev/97a0c6230ece
python-dev commentedon Apr 8, 2012
New changeset d941d1fcc6e6 by Antoine Pitrou in branch '2.7':
Issue bpo-7978: socketserver now restarts the select() call when EINTR is returned.
http://hg.python.org/cpython/rev/d941d1fcc6e6
vstinner commentedon Apr 8, 2012
You must recompute the timeout when select() is interrupted: see issue bpo-12338.
pitrou commentedon Apr 8, 2012
Ah, right. It's a minor issue, though. I would suggest opening a separate issue for it. The patch is now committed to all 3 branches (I'm finally convinced this is a bug worth fixing), so I'm closing this issue.
JerzyKozera commentedon Apr 8, 2012
I forgot to mention my patch is 3.3-only, sorry - it depends on changes from bpo-12555 (http://hg.python.org/cpython/rev/41a1de81ef2b#l18.21 to be precise). To support 3.2 and 2.7:
(1) select.error must be caught as in the original patch,
(2) e.args[0] must be used - select.error doesn't have 'errno' attribute.
Should I prepare the patch for 3.2 and 2.7?
Regarding not updating the timeout, it was already mentioned above. Though as an afterthought, it might be worrying that if the process is receiving repeated signals with interval between them less than timeout, we might fall into infinite loop of select() when it should timeout, but that is probably very obscure case.
python-dev commentedon Apr 8, 2012
New changeset f8e7fcd581ff by Antoine Pitrou in branch '3.2':
Fix the patch for issue bpo-7978: select() raises select.error before 3.3, not OSError.
http://hg.python.org/cpython/rev/f8e7fcd581ff
New changeset 4298d6e79ecb by Antoine Pitrou in branch '2.7':
Fix the patch for issue bpo-7978: select() raises select.error before 3.3, not OSError.
http://hg.python.org/cpython/rev/4298d6e79ecb
pitrou commentedon Apr 8, 2012
It shouldn't be necessary, I've just committed fixes.
Thanks for noticing!