Patches item #1564547, was opened at 2006-09-24 15:13 Message generated for change (Comment added) made by gustavo 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 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: Gustavo J. A. M. Carneiro (gustavo) Date: 2006-09-28 16: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 03: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 15: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 14: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 00: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 _______________________________________________ Patches mailing list Patches@python.org http://mail.python.org/mailman/listinfo/patches
