Repository navigation
Patch for signal.set_wakeup_fd #45924
Description
| BPO | 1583 |
|---|---|
| Nosy | @birkenfeld, @vstinner, @jdemeyer |
| 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-19.19:41:39.688>
created_at = <Date 2007-12-10.23:20:05.669>
labels = ['extension-modules']
title = 'Patch for signal.set_wakeup_fd'
updated_at = <Date 2019-04-11.11:04:48.554>
user = 'https://bugs.python.org/Rhamphoryncus'bugs.python.org fields:
activity = <Date 2019-04-11.11:04:48.554>
actor = 'vstinner'
assignee = 'gvanrossum'
closed = True
closed_date = <Date 2007-12-19.19:41:39.688>
closer = 'gvanrossum'
components = ['Extension Modules']
creation = <Date 2007-12-10.23:20:05.669>
creator = 'Rhamphoryncus'
dependencies = []
files = ['8915', '8916', '8925', '8988']
hgrepos = []
issue_num = 1583
keywords = ['patch']
message_count = 23.0
messages = ['58385', '58386', '58387', '58392', '58438', '58442', '58662', '58663', '58784', '58819', '339740', '339766', '339799', '339802', '339805', '339806', '339807', '339826', '339871', '339892', '339897', '339962', '339963']
nosy_count = 5.0
nosy_names = ['georg.brandl', 'Rhamphoryncus', 'gustavo', 'vstinner', 'jdemeyer']
pr_nums = []
priority = 'normal'
resolution = 'accepted'
stage = None
status = 'closed'
superseder = None
type = None
url = 'https://bugs.python.org/issue1583'
versions = ['Python 2.6']
Activity
Rhamphoryncus commentedon Dec 10, 2007
This adds signal.set_wakeup_fd(fd), which allows a single library to be
woken when a signal comes in.
Rhamphoryncus commentedon Dec 10, 2007
Guido, it looks like I can't alter the Assigned To field. You get the
Nosy List instead. ;)
gvanrossum commentedon Dec 10, 2007
Thanks!
Can you add a doc patch too? Doc/library/signal.rst
Rhamphoryncus commentedon Dec 10, 2007
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.
birkenfeld commentedon Dec 11, 2007
'\0'will be correct.Rhamphoryncus commentedon Dec 11, 2007
Thanks georg.
gustavo commentedon Dec 15, 2007
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.
Rhamphoryncus commentedon Dec 15, 2007
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);
gvanrossum commentedon Dec 18, 2007
Here's an updated patch that applies smoothly to the current trunk.
gvanrossum commentedon Dec 19, 2007
Committed revision 59574.
I added a simple C API as well, PySignal_SetWakeupFd(fd).
Thanks all!
jdemeyer commentedon Apr 9, 2019
Why is this using type "sig_atomic_t" for a file descriptor instead of "int" (which is the type of file descriptors)? See #12670
Rhamphoryncus commentedon Apr 9, 2019
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.
jdemeyer commentedon Apr 9, 2019
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.
Rhamphoryncus commentedon Apr 9, 2019
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:
jdemeyer commentedon Apr 9, 2019
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".
jdemeyer commentedon Apr 9, 2019
What do you mean with this? You can't write a complete array atomically, so I don't see how this would help.
Rhamphoryncus commentedon Apr 9, 2019
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'.
jdemeyer commentedon Apr 10, 2019
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.
Rhamphoryncus commentedon Apr 10, 2019
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.
jdemeyer commentedon Apr 10, 2019
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.
Rhamphoryncus commentedon Apr 10, 2019
signalmodule.c has a hack to limit it to the main thread. Otherwise there's all sorts of platform-specific behaviour.
jdemeyer commentedon Apr 11, 2019
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.
vstinner commentedon Apr 11, 2019
Please don't discuss on closed issues. Either reopen the issue or open a new issue.