| msg234775 - (view) |
Author: Serhiy Storchaka (serhiy.storchaka) *  |
Date: 2015-01-26 20:38 |
| In C the SIG_DFL and SIG_IGN macros expand into integral expressions that are not equal to an address of any function. In Python they are int objects with platform depended values. Second argument of the signal() function should be SIG_DFL, SIG_IGN, or a function. The getsignal() function returns SIG_DFL, SIG_IGN, None, or a function. They are tested for identity in signal() and getsignal() returns identical values. So actually SIG_DFL and SIG_IGN are just singletons. I propose to turn SIG_DFL and SIG_IGN into functions. Benefits: 1. They will have names and self-descriptive representation. 2. They will have docstrings. 3. The signature of signal() will be simpler. The type of second argument would be function. 4. Their pickling will be portable. This patch depends on the backout of turning these constants to enums (). |
|
|
| msg238327 - (view) |
Author: Ethan Furman (ethan.furman) * |
Date: 2015-03-17 18:13 |
| A private method is being added to Enum to better support Enum replacement of constants, part of which includes changing __reduce_ex__ to return the string of the name. These changes answer points 1 and 4. Point 2 would be nice, but seems somewhat less important if Enums are being used. Point 3 -- I don't see how 'Signal(xxx, yyy)' is more complicated than 'Signal(xxx, zzz)'? |
|
|
| msg238495 - (view) |
Author: Serhiy Storchaka (serhiy.storchaka) * |
Date: 2015-03-19 07:17 |
| Currently the action parameter of signal.signal() can be SIG_DFL, SIG_IGN, or a callable Python object. Would be just a callable Python object. This is simpler. The main benefit is that they will be better look in pydoc output. |
|
|
| msg238538 - (view) |
Author: Ethan Furman (ethan.furman) * |
Date: 2015-03-19 15:51 |
| Thanks for the explanation, point taken. However, it seems to me that changing the type of the constants from 'int' to 'function' is backwards incompatible, will break code, and is probably not worth it (and would require a deprecation period if it was worth it). I still think the better answer is to delve into Modules/signalmodule.c and fix the comparisons. |
|
|
| msg324075 - (view) |
Author: Xiang Zhang (xiang.zhang) * |
Date: 2018-08-25 16:26 |
| I'm afraid this could break old codes so not sure it's worth or not. I've seen code like `SIGTERM = 15; signal.signal(SIGTERM, handler)`. I won't be surprised to see a handcraft SIG_DFL/IGN. |
|
|
| msg324084 - (view) |
Author: Serhiy Storchaka (serhiy.storchaka) * |
Date: 2018-08-25 17:42 |
| Numerical values of signal numbers are standardized. signal.signal(15, handler) is valid. SIGTERM is just a handy name for the constant 15. But SIG_DFL and SIG_IGN are not integers in C (they are special pointers), and it is not declared anywhere that they should be integers in Python. The code that converts them to Python integer is implementation depending (PyLong_FromVoidPtr). Handcrafted SIG_DFL and SIG_IGN are broken by design, because the signal module uses identity comparison for SIG_DFL and SIG_IGN. |
|
|
| msg324096 - (view) |
Author: Xiang Zhang (xiang.zhang) * |
Date: 2018-08-25 19:36 |
| I miss that. So this change seems to be user transparent and make more sense then. |
|
|
| msg411037 - (view) |
Author: Christian Heimes (christian.heimes) * |
Date: 2022-01-20 17:00 |
| Serhiy, could you please rebase your PR to tip of main branch? I'd like to try it out. |
|
|
| msg411045 - (view) |
Author: Serhiy Storchaka (serhiy.storchaka) * |
Date: 2022-01-20 20:43 |
| It may take a time, because the module initialization code has been completely rewritten. |
|
|
| msg411104 - (view) |
Author: Christian Heimes (christian.heimes) * |
Date: 2022-01-21 09:54 |
| Understood, thanks! A trivial fix for the integer comparison bug would solve my issue: --- a/Modules/signalmodule.c +++ b/Modules/signalmodule.c @@ -527,21 +527,20 @@ signal_signal_impl(PyObject *module, int signalnum, PyObject *handler) "signal number out of range"); return NULL; } - if (handler == modstate->ignore_handler) { + if (PyCallable_Check(handler)) { + func = signal_handler; + } + else if (PyObject_RichCompareBool(handler, modstate->ignore_handler, Py_EQ) == 1) { func = SIG_IGN; } - else if (handler == modstate->default_handler) { + else if (PyObject_RichCompareBool(handler, modstate->default_handler, Py_EQ) == 1) { func = SIG_DFL; - } - else if (!PyCallable_Check(handler)) { + } else { _PyErr_SetString(tstate, PyExc_TypeError, "signal handler must be signal.SIG_IGN, " "signal.SIG_DFL, or a callable object"); return NULL; } - else { - func = signal_handler; - } /* Check for pending signals before changing signal handler */ if (_PyErr_CheckSignalsTstate(tstate)) { |
|
|
| msg414747 - (view) |
Author: Christian Heimes (christian.heimes) * |
Date: 2022-03-08 10:39 |
| My PR 31759 removes the assumption of small int singletons and replaces C comparison with PyObject_RichCompareBool() Py_EQ calls. I still prefer Serhiy's solution, but it may cause backwards incompatible breakage. My fix can be backported to 3.9 and 3.10 without breakage. It resolves my failing tests on Emscripten, too. |
|
|
| msg414749 - (view) |
Author: Serhiy Storchaka (serhiy.storchaka) * |
Date: 2022-03-08 12:29 |
| Agree. There were too many changes in this code, and making SIG_DFL and SIG_IGN functions exposes some issues with sharing objects between interpreters. It is easier to keep them integers for now. |
|
|
| msg414765 - (view) |
Author: miss-islington (miss-islington) |
Date: 2022-03-08 18:22 |
| New changeset c8a47e76a391c8818bf10a282cdcd3bb5c23ebf6 by Christian Heimes in branch 'main': bpo-23325: Fix SIG_IGN and SIG_DFL int comparison in signal module (GH-31759) https://github.com/python/cpython/commit/c8a47e76a391c8818bf10a282cdcd3bb5c23ebf6 |
|
|
| msg414767 - (view) |
Author: miss-islington (miss-islington) |
Date: 2022-03-08 18:53 |
| New changeset 95b001fe6766f491f4356f8bcf23d6895bab2342 by Miss Islington (bot) in branch '3.10': bpo-23325: Fix SIG_IGN and SIG_DFL int comparison in signal module (GH-31759) https://github.com/python/cpython/commit/95b001fe6766f491f4356f8bcf23d6895bab2342 |
|
|