Python 3.3 adds timeout arguments to communicate() and wait() methods of subprocess.Popen, acquire() method of threading.Lock, and maybe more. I like (optional) timeouts, and so it would be nice to have sigtimedwait().

If we expose sigtimedwait(), it's trivial to expose sigwaitinfo(), so should expose both.

Here's a patch to add the two functions (with docs and tests).

You'll need to run autoreconf before compiling.

Cool, a patch! "Some" comments.

Why do you wait until the end of PyInit_signal() to set initialized to 1? If this variable is only present to call PyStructSequence_InitType() only once, you can set initialized to 1 directly in the if. Is it possible that PyInit_signal() is called more than once? C modules cannot be unloaded in Python, but I see that the init function of the posixmodule.c has also a static initialized variable.

Doc: The sigwaitinfo()/sigtimedwait() manual page contains interesting infos:

"(If one of the signals in set is already pending for the calling thread,  sigwaitinfo() will return immediately with information about that signal.)"

"If both fields of this structure are specified as  0,  a  poll  is  performed:  sigtimedwait() returns  immediately,  either with information about a signal that was pending for the caller, or with an error if none of the signals in set was pending."

The manpage doesn't tell that the signal handler is not called, should we say it in the Python doc?

Doc: you may add links between pause(), sigwait(), sigwaitinfo() and sigtimedwait() functions. We should maybe reorganise the signal doc to group functions. We need maybe a section for "pending signals" functions, functions to block or wait signals: pause(), pthread_sigmask(), sigpending(), sigwait(), sigwaitinfo(), sigtimedwait(). Another big theme of the signal module is signal handling. We may group functions by these two themes. Well, it is better to reorganize the doc is a second commit ;-)

The timeout is a tuple. Usually, Python uses float for timeout (e.g. see select.select). I suppose that you chose a tuple to keep the precision of the timespec structure. We may accept both types: (sec: int, nanosec: int) or sec: float. It would be nice to have the opinion of our time expect, Alexander Belopolsky.

It is possible to pass a negative timeout: the select() functions accepts or not negative timeout depending on the platform. In Python 3.3, select.select() now always raise an exception if the timeout is negative to have the same behaviour on all platforms. According to the Linux manual page, sigtimedwait() can return EINVAL if the timeout is invalid. We may also always raise an exception if the timeout is negative in sigtimedwait()?

signal_sigwaitinfo() and signal_sigtimedwait() use iterable_to_sigset() whereas this function is only compiled if "#if defined(PYPTHREAD_SIGMASK) || defined(HAVE_SIGWAIT)". You have to fix this test.

According to the manual page, sigwaitinfo() or sigtimedwait() can be interrupted (EINTR) by an unexpected signal (in signal not the signal set): you should call PyErr_CheckSignals(). You should add a test for this case.

Your patch doesn't touch configure nor pyconfig.h.in, only configure.in. Edit configure manually (better to limit the size of the patch) and run autoheader to regenerate pyconfig.h.in (or maybe edit manually pyconfig.h.in).

siginfo_t structure contains more fields, but I don't know if we need all of them. It can be improved later.

sigtimedwait() raises a OSError(EGAIN) on timeout. The behaviour must be documented, or we can choose another behaviour. We may simply return None in case of a timeout, it is just more practical to use than a try/except. For example, threading.Lock.acquire(timeout) simply returns False in case of a timeout. select.select() returns ([], [], []) in case of a timeout, not an exception.

test_sigtimedwait_timeout(): why do you call signal.alarm()? You may also add a test for timeout=0, I suppose that it is a special timeout value.

I will do a deeper review on the second version of your patch :-) Thanks again for the patch. I tried to write it, but I was bored of the siginfo_t fields (too many fields, and I didn't know how to expose them in Python).

Thanks for the review.

> Why do you wait until the end of PyInit_signal() to set initialized to 1?
Fixed.

> If this variable is only present ... but I see that the init function of the posixmodule.c has also a static initialized variable.
I simply followed the same format of the posix module.

> "(If one of the signals in set is already pending for the calling thread,  sigwaitinfo() will return immediately with information about that signal.)"
Added.

> "If both fields of this structure are specified as  0,  a  poll  is  performed:  sigtimedwait() returns  immediately,  either with information about a signal that was pending for the caller, or with an error if none of the signals in set was pending."
Added.

> The manpage doesn't tell that the signal handler is not called, should we say it in the Python doc?
Added - let's rather be more explicit.

> Doc: you may add links between pause(), sigwait(), sigwaitinfo() and sigtimedwait() functions.
Added.

> The timeout is a tuple. Usually, Python uses float for timeout (e.g. see select.select). I suppose that you chose a tuple to keep the precision of the timespec structure. We may accept both types: (sec: int, nanosec: int) or sec: float. It would be nice to have the opinion of our time expect, Alexander Belopolsky.
The are some uses of this tuple that were added in #10812 (e.g. futimens) that was reviewed by Alexander.
However, there was a conflict about it at #11457.
I'd say for now, let's keep it as a tuple and if a decision is eventually made as to how to
represent nanosecond timestamps, we can change them all then.

> It is possible to pass a negative timeout
It now raises an exception like select.

> signal_sigwaitinfo() and signal_sigtimedwait() use iterable_to_sigset()
Fixed.

> According to the manual page, sigwaitinfo() or sigtimedwait() can be interrupted (EINTR)
Actually, PyErr_SetFromErrno() does this implicitly. I added a test case anyway.

> Your patch doesn't touch configure nor pyconfig.h.in, only configure.in.
I prefer to keep the generated changes out of the patch. When I commit, I'll run autoreconf.

> siginfo_t structure contains more fields, but I don't know if we need all of them. It can be improved later.
POSIX 2008 specifies:
int           si_signo  Signal number. 
int           si_code   Signal code. 
int           si_errno  If non-zero, an errno value associated with 
                        this signal, as described in <errno.h>. 
pid_t         si_pid    Sending process ID. 
uid_t         si_uid    Real user ID of sending process. 
void         *si_addr   Address of faulting instruction. 
int           si_status Exit value or signal. 
long          si_band   Band event for SIGPOLL. 
union sigval  si_value  Signal value.

So I've left out si_addr (I don't think it's needed), si_band (we don't have SIGPOLL)
and si_value (not sure what it's for or how to represent a union :-)).

> sigtimedwait() raises a OSError(EGAIN) on timeout.
It now returns None.

> test_sigtimedwait_timeout(): why do you call signal.alarm()?
Copy/paste error.

> You may also add a test for timeout=0, I suppose that it is a special timeout value.
Added.

> I will do a deeper review on the second version of your patch :-)
How much more in depth can it get ;-) ?

>> It is possible to pass a negative timeout
> It now raises an exception like select.

Great.

>> According to the manual page, sigwaitinfo() or sigtimedwait() 
>> can be interrupted (EINTR)
> Actually, PyErr_SetFromErrno() does this implicitly. I added a test case anyway.

I would prefer an explicit call to PyErr_CheckSignals(), but if there is a test, it's just fine.

> So I've left out si_addr (I don't think it's needed)
> ... and si_value (not sure what it's for or how to represent a union :-))

I don't think that it means something in Python to have an address or this low-level "value" field. If someone needs them, we would need an use case with an example (to test it!). (So ok to not expose them)

> ... si_band (we don't have SIGPOLL)

What do you mean? signal.SIGPOLL exists in Python 3.3.

>> sigtimedwait() raises a OSError(EGAIN) on timeout.
> It now returns None.

Great.

--

>> I will do a deeper review on the second version of your patch :-)
> How much more in depth can it get ;-) ?

Here you have, issue12303_v2.patch:

 - "PyStructSequence_SET_ITEM(result, 4, PyLong_FromPid(si->si_uid));" looks wrong (we don't need to call PyLong_FromLongLong): si_uid type is uid_t, not pid_t. posix_getuid() simply uses PyLong_FromLong((long)getuid()).

 - sigwaitinfo() doc doesn't mention that the function can be interrupted if an unexpected signal is received. I don't know if it should be mentionned. It is mentionned in the manpage, in the ERRORS section (EINTR).

 - tests: test_sigtimedwait_negative_timeout() doesn't need to use _wait_helper() (which creates a subprocess!). You may also test (-1, 0) and (0, -1) timeouts.

 - test_sigwaitinfo(), test_sigtimedwait_poll(), test_sigwaitinfo_interrupted() are called from a child process. In test_wait(), I chose to write manually to stdout and call os._exit(1) (oh, I forgot an explicit sys.stdout.flush()). I don't know if you can use TestCase methods in a child process (I don't know what is written to stdout, _wait_helper() calls os._exit).

 - you may want to prepare the "What's new in Python 3.3" document (mention the 2 new functions)

 - style: _wait_helper(): you don't have to mark the helper as private (-> wait_helper())

 - style: _fill_siginfo() is already a static function you don't need a "_" prefix

> What do you mean? signal.SIGPOLL exists in Python 3.3.

Right, si_band added.

> - test_sigwaitinfo(), test_sigtimedwait_poll(), test_sigwaitinfo_interrupted() are called from a child process. In test_wait(), I chose to write manually to stdout and call os._exit(1) (oh, I forgot an explicit sys.stdout.flush()). I don't know if you can use TestCase methods in a child process (I don't know what is written to stdout, _wait_helper() calls os._exit).

I assume you mean stderr? Well anyway if the assert* fails, an exception is raised and then caught by this code in wait_helper():
"""
except BaseException as err:
                print("error: {}".format(err), file=sys.stderr)
                sys.stderr.flush()
                os._exit(1)
"""
which prints it to stderr.
If this is OK, I'll commit the patch.

> If this is OK, I'll commit the patch.

Yep, this version is the good one. Please keep the issue open until the test pass on all buildbots. Each newly added signal functions took me some days to fix the test for Mac OS X and/or FreeBSD 6. But you are maybe more lucky than me ;-)

New changeset 1137f7021b95 by Ross Lagerwall in branch 'default':
Issue #12303: Add sigwaitinfo() and sigtimedwait() to the signal module.
http://hg.python.org/cpython/rev/1137f7021b95

New changeset 768234f5c246 by Ross Lagerwall in branch 'default':
Fix test_signal on Windows after #12303.
http://hg.python.org/cpython/rev/768234f5c246

From the FreeBSD 6.4 buildbot:
"""
[172/356/2] test_signal
error: 'NoneType' object has no attribute 'si_signo'
error: False is not true
test test_signal failed -- multiple errors occurred; run in verbose mode for details
Re-running test test_signal in verbose mode
test_getsignal (test.test_signal.PosixTests) ... ok
test_out_of_range_signal_number_raises_error (test.test_signal.PosixTests) ... ok
test_setting_signal_handler_to_none_raises_error (test.test_signal.PosixTests) ... ok
test_main (test.test_signal.InterProcessSignalTests) ... skipped 'inter process signals not reliable (do not mix well with threading) on freebsd6'
test_pending (test.test_signal.WakeupSignalTests) ... ok
test_pthread_kill_main_thread (test.test_signal.WakeupSignalTests) ... ok
test_signum (test.test_signal.WakeupSignalTests) ... ok
test_wakeup_fd_during (test.test_signal.WakeupSignalTests) ... ok
test_wakeup_fd_early (test.test_signal.WakeupSignalTests) ... ok
test_siginterrupt_off (test.test_signal.SiginterruptTest) ... ok
test_siginterrupt_on (test.test_signal.SiginterruptTest) ... ok
test_without_siginterrupt (test.test_signal.SiginterruptTest) ... ok
test_itimer_exc (test.test_signal.ItimerTest) ... ok
test_itimer_prof (test.test_signal.ItimerTest) ... skipped 'itimer not reliable (does not mix well with threading) on freebsd6'
test_itimer_real (test.test_signal.ItimerTest) ... ok
test_itimer_virtual (test.test_signal.ItimerTest) ... skipped 'itimer not reliable (does not mix well with threading) on some BSDs.'
test_issue9324 (test.test_signal.WindowsSignalTests) ... skipped 'Windows specific'
test_pthread_kill (test.test_signal.PendingSignalsTests) ... ok
test_pthread_sigmask (test.test_signal.PendingSignalsTests) ... ok
test_pthread_sigmask_arguments (test.test_signal.PendingSignalsTests) ... ok
test_sigpending (test.test_signal.PendingSignalsTests) ... ok
test_sigpending_empty (test.test_signal.PendingSignalsTests) ... ok
test_sigtimedwait (test.test_signal.PendingSignalsTests) ... ok
test_sigtimedwait_negative_timeout (test.test_signal.PendingSignalsTests) ... ok
test_sigtimedwait_poll (test.test_signal.PendingSignalsTests) ... error: 'NoneType' object has no attribute 'si_signo'
FAIL
test_sigtimedwait_timeout (test.test_signal.PendingSignalsTests) ... ok
test_sigwait (test.test_signal.PendingSignalsTests) ... ok
test_sigwait_thread (test.test_signal.PendingSignalsTests) ... ok
test_sigwaitinfo (test.test_signal.PendingSignalsTests) ... ok
test_sigwaitinfo_interrupted (test.test_signal.PendingSignalsTests) ... error: False is not true
FAIL

======================================================================
FAIL: test_sigtimedwait_poll (test.test_signal.PendingSignalsTests)
----------------------------------------------------------------------
Traceback (most recent call last):
  File "/usr/home/db3l/buildarea/3.x.bolen-freebsd/build/Lib/test/test_signal.py", line 685, in test_sigtimedwait_poll
    self.wait_helper(test, self.handler, signal.SIGALRM)
  File "/usr/home/db3l/buildarea/3.x.bolen-freebsd/build/Lib/test/test_signal.py", line 645, in wait_helper
    self.assertEqual(os.waitpid(pid, 0), (pid, 0))
AssertionError: Tuples differ: (55455, 256) != (55455, 0)

First differing element 1:
256
0

- (55455, 256)
?         ^^^

+ (55455, 0)
?         ^


======================================================================
FAIL: test_sigwaitinfo_interrupted (test.test_signal.PendingSignalsTests)
----------------------------------------------------------------------
Traceback (most recent call last):
  File "/usr/home/db3l/buildarea/3.x.bolen-freebsd/build/Lib/test/test_signal.py", line 721, in test_sigwaitinfo_interrupted
    self.wait_helper(test, self.alarm_handler, signal.SIGUSR1)
  File "/usr/home/db3l/buildarea/3.x.bolen-freebsd/build/Lib/test/test_signal.py", line 645, in wait_helper
    self.assertEqual(os.waitpid(pid, 0), (pid, 0))
AssertionError: Tuples differ: (55460, 256) != (55460, 0)

First differing element 1:
256
0

- (55460, 256)
?         ^^^

+ (55460, 0)
?         ^
"""




@Victor, you've had some experience with fixing signals on the FreeBSD 6 buildbot.
Have you got any ideas as to what the cause of this may be?

> @Victor, you've had some experience with fixing signals
> on the FreeBSD 6 buildbot...

It's not exactly that I had some experience, it's just that I have a SSH access to the buildbot.

The following code hangs for (exactly?) 30 seconds on sigwaitinfo():
----
import os, signal, threading
s = signal.SIGALRM
signal.pthread_sigmask(signal.SIG_BLOCK, [s])
os.kill(os.getpid(), s)
signal.sigwaitinfo([s])
signal.pthread_sigmask(signal.SIG_UNBLOCK, [s])
----
sigwait() and sigtimedwait() wait also 30 seconds.

The following code only hangs for 1 second using sigwait(), sigwaitinfo() or sigtimedwait():
----
import os, signal, threading
s = signal.SIGALRM
signal.pthread_sigmask(signal.SIG_BLOCK, [s])
os.kill(os.getpid(), s)
signal.sigwaitinfo([s])
signal.pthread_sigmask(signal.SIG_UNBLOCK, [s])
----

test_sigtimedwait_poll() should be skipped on FreeBSD 6, there is a bug in the OS.

test_sigwaitinfo_interrupted() fails because SIGALRM signal handler is called, and the default FreeBSD handler stops the process. You should install a dummy signal handler (e.g. lambda signum, frame: None) for SIGALRM. I don't understand why the test doesn't fail on Linux, the default handler of SIGALRM on Linux stops also the process.

> test_sigwaitinfo_interrupted() fails because SIGALRM
> signal handler is called ...

Oh, the problem is that sigwait() behaviour changes after a fork: it is interrupted if an unexpected signal is received, but the signal handler is not called. It behaves correctly (the signal handler is called) without the fork.

The correct fix is maybe to avoid the os.fork() instead of skipping the test on FreeBSD 6.

> Oh, the problem is that sigwait() behaviour changes after a fork: it is interrupted if an unexpected signal is received, but the signal handler is not called. It behaves correctly (the signal handler is called) without the fork.
>

Reminds me of http://bugs.python.org/issue8407#msg138066

But I think we could just remove this test: honestly, I'm not sure
that checking the behavior in case of delivery of an unblocked signal
is really useful, especially since the behavior is not clearly
defined, see POSIX man page:
"""
The sigwaitinfo() and sigtimedwait() functions may fail if:

[EINTR]
The wait was interrupted by an unblocked, caught signal. It will be
documented in system documentation whether this error will cause these
functions to fail.
"""

"may fail" [...] "It will be documented in system documentation"

New changeset 00ca0c2c7bc0 by Victor Stinner in branch 'default':
Issue #12303: run sig*wait*() tests in a subprocesss
http://hg.python.org/cpython/rev/00ca0c2c7bc0

> But I think we could just remove this test

The test pass on Linux and FreeBSD 6 using a subprocess. I commited my patch to replace fork() by subprocess, let's see how it works on buildbots.

The tests seem to be working fine on all the buildbots now...

Thanks!