| msg171745 - (view) |
Author: Felipe Cruz (felipecruz) * |
Date: 2012-10-01 21:05 |
| It's possible to set a read only FD to signal.set_wakeup_fd(fd) Since write call[1] inside 'trip_signal' return code is ignored, no error will be raised. An untested solution is to call fcntl in this FD to check presence of write flags. 1 - http://hg.python.org/cpython/file/fb90e2ff95b7/Modules/signalmodule.c#l187 |
|
|
| msg171760 - (view) |
Author: STINNER Victor (vstinner) *  |
Date: 2012-10-01 23:26 |
| Is it really a bug? A file descriptor is just an integer, it may be replaced later. Passed fd may be writable when set_wakeup_fd() is called, but then become read-only. |
|
|
| msg171770 - (view) |
Author: Felipe Cruz (felipecruz) * |
Date: 2012-10-02 02:05 |
| I would not say that is a bug, but there is a write(wakeup_fd) call with ignored return code and maybe this can be improved to an output to stderr, or maybe a better solution. |
|
|
| msg171776 - (view) |
Author: Charles-François Natali (neologix) * |
Date: 2012-10-02 07:55 |
| > I would not say that is a bug, but there is a write(wakeup_fd) call > with ignored return code and maybe this can be improved to an output > to stderr, or maybe a better solution. The problem is that it's called from the signal handler, so there's not much we can do here (and certainly not log a warning or raise an exception). There's a contract to respect here, and if the user doesn't follow it, trouble will follow: for example, if the passed FD isn't non-blocking, then the signal handler can deadlock if the write() blocks (pipe, socket, whatever). |
|
|
| msg171841 - (view) |
Author: Antoine Pitrou (pitrou) * |
Date: 2012-10-03 00:05 |
| > The problem is that it's called from the signal handler, so there's not > much we can do here (and certainly not log a warning or raise an > exception). However, I think the errno could be passed via the "void *" argument to Py_AddPendingCall. Then checksignals_witharg can raise an error if the received errno is non-zero. I agree with Felipe that issues here can be difficult to diagnose. For example the fd could get mistakingly closed, but the write() EBADF would then be ignored and the expected signal wakeups would be lost. |
|
|
| msg171847 - (view) |
Author: Felipe Cruz (felipecruz) * |
Date: 2012-10-03 01:55 |
| Hello, since Antonie mentioned Py_AddPendingCall I came up with a patch describing what he proposed. Let me know if this patch can be improved or discarded(if the problem requires a more sophisticated solution). In case of improvement I can also submit another patch with a test case. |
|
|
| msg171853 - (view) |
Author: Charles-François Natali (neologix) * |
Date: 2012-10-03 05:43 |
| > since Antonie mentioned Py_AddPendingCall I came up with a patch describing what he proposed. > > Let me know if this patch can be improved or discarded(if the problem requires a more sophisticated solution). In case of improvement I can also submit another patch with a test case. Why limit to EBADF? You could also have EPIPE, EINVAL and many other errors. The only error you may not want to report is EAGAIN. |
|
|
| msg171854 - (view) |
Author: Charles-François Natali (neologix) * |
Date: 2012-10-03 05:51 |
| > I agree with Felipe that issues here can be difficult to diagnose. For example the fd could get mistakingly closed, but the write() EBADF would then be ignored and the expected signal wakeups would be lost. Yeah, but it's also completely possible that the saved fd now points to another file descriptor, and you end up corrupting a file, without getting any error at all, and not getting signal wakeups. This API is really fragile and dangerous, and warrants care anyway... |
|
|
| msg171861 - (view) |
Author: STINNER Victor (vstinner) * |
Date: 2012-10-03 07:04 |
| A signal handler can be called anymore, anywhere. How do you handle such exception in an application? "handle": do something better than exit the apllication. |
|
|
| msg171865 - (view) |
Author: STINNER Victor (vstinner) * |
Date: 2012-10-03 07:55 |
| > A signal handler can be called anymore, anywhere. Oopos: anywhere/anytime. |
|
|
| msg171872 - (view) |
Author: Charles-François Natali (neologix) * |
Date: 2012-10-03 08:40 |
| > A signal handler can be called anymore, anywhere. How do you handle such > exception in an application? "handle": do something better than exit the > apllication. Well, chances are you won't, but failing with an explicit error message is better than silently failing to deliver signals (which may result in a deadlock, for example). |
|
|
| msg171888 - (view) |
Author: Felipe Cruz (felipecruz) * |
Date: 2012-10-03 14:23 |
| > Why limit to EBADF? You could also have EPIPE, EINVAL and many other errors. > The only error you may not want to report is EAGAIN. Charles, You're right! If all errno cases get covered in the patch, will It looks reasonable? |
|
|
| msg171901 - (view) |
Author: Charles-François Natali (neologix) * |
Date: 2012-10-03 18:19 |
| >> Why limit to EBADF? You could also have EPIPE, EINVAL and many other errors. >> The only error you may not want to report is EAGAIN. > > Charles, > You're right! If all errno cases get covered in the patch, will It looks reasonable? Raising an error in case the signal can't be written to the FD (because the other end didn't drain the pipe/socket) seems reasonable. You should just retry on EINTR (although any sane implementation shouldn't return EINTR on non-blocking write, I don't think it's strictly prohibited by POSIX). |
|
|
| msg171917 - (view) |
Author: Felipe Cruz (felipecruz) * |
Date: 2012-10-04 02:14 |
| > Raising an error in case the signal can't be written to the FD > (because the other end didn't drain the pipe/socket) seems reasonable. > You should just retry on EINTR (although any sane implementation > shouldn't return EINTR on non-blocking write, I don't think it's > strictly prohibited by POSIX). You mean retry one time or until success? |
|
|
| msg171997 - (view) |
Author: Charles-François Natali (neologix) * |
Date: 2012-10-04 21:34 |
| > You mean retry one time or until success? Until success. It should also come with a test. |
|
|
| msg172030 - (view) |
Author: Felipe Cruz (felipecruz) * |
Date: 2012-10-04 23:55 |
| This patch retries write() until success if errno == EINTR. There is also a test. |
|
|
| msg173091 - (view) |
Author: Felipe Cruz (felipecruz) * |
Date: 2012-10-16 20:22 |
| I've followed latest suggestions. Test and code updated. |
|
|
| msg195294 - (view) |
Author: Antoine Pitrou (pitrou) * |
Date: 2013-08-15 21:55 |
| Here is a cleaned up patch. However, I'm wondering if raising an error is a good idea. The effect will be to get cryptic OSErrors at random execution points. Perhaps using PyErr_WriteUnraisable would be better? |
|
|
| msg195304 - (view) |
Author: Felipe Cruz (felipecruz) * |
Date: 2013-08-16 00:04 |
| Looks like PyErr_WriteUnraisable can be a better choice. Exceptions at random execution points looks a little bit dirty at least for this case. |
|
|
| msg195402 - (view) |
Author: Antoine Pitrou (pitrou) * |
Date: 2013-08-16 19:27 |
| Updated patch using PyErr_WriteUnraisable(). |
|
|
| msg195422 - (view) |
Author: Felipe Cruz (felipecruz) * |
Date: 2013-08-16 21:18 |
| Looks good. |
|
|
| msg195502 - (view) |
Author: Roundup Robot (python-dev)  |
Date: 2013-08-17 18:33 |
| New changeset e2b234f5bf7d by Antoine Pitrou in branch 'default': Issue #16105: When a signal handler fails to write to the file descriptor registered with ``signal.set_wakeup_fd()``, report an exception instead of ignoring the error. http://hg.python.org/cpython/rev/e2b234f5bf7d |
|
|
| msg195508 - (view) |
Author: Antoine Pitrou (pitrou) * |
Date: 2013-08-17 19:16 |
| Ok, I pushed the patch to 3.4. Thanks for the report! |
|
|