STINNER Victor <victor.stin...@haypocalc.com> added the comment: 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). ---------- _______________________________________ Python tracker <rep...@bugs.python.org> <http://bugs.python.org/issue12303> _______________________________________ _______________________________________________ Python-bugs-list mailing list Unsubscribe: http://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com
