[issue19850] asyncio: limit EINTR occurrences with SA_RESTART
Roundup Robot added the comment: New changeset 546cad3627e2 by Charles-François Natali in branch 'default': Issue #19850: asyncio: Set SA_RESTART when registering a signal handler to http://hg.python.org/cpython/rev/546cad3627e2 -- nosy: +python-dev ___ Python tracker rep...@bugs.python.org http://bugs.python.org/issue19850 ___ ___ Python-bugs-list mailing list Unsubscribe: https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com
[issue19850] asyncio: limit EINTR occurrences with SA_RESTART
Charles-François Natali added the comment: Thanks! -- resolution: - fixed stage: patch review - committed/rejected status: open - closed ___ Python tracker rep...@bugs.python.org http://bugs.python.org/issue19850 ___ ___ Python-bugs-list mailing list Unsubscribe: https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com
[issue19850] asyncio: limit EINTR occurrences with SA_RESTART
Anthony Baire added the comment: The patch is fine, but it is hard to rely on it to prevent bugs from happening because that requires cooperation from all modules registering signal handlers. Anyway it facilitates reusing code that was not written for an event-driven context (and many will do that through .run_in_executor()). If the patch is accepted, it would be wise to write a note in .run_in_executor()'s doc saying that asyncio uses SA_RESTART by default in all its handler and that EINTR is prevented *as long as* no other handlers are registered elsewhere. -- nosy: +aba ___ Python tracker rep...@bugs.python.org http://bugs.python.org/issue19850 ___ ___ Python-bugs-list mailing list Unsubscribe: https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com
[issue19850] asyncio: limit EINTR occurrences with SA_RESTART
STINNER Victor added the comment: SA_RESTART doesn't need to be enforced. It's better to use it, but selectors and asyncio modules already handle EINTR error. -- ___ Python tracker rep...@bugs.python.org http://bugs.python.org/issue19850 ___ ___ Python-bugs-list mailing list Unsubscribe: https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com
[issue19850] asyncio: limit EINTR occurrences with SA_RESTART
Guido van Rossum added the comment: Please answer this question. Under what circumstances can a signal handler interrupt a blocking system call in a thread that is not the main thread? -- ___ Python tracker rep...@bugs.python.org http://bugs.python.org/issue19850 ___ ___ Python-bugs-list mailing list Unsubscribe: https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com
[issue19850] asyncio: limit EINTR occurrences with SA_RESTART
STINNER Victor added the comment: 2013/12/3 Guido van Rossum rep...@bugs.python.org: Please answer this question. Under what circumstances can a signal handler interrupt a blocking system call in a thread that is not the main thread? There is no guarantee that the signal handler is called in the main thread. On FreeBSD, if I remember correctly, it is called in a random thread. You can control which thread gets the signal using signal.pthread_sigmask() (block signals in other threads) and signal.pthread_kill() (send a signal a specific thread). -- ___ Python tracker rep...@bugs.python.org http://bugs.python.org/issue19850 ___ ___ Python-bugs-list mailing list Unsubscribe: https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com
[issue19850] asyncio: limit EINTR occurrences with SA_RESTART
Charles-François Natali added the comment: The patch is fine, but it is hard to rely on it to prevent bugs from happening because that requires cooperation from all modules registering signal handlers. Once again, that's why the bug report says *limit* EINTR occurrences. We all know this won't prevent all occurrences. Anyway it facilitates reusing code that was not written for an event-driven context (and many will do that through .run_in_executor()). If the patch is accepted, it would be wise to write a note in .run_in_executor()'s doc saying that asyncio uses SA_RESTART by default in all its handler and that EINTR is prevented *as long as* no other handlers are registered elsewhere. The single most common cause of signals is SIGCHLD (in a normal code). Since asyncio setups up the SIGCHLD handler, this should cover most of the cases (BTW, just set up a SIGCHLD handler in any Python process spawning subprocesses, and it becomes unusable since EINTR isn't handled in the stdlib). Please answer this question. Under what circumstances can a signal handler interrupt a blocking system call in a thread that is not the main thread? I answered in my prevous message: POSIX makes no guarantee whatsoever as to which thread will receive the signal (except of course in case of synchronous signales like SIGSEGV/SIGFPE). The Linux kernel makes it best to deliver it to the main thread when possible, but in certains cases it can't, and other OS just don't bother. See e.g. issue #19857 for an occurrence on FreeBSD. Also, even the main thread can sometimes make blocking calls subject to EINTR (e.g. acquiring a lock). So the possibility of breakage are real, but if people prefer to wait see, that's fine :-) -- ___ Python tracker rep...@bugs.python.org http://bugs.python.org/issue19850 ___ ___ Python-bugs-list mailing list Unsubscribe: https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com
[issue19850] asyncio: limit EINTR occurrences with SA_RESTART
Guido van Rossum added the comment: OK, I've harassed you enough. Sorry. Go ahead and commit this. -- ___ Python tracker rep...@bugs.python.org http://bugs.python.org/issue19850 ___ ___ Python-bugs-list mailing list Unsubscribe: https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com
[issue19850] asyncio: limit EINTR occurrences with SA_RESTART
STINNER Victor added the comment: asyncio_sa_restart.diff looks good to me, it cannot make the situation worse. signal.siginterrupt() looks to be available on all non-Windows buildbots and working correctly: test_signal tests it and the test pass. -- nosy: +haypo ___ Python tracker rep...@bugs.python.org http://bugs.python.org/issue19850 ___ ___ Python-bugs-list mailing list Unsubscribe: https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com
[issue19850] asyncio: limit EINTR occurrences with SA_RESTART
Guido van Rossum added the comment: I have a question. Is there actually any need for this with asyncio? The selector already handles EINTR, and all the I/O done by asyncio's transports already handles it too (there are except (BlockingIOError, InterruptedError) clauses all over the place). Any *other* I/O syscalls (unless a program violates the asyncio rules against doing your own blocking I/O) would either be disk file I/O, which IIUC cannot elicit EINTR, or run in a separate thread, where presumably it wouldn't be interrupted by a signal handler, since SIGCHLD is delivered to the main thread. (It's actually the last part I am not 100% sure of.) -- ___ Python tracker rep...@bugs.python.org http://bugs.python.org/issue19850 ___ ___ Python-bugs-list mailing list Unsubscribe: https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com
[issue19850] asyncio: limit EINTR occurrences with SA_RESTART
STINNER Victor added the comment: I have a question. Is there actually any need for this with asyncio? I believe that the libc and the kernel knows better than Python how to restart a syscalls, than Python. I expect more reliable timeout, or the kernel may avoid context switches (don't wake up the process). In practice, you should not see any difference. Or maybe only on some corner cases. But do you want to handle these corner cases while there is already a portable flag for them? :-) -- ___ Python tracker rep...@bugs.python.org http://bugs.python.org/issue19850 ___ ___ Python-bugs-list mailing list Unsubscribe: https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com
[issue19850] asyncio: limit EINTR occurrences with SA_RESTART
Guido van Rossum added the comment: That's just rhetoric -- I am beginning to believe that nobody has any data. Here's some opposite rhetoric. If it ain't broke, don't fix it. Plus, if it's so much better, why isn't it the default? If you say for backward compatibility I will say exactly! -- ___ Python tracker rep...@bugs.python.org http://bugs.python.org/issue19850 ___ ___ Python-bugs-list mailing list Unsubscribe: https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com
[issue19850] asyncio: limit EINTR occurrences with SA_RESTART
Gregory P. Smith added the comment: I believe that the libc and the kernel knows better than Python how to restart a syscalls, than Python. I expect more reliable timeout, or the kernel may avoid context switches (don't wake up the process). See the man page i linked to, calls like select and poll with timeouts are not auto-restarted. -- ___ Python tracker rep...@bugs.python.org http://bugs.python.org/issue19850 ___ ___ Python-bugs-list mailing list Unsubscribe: https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com
[issue19850] asyncio: limit EINTR occurrences with SA_RESTART
Charles-François Natali added the comment:
Guido van Rossum added the comment:
That's just rhetoric -- I am beginning to believe that nobody has any data.
Here's some opposite rhetoric. If it ain't broke, don't fix it. Plus, if
it's so much better, why isn't it the default? If you say for backward
compatibility I will say exactly!
Well, it's the default on BSD and Linux, but Python deliberately disables it:
PyOS_setsig(int sig, PyOS_sighandler_t handler)
{
#ifdef HAVE_SIGACTION
/* Some code in Modules/signalmodule.c depends on sigaction() being
* used here if HAVE_SIGACTION is defined. Fix that if this code
* changes to invalidate that assumption.
*/
struct sigaction context, ocontext;
context.sa_handler = handler;
sigemptyset(context.sa_mask);
context.sa_flags = 0;
if (sigaction(sig, context, ocontext) == -1)
return SIG_ERR;
return ocontext.sa_handler;
#else
PyOS_sighandler_t oldhandler;
oldhandler = signal(sig, handler);
#ifdef HAVE_SIGINTERRUPT
siginterrupt(sig, 1);
#endif
return oldhandler;
#endif
}
It's done because we want syscalls to return with EINTR to be able to
run signal handlers, but since asyncio uses a wakeup file descriptor,
it's unnecessary here.
Any *other* I/O syscalls (unless a program violates the asyncio rules against
doing your own blocking I/O) would either be disk file I/O, which IIUC cannot
elicit EINTR, or run in a separate thread, where presumably it wouldn't be
interrupted by a signal handler, since SIGCHLD is delivered to the main
thread.
(It's actually the last part I am not 100% sure of.)
In order:
- Linux avoids EINTR on file I/O, but that's not necessarily the case
on other OS (e.g. on NFS)
- Many syscalls can return EINTR, not only I/O related, e.g. waitpid().
- As for threads, there's absolutely no guarantee that the signal will
be delivered to the main thread.
--
___
Python tracker rep...@bugs.python.org
http://bugs.python.org/issue19850
___
___
Python-bugs-list mailing list
Unsubscribe:
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com
[issue19850] asyncio: limit EINTR occurrences with SA_RESTART
New submission from Charles-François Natali: asyncio makes heavy use of SIGCHLD for subprocesses. A consequence of this if that a lot of syscalls can fail with EINTR (see e.g. issue #18885). The attached patch uses SA_RESTART (through signal.siginterrupt()) to limit EINTR occurrences, e.g. : $ ./python -c import os; from signal import *; signal(SIGALRM, lambda *args: None); alarm(1); os.read(0, 1) Traceback (most recent call last): File string, line 1, in module InterruptedError: [Errno 4] Interrupted system call With siginterrupt(False): $ ./python -c import os; from signal import *; signal(SIGALRM, lambda *args: None); siginterrupt(SIGALRM, False); alarm(1); os.read(0, 1) [manual interruption] ^CTraceback (most recent call last): File string, line 1, in module KeyboardInterrupt It doesn't come with test because it's hard to come up with a syscall that's guaranteed to be restarted on al OS (and also because I'm having a hard time with all those mock-based asyncio tests ;-), but at least it doesn't break the test suite :-) See https://groups.google.com/d/topic/python-tulip/9T2_tGWe0Sc/discussion for more background. -- components: Library (Lib) files: asyncio_sa_restart.diff keywords: patch messages: 204915 nosy: gvanrossum, neologix priority: normal severity: normal stage: patch review status: open title: asyncio: limit EINTR occurrences with SA_RESTART type: enhancement versions: Python 3.4 Added file: http://bugs.python.org/file32923/asyncio_sa_restart.diff ___ Python tracker rep...@bugs.python.org http://bugs.python.org/issue19850 ___ ___ Python-bugs-list mailing list Unsubscribe: https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com
[issue19850] asyncio: limit EINTR occurrences with SA_RESTART
Guido van Rossum added the comment: Do you haven an example of a program using asyncio that fails because of this? Adding gps because he seems to agree that EINTR must die. -- nosy: +gregory.p.smith ___ Python tracker rep...@bugs.python.org http://bugs.python.org/issue19850 ___ ___ Python-bugs-list mailing list Unsubscribe: https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com
[issue19850] asyncio: limit EINTR occurrences with SA_RESTART
Gregory P. Smith added the comment: It sounds like doing this is fine (as Glyph suggests in the email thread) but it isn't magically going to solve all EINTR problems, just reduce the number of calls that could encounter them. http://man7.org/linux/man-pages/man7/signal.7.html see the Interruption of system calls and library functions section. Note that with this SA_RESTART flag set via siginterrupt you _may_ be making a trade off between being able to process python signal handlers during long reads or writes vs having to wait until the entire thing has finished. given that, I'm ambivalent about this patch being worthy. (re: Explicitly testing this, it is hard. for the broader Python test suite as a whole I've been pondering a regrtest mode that'll continually pester all of the test processes with signals to try and expose EINTR issues. Low on my TODO ideas list, I haven't written code to do it.) -- ___ Python tracker rep...@bugs.python.org http://bugs.python.org/issue19850 ___ ___ Python-bugs-list mailing list Unsubscribe: https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com
[issue19850] asyncio: limit EINTR occurrences with SA_RESTART
Charles-François Natali added the comment: It sounds like doing this is fine (as Glyph suggests in the email thread) but it isn't magically going to solve all EINTR problems, just reduce the number of calls that could encounter them. Indeed, hence *limit* EINTR occurrences :-) Note that with this SA_RESTART flag set via siginterrupt you _may_ be making a trade off between being able to process python signal handlers during long reads or writes vs having to wait until the entire thing has finished. asyncio uses a wakeup FD, registered in the main event loop: so as soon as a signal is received, the main loop will wake up and run the signal handler. So this would only be a problem if you were doing a blocking syscall from the main loop thread, which would be a really bad idea anyway. -- ___ Python tracker rep...@bugs.python.org http://bugs.python.org/issue19850 ___ ___ Python-bugs-list mailing list Unsubscribe: https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com
