Repository navigation
Py_signal_pipe #44020
Description
| BPO | 1564547 |
|---|---|
| Nosy | @mwhudson, @gvanrossum, @loewis |
| Superseder | |
| 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 = 'https://github.com/gvanrossum'
closed_at = <Date 2007-12-10.23:49:41.598>
created_at = <Date 2006-09-24.14:13:07.000>
labels = ['interpreter-core']
title = 'Py_signal_pipe'
updated_at = <Date 2007-12-10.23:50:16.648>
user = 'https://bugs.python.org/gustavo'bugs.python.org fields:
activity = <Date 2007-12-10.23:50:16.648>
actor = 'gvanrossum'
assignee = 'gvanrossum'
closed = True
closed_date = <Date 2007-12-10.23:49:41.598>
closer = 'gvanrossum'
components = ['Interpreter Core']
creation = <Date 2006-09-24.14:13:07.000>
creator = 'gustavo'
dependencies = []
files = ['7547', '8898']
hgrepos = []
issue_num = 1564547
keywords = ['patch']
message_count = 30.0
messages = ['51147', '51148', '51149', '51150', '51151', '51152', '51153', '51154', '51155', '51156', '51157', '51158', '51159', '51160', '51161', '51162', '51163', '51164', '51165', '51166', '51167', '51168', '51169', '58264', '58275', '58307', '58317', '58321', '58322', '58391']
nosy_count = 8.0
nosy_names = ['mwh', 'gvanrossum', 'loewis', 'anthonybaxter', 'jafo', 'exarkun', 'Rhamphoryncus', 'gustavo']
pr_nums = []
priority = 'normal'
resolution = 'rejected'
stage = None
status = 'closed'
superseder = '1583'
type = None
url = 'https://bugs.python.org/issue1564547'
versions = ['Python 2.6']
Activity
gustavo commentedon Sep 24, 2006
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
Rhamphoryncus commentedon Sep 26, 2006
Logged In: YES
user_id=12364
I've looked over the patch, although I haven't tested it. I
have the following suggestions:
drop signals, may delay indefinitely, new handlers may get
signals intended for old, etc)
check for readability of the pipe, NOT read from it
doesn't exist
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.
fcntl(). I'm not sure it's worth having a macro for that
anyway.
read()/write() being memory barriers.
check_signals() ;)
SIGINT being within 0..UCHAR_MAX, assuming NSIG is ripped
out entierly.
call to write(), not before.
it's called from the main thread.
gustavo commentedon Sep 27, 2006
Logged In: YES
user_id=908
True, I'll try to add more documentation...
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).
And? I occasionally hear horror stories of platforms where
sizeof(char) != 1, I'm not taking any chances :)
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.
gustavo commentedon Sep 27, 2006
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.
Rhamphoryncus commentedon Sep 28, 2006
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.
gustavo commentedon Sep 28, 2006
Logged In: YES
user_id=908
Even if sizeof(char) == 1, 'sizeof(signum_c)' is much more
readable than just a plain '1'.
Rhamphoryncus commentedon Sep 29, 2006
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.
gustavo commentedon Jan 25, 2007
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
gustavo commentedon Jan 25, 2007
File Added: python-signals.diff
Rhamphoryncus commentedon Jan 25, 2007
gustavo, there's two patches attached and it's not entirely clear which one is current. Please delete the older one.
exarkun commentedon Jan 25, 2007
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.
loewis commentedon Jan 29, 2007
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.
gustavo commentedon Jan 29, 2007
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.
11 remaining items
gustavo commentedon Dec 7, 2007
"My understanding is that there are some things that the current patch
does not address."
Well, I don't know what those things are, so it's hard for me to address
them. :-)
gvanrossum commentedon Dec 8, 2007
I've been told to look at this one more time.
gustavo commentedon Dec 9, 2007
Minimal patch that just adds the pipe but does not attempt to fix
anything else.
Rhamphoryncus commentedon Dec 9, 2007
The minimal patch doesn't initialize dummy_char or dummy_c. It's
harmless here, but please fix it. ;)
sizeof(dummy_char) will always be 1 (C defines sizeof as multiples of
char.) The convention seems to be hardcoding 1 instead.
gustavo commentedon Dec 9, 2007
The variable is called 'dummy' for a reason. The value written or read
is irrevelant...
gvanrossum commentedon Dec 10, 2007
Rejected, in favor of bpo-1583 which lets the app choose whether to use a
fd or not. We had an extensive #python-dev discussion on this and this
time the rejection is irrevocable.