classification
Title: Patch for signal.set_wakeup_fd
Type: Stage:
Components: Extension Modules Versions: Python 2.6
process
Status: closed Resolution: accepted
Dependencies: Superseder:
Assigned To: gvanrossum Nosy List: Rhamphoryncus, georg.brandl, gustavo, jdemeyer, vstinner
Priority: normal Keywords: patch

Created on 2007-12-10 23:20 by Rhamphoryncus, last changed 2019-04-11 11:04 by vstinner. This issue is now closed.

Files
File name Uploaded Description Edit
python2.6-set_wakeup_fd1.diff Rhamphoryncus, 2007-12-10 23:20
python2.6-set_wakeup_fd2.diff Rhamphoryncus, 2007-12-10 23:54
python2.6-set_wakeup_fd3.diff Rhamphoryncus, 2007-12-11 17:25
python2.6-set_wakeup_fd4.diff gvanrossum, 2007-12-18 22:44
Messages (23)
msg58385 - (view) Author: Adam Olsen (Rhamphoryncus) Date: 2007-12-10 23:20
This adds signal.set_wakeup_fd(fd), which allows a single library to be
woken when a signal comes in.
msg58386 - (view) Author: Adam Olsen (Rhamphoryncus) Date: 2007-12-10 23:23
Guido, it looks like I can't alter the Assigned To field.  You get the
Nosy List instead. ;)
msg58387 - (view) Author: Guido van Rossum (gvanrossum) * (Python committer) Date: 2007-12-10 23:28
Thanks!

Can you add a doc patch too?  Doc/library/signal.rst
msg58392 - (view) Author: Adam Olsen (Rhamphoryncus) Date: 2007-12-10 23:54
version 2, adds to Doc/library/signal.rst.  It also tweaks the
set_wakeup_fd's docstring.

I haven't verified that my formatting in signal.rst is correct. 
Specifically, the '\0' should be checked.
msg58438 - (view) Author: Georg Brandl (georg.brandl) * (Python committer) Date: 2007-12-11 16:27
``'\0'`` will be correct.
msg58442 - (view) Author: Adam Olsen (Rhamphoryncus) Date: 2007-12-11 17:25
Thanks georg.
msg58662 - (view) Author: Gustavo J. A. M. Carneiro (gustavo) * Date: 2007-12-15 14:50
The patch looks great.

But I was wondering if there is any chance to export a C API in addition
to the Python one?  That is because PyGTK is mostly C, the code that
needs this is C, it would be easier to call a C API.
msg58663 - (view) Author: Adam Olsen (Rhamphoryncus) Date: 2007-12-15 17:35
The python API has the advantage that you can test for it at runtime,
avoiding a compile-time check.  I don't know if this is significant though.

I don't see the big deal about a C API.  All you need to do is call
PyImport_ImportModule("signal") and PyObject_CallMethod(mod,
"set_wakeup_fd", "i", fd);
msg58784 - (view) Author: Guido van Rossum (gvanrossum) * (Python committer) Date: 2007-12-18 22:44
Here's an updated patch that applies smoothly to the current trunk.
msg58819 - (view) Author: Guido van Rossum (gvanrossum) * (Python committer) Date: 2007-12-19 19:41
Committed revision 59574.

I added a simple C API as well, PySignal_SetWakeupFd(fd).

Thanks all!
msg339740 - (view) Author: Jeroen Demeyer (jdemeyer) * Date: 2019-04-09 12:49
Why is this using type "sig_atomic_t" for a file descriptor instead of "int" (which is the type of file descriptors)? See https://github.com/python/cpython/pull/12670
msg339766 - (view) Author: Adam Olsen (Rhamphoryncus) Date: 2019-04-09 15:52
The fd field may be written from the main thread simultaneous with the signal handler activating and reading it out.  Back in 2007 the only POSIX-compliant type allowed for that was sig_atomic_t, anything else was undefined.

Looks like pycore_atomic.h should have alternatives now but I'm not at all familiar with it.
msg339799 - (view) Author: Jeroen Demeyer (jdemeyer) * Date: 2019-04-09 18:17
> Back in 2007 the only POSIX-compliant type allowed for that was sig_atomic_t, anything else was undefined.

Fair enough, but having a non-atomic type is still much better than a completely wrong type. In other words, the requirement of fitting a file descriptor is more important than being atomic.
msg339802 - (view) Author: Adam Olsen (Rhamphoryncus) Date: 2019-04-09 18:55
Disagree; if you're writing signal-handling code you should be very careful to do it properly, even if that's only proper for your current platform.  If you can't do it properly you should find an alternative that doesn't involve signals.

The fact that sig_atomic_t is only 1 byte on VxWorks strongly implies using int WILL fail in strange ways on that platform.  I can see three options:

1) use pycore_atomic.h, implementing it for VxWorks if you haven't already.  This also implies sig_atomic_t could have been int but wasn't for some reason, such as performance.
2) disable wakeup_fd entirely.  It's obscure, GNOME being the biggest user I can think of.
3) unpack the int into an array of sig_atomic_t.  Only the main thread writes to it so this method is ugly but viable.
msg339805 - (view) Author: Jeroen Demeyer (jdemeyer) * Date: 2019-04-09 20:40
I'm not sure with what you disagree. At least, you have to admit that using sig_atomic_t is buggy for different reasons than signal safety, namely that there is no guarantee that one can safely convert back and forth to an "int".
msg339806 - (view) Author: Jeroen Demeyer (jdemeyer) * Date: 2019-04-09 20:41
> unpack the int into an array of sig_atomic_t.

What do you mean with this? You can't write a complete array atomically, so I don't see how this would help.
msg339807 - (view) Author: Adam Olsen (Rhamphoryncus) Date: 2019-04-09 21:03
Converting to/from sig_atomic_t could have a compile time check on currently supported platforms and isn't buggy for them.  For platforms with a different size you could do a runtime check, only allowing a fd in the range of 0-254 (with 255 reserved); that could sometimes fail, yes, but at least it's explicit, easily understood failure.  Just using int would fail in undefined ways down the road, likely writing to a random fd instead (corrupting whatever it was doing), with no way to trace it back.

Unpacking the int would mean having one sig_atomic_t for 'invalid', using that instead of INVALID_FD, plus an array of sig_atomic_t for the fd itself.  Every time you want to change the fd you first set the 'invalid' flag, then the individual bytes, then clear 'invalid'.
msg339826 - (view) Author: Jeroen Demeyer (jdemeyer) * Date: 2019-04-10 05:56
> Unpacking the int would mean having one sig_atomic_t for 'invalid', using that instead of INVALID_FD, plus an array of sig_atomic_t for the fd itself.  Every time you want to change the fd you first set the 'invalid' flag, then the individual bytes, then clear 'invalid'.

I'm not sure that this is thread-safe as processors can reorder instructions, so there is no guarantee that memory is written in the expected order. That's one of the problems that C11/C++11 atomics solve.
msg339871 - (view) Author: Adam Olsen (Rhamphoryncus) Date: 2019-04-10 15:30
signal-safe is different from thread-safe (despite conceptual similarities), but regardless it's been a long time since I last delved into this so I'm quite rusty.  I could be doing it all wrong.
msg339892 - (view) Author: Jeroen Demeyer (jdemeyer) * Date: 2019-04-10 21:10
> signal-safe is different from thread-safe

I know, but I think that other threads can receive signals too. So in that case, it needs to be signal-safe as well as thread-safe.
msg339897 - (view) Author: Adam Olsen (Rhamphoryncus) Date: 2019-04-10 22:51
signalmodule.c has a hack to limit it to the main thread.  Otherwise there's all sorts of platform-specific behaviour.
msg339962 - (view) Author: Jeroen Demeyer (jdemeyer) * Date: 2019-04-11 11:03
> signalmodule.c has a hack to limit it to the main thread.

The Python signal handler always runs in the main thread, but the signal can be caught by any thread. In other words, trip_signal() can be run by any thread.
msg339963 - (view) Author: STINNER Victor (vstinner) * (Python committer) Date: 2019-04-11 11:04
Please don't discuss on closed issues. Either reopen the issue or open a new issue.
History
Date User Action Args
2019-04-11 11:04:48vstinnersetnosy: + vstinner
messages: + msg339963
2019-04-11 11:03:06jdemeyersetmessages: + msg339962
2019-04-10 22:51:43Rhamphoryncussetmessages: + msg339897
2019-04-10 21:10:33jdemeyersetmessages: + msg339892
2019-04-10 16:00:48gvanrossumsetnosy: - gvanrossum
2019-04-10 15:30:41Rhamphoryncussetmessages: + msg339871
2019-04-10 05:56:12jdemeyersetmessages: + msg339826
2019-04-09 21:03:56Rhamphoryncussetmessages: + msg339807
2019-04-09 20:41:03jdemeyersetmessages: + msg339806
2019-04-09 20:40:09jdemeyersetmessages: + msg339805
2019-04-09 18:55:56Rhamphoryncussetmessages: + msg339802
2019-04-09 18:17:05jdemeyersetmessages: + msg339799
2019-04-09 15:52:08Rhamphoryncussetmessages: + msg339766
2019-04-09 12:49:04jdemeyersetnosy: + jdemeyer
messages: + msg339740
2007-12-19 19:41:39gvanrossumsetstatus: open -> closed
resolution: accepted
messages: + msg58819
2007-12-18 22:44:17gvanrossumsetfiles: + python2.6-set_wakeup_fd4.diff
messages: + msg58784
components: + Extension Modules
2007-12-15 17:35:23Rhamphoryncussetmessages: + msg58663
2007-12-15 14:50:32gustavosetnosy: gvanrossum, georg.brandl, Rhamphoryncus, gustavo
2007-12-15 14:50:13gustavosetnosy: + gustavo
messages: + msg58662
2007-12-11 17:25:58Rhamphoryncussetfiles: + python2.6-set_wakeup_fd3.diff
messages: + msg58442
2007-12-11 16:27:48georg.brandlsetnosy: + georg.brandl
messages: + msg58438
2007-12-10 23:54:18Rhamphoryncussetfiles: + python2.6-set_wakeup_fd2.diff
messages: + msg58392
2007-12-10 23:50:16gvanrossumlinkissue1564547 superseder
2007-12-10 23:44:33gvanrossumsetkeywords: + patch, - py3k
2007-12-10 23:44:25gvanrossumsetpriority: normal
2007-12-10 23:28:28gvanrossumsetkeywords: + py3k
assignee: gvanrossum
messages: + msg58387
versions: + Python 2.6
2007-12-10 23:23:16Rhamphoryncussetnosy: + gvanrossum
messages: + msg58386
2007-12-10 23:20:05Rhamphoryncuscreate