Patch for signal.set_wakeup_fd

[Rhamphoryncus]

BPO	1583
Nosy	@birkenfeld, @vstinner, @jdemeyer
Files	python2.6-set_wakeup_fd1.diff python2.6-set_wakeup_fd2.diff python2.6-set_wakeup_fd3.diff python2.6-set_wakeup_fd4.diff

^{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']

[Rhamphoryncus]

This adds signal.set_wakeup_fd(fd), which allows a single library to be
woken when a signal comes in.

[Rhamphoryncus]

Guido, it looks like I can't alter the Assigned To field. You get the
Nosy List instead. ;)

[gvanrossum]

Thanks!

Can you add a doc patch too? Doc/library/signal.rst

[Rhamphoryncus]

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]

'\0' will be correct.

[Rhamphoryncus]

Thanks georg.

[gustavo]

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]

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]

Here's an updated patch that applies smoothly to the current trunk.

[gvanrossum]

Committed revision 59574.

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

Thanks all!

[jdemeyer]

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]

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]

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.

[Rhamphoryncus]

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.

[jdemeyer]

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]

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.

[Rhamphoryncus]

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]

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.

[Rhamphoryncus]

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]

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.

[Rhamphoryncus]

signalmodule.c has a hack to limit it to the main thread. Otherwise there's all sorts of platform-specific behaviour.

[jdemeyer]

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.

[vstinner]

Please don't discuss on closed issues. Either reopen the issue or open a new issue.