[Patches] [ python-Patches-1564547 ] Py_signal_pipe
SourceForge.net
noreply at sourceforge.net
Mon Jan 29 21:01:27 CET 2007
Patches item #1564547, was opened at 2006-09-24 16:13
Message generated for change (Comment added) made by loewis
You can respond by visiting:
https://sourceforge.net/tracker/?func=detail&atid=305470&aid=1564547&group_id=5470
Please note that this message will contain a full copy of the comment thread,
including the initial issue submission, for this request,
not just the latest update.
Category: Core (C code)
Group: Python 2.6
Status: Open
Resolution: None
Priority: 5
Private: No
Submitted By: Gustavo J. A. M. Carneiro (gustavo)
Assigned to: Nobody/Anonymous (nobody)
Summary: Py_signal_pipe
Initial Comment:
Problem: how to wakeup extension modules running poll()
so that they can let python check for signals.
Solution: use a pipe to communicate between signal
handlers and main thread. The read end of the pipe can
then be monitored by poll/select for input events and
wake up poll(). As a side benefit, it avoids the usage
of Py_AddPendingCall / Py_MakePendingCalls, which are
patently not "async safe".
All explained in this thread:
http://mail.python.org/pipermail/python-dev/2006-September/068569.html
----------------------------------------------------------------------
>Comment By: Martin v. Löwis (loewis)
Date: 2007-01-29 21:01
Message:
Logged In: YES
user_id=21627
Originator: NO
I see. I think this can be fixed fairly easily: install the signal
handlers with sigaction, and prevent any nested delivery of signals through
sa_mask. Then, no two signal handlers will get invoked simultaneously.
----------------------------------------------------------------------
Comment By: Gustavo J. A. M. Carneiro (gustavo)
Date: 2007-01-29 19:53
Message:
Logged In: YES
user_id=908
Originator: YES
Py_AddPendingCall is not async safe. It's obvious looking at the code,
and it even says so in a comment:
/* XXX Begin critical section */
/* XXX If you want this to be safe against nested
XXX asynchronous calls, you'll have to work harder! */
----------------------------------------------------------------------
Comment By: Martin v. Löwis (loewis)
Date: 2007-01-29 19:36
Message:
Logged In: YES
user_id=21627
Originator: NO
Can you please explain in what sense the current framework isn't "async
safe"? You might be referring to "async-signal-safe functions", which is a
term specified by POSIX, referring to functions that may be called in a
signal handler. The Python signal handler, signal_handler, calls these
functions:
* getpid
* Py_AddPendingCall
* PyOS_setsig
** sigemptyset
** sigaction
AFAICT, this is the complete list of functions called in a signal handler.
Of these, only getpid, sigemptyset, and sigaction are library functions,
and they are all specified as async-signal safe. So the current
implementation is async-signal safe.
Usage of pthread_kill wouldn't make it more platform-specific than your
patch. pthread_kill is part of the POSIX standard, and so is pipe(2). So
both changes work on a POSIX system, and neither change would be portable
if all you have is standard C.
----------------------------------------------------------------------
Comment By: Gustavo J. A. M. Carneiro (gustavo)
Date: 2007-01-29 12:07
Message:
Logged In: YES
user_id=908
Originator: YES
But if you think about it, support for other cases have to be extensions
of this patch. In an async handler it's not safe to do about anything.
The current framework is not async safe, it just happens to work most of
the time.
If we use pthread_kill we will start to enter platform-specific code; what
will happen in systems without POSIX threads? What signal do we use to
wake up the main thread? Do system calls that receive signals return EINTR
for this platform or not (can we guarantee it always happens)? Which one
is the main thread anyway?
In any case, anything we want to do can be layered on top of the
Py_signal_pipe API in a very safe way, because reading from a pipe is
decoupled from the async handler, therefore this handler is allowed to
safely do anything it wants, like pthread_kill. But IMHO that part should
be left out of Python; let the frameworks do it themselves.
----------------------------------------------------------------------
Comment By: Martin v. Löwis (loewis)
Date: 2007-01-29 09:41
Message:
Logged In: YES
user_id=21627
Originator: NO
I'm -1 on this patch. The introduction of a pipe makes it essentially
gtk-specific: It will only work with gtk (for a while, until other
frameworks catch up - which may take years), and it will only wake up a gtk
thread that is in the gtk poll call.
It fails to support cases where the main thread blocks in a different
blocking call (i.e. neither select nor poll). I think a better mechanism is
needed to support that case, e.g. by waking up the main thread with
pthread_kill.
----------------------------------------------------------------------
Comment By: Jp Calderone (kuran)
Date: 2007-01-25 20:22
Message:
Logged In: YES
user_id=366566
Originator: NO
The attached patch also fixes a bug in the order in which signal handlers
are run. Previously, they would be run in numerically ascending signal
number order. With the patch attached, they will be run in the order they
are processed by Python.
----------------------------------------------------------------------
Comment By: Adam Olsen (rhamphoryncus)
Date: 2007-01-25 19:38
Message:
Logged In: YES
user_id=12364
Originator: NO
gustavo, there's two patches attached and it's not entirely clear which
one is current. Please delete the older one.
----------------------------------------------------------------------
Comment By: Gustavo J. A. M. Carneiro (gustavo)
Date: 2007-01-25 19:11
Message:
Logged In: YES
user_id=908
Originator: YES
File Added: python-signals.diff
----------------------------------------------------------------------
Comment By: Gustavo J. A. M. Carneiro (gustavo)
Date: 2007-01-25 18:57
Message:
Logged In: YES
user_id=908
Originator: YES
Damn this SF bug tracker! ;(
The patch I uploaded (yes, it was me, not anonymous) fixes some bugs and
also fixes http://www.python.org/sf/1643738
----------------------------------------------------------------------
Comment By: Adam Olsen (rhamphoryncus)
Date: 2006-09-29 23:09
Message:
Logged In: YES
user_id=12364
I'm concerned about the interface to
PyOS_InterruptOccurred(). The original version peeked ahead
for only that signal, and handled it manually. No need to
report errors. The new version will first call arbitrary
python functions to handle any earlier signals, then an
arbitrary python function for the interrupt itself, and then
will not report any errors they produce. It may not even
get to the interrupt, even if one is waiting.
I'm not sure PyOS_InterruptOccurred() is called when
arbitrary python code is acceptable. I suspect it should be
dropped entierly, in favour of a more robust API.
Otoh, some of it appears quite crufty. One version in
intrcheck.c lacks a return statement, invoking undefined
behavior in C.
One other concern I have is that signalmodule.c should never
been unloaded, if loaded via dlopen. A delayed signal
handler may reference it indefinitely. However, I see no
sane way to enforce this.
----------------------------------------------------------------------
Comment By: Gustavo J. A. M. Carneiro (gustavo)
Date: 2006-09-28 17:31
Message:
Logged In: YES
user_id=908
> ...sizeof(char) will STILL return 1 in such a case...
Even if sizeof(char) == 1, 'sizeof(signum_c)' is much more
readable than just a plain '1'.
----------------------------------------------------------------------
Comment By: Adam Olsen (rhamphoryncus)
Date: 2006-09-28 04:50
Message:
Logged In: YES
user_id=12364
Any compiler where sizeof(char) != 1 is *deeply* broken. In
C, a byte isn't always 8 bits (if it uses bits at all!).
It's possible for a char to take (for instance) 32 bits, but
sizeof(char) will STILL return 1 in such a case. A mention
of this in the wild is here:
http://lkml.org/lkml/1998/1/22/4
If you find a compiler that's broken, I'd love to hear about
it. :)
# error Too many signals to fit on an unsigned char!
Should be "in", not "on" :)
A comment in signal_handler() about ignoring the return
value of write() may be good.
initsignal() should avoid not replace
Py_signal_pipe/Py_signal_pipe_w if called a second time
(which is possible, right?). If so, it should probably not
set them until after setting non-blocking mode.
check_signals() should not call
PyEval_CallObject(Handlers[signum].func, ...) if func is
NULL, which may happen after finisignal() clears it.
----------------------------------------------------------------------
Comment By: Gustavo J. A. M. Carneiro (gustavo)
Date: 2006-09-27 16:34
Message:
Logged In: YES
user_id=908
and of course this
> * PyErr_SetInterrupt() needs to set is_tripped after the
call to write(), not before.
is correct, good catch.
New patch uploaded.
----------------------------------------------------------------------
Comment By: Gustavo J. A. M. Carneiro (gustavo)
Date: 2006-09-27 15:42
Message:
Logged In: YES
user_id=908
> * Needs documentation ...
True, I'll try to add more documentation...
> * I think we should be more paranoid about the range of
possible signals. NSIG does not appear to be defined by
SUSv2 (no clue about Posix). We should size the Handlers
array to UCHAR_MAX and set any signals outside the range of
0..UCHAR_MAX to either 0 (null signal) or UCHAR_MAX. I'm
not sure we should ever use NSIG.
I disagree. Creating an array of size UCHAR_MAX is just
wasting memory. If you check the original python code,
there's already fallback code to define NSIG if it's not
already defined (if not defined, it could end up being
defines as 64).
> * In signal_hander() sizeof(signum_c) is inherently 1. ;)
And? I occasionally hear horror stories of platforms where
sizeof(char) != 1, I'm not taking any chances :)
> * PyOS_InterruptOccurred() should probably still check
that it's called from the main thread.
check_signals already bails out if that is the case. But in
fact it bails out without setting the interrupt_occurred
output parameter, so I fixed that.
fcntl error checking... will work on it.
----------------------------------------------------------------------
Comment By: Adam Olsen (rhamphoryncus)
Date: 2006-09-27 01:53
Message:
Logged In: YES
user_id=12364
I've looked over the patch, although I haven't tested it. I
have the following suggestions:
* Needs documentation explaining the signal weirdness (may
drop signals, may delay indefinitely, new handlers may get
signals intended for old, etc)
* Needs to be explicit that users must only poll/select to
check for readability of the pipe, NOT read from it
* The comment for is_tripped refers to sigcheck(), which
doesn't exist
* I think we should be more paranoid about the range of
possible signals. NSIG does not appear to be defined by
SUSv2 (no clue about Posix). We should size the Handlers
array to UCHAR_MAX and set any signals outside the range of
0..UCHAR_MAX to either 0 (null signal) or UCHAR_MAX. I'm
not sure we should ever use NSIG.
* In signal_hander() sizeof(signum_c) is inherently 1. ;)
* The set_nonblock macro doesn't check for errors from
fcntl(). I'm not sure it's worth having a macro for that
anyway.
* Needs some documentation of the assumptions about
read()/write() being memory barriers.
* In check_signals() sizeof(signum) is inherently 1.
* There's a blank line with tabs near the end of
check_signals() ;)
* PyErr_SetInterrupt() should use a compile-time check for
SIGINT being within 0..UCHAR_MAX, assuming NSIG is ripped
out entierly.
* PyErr_SetInterrupt() needs to set is_tripped after the
call to write(), not before.
* PyOS_InterruptOccurred() should probably still check that
it's called from the main thread.
----------------------------------------------------------------------
You can respond by visiting:
https://sourceforge.net/tracker/?func=detail&atid=305470&aid=1564547&group_id=5470
More information about the Patches
mailing list