Turn SIG_DFL and SIG_IGN into functions

@warsaw

BPO	23325
Nosy	@warsaw, @brettcannon, @vstinner, @giampaolo, @tiran, @serhiy-storchaka, @zhangyangyu, @miss-islington
PRs	bpo-23325: Turn signal.SIG_DFL and signal.SIG_IGN into functions. #8920 bpo-23325: Fix SIG_IGN and SIG_DFL int comparison in signal module #31759 [3.10] bpo-23325: Fix SIG_IGN and SIG_DFL int comparison in signal module (GH-31759) #31768
Files	signal_std_handlers.patch

^{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 = None
closed_at = None
created_at = <Date 2015-01-26.20:38:30.138>
labels = ['type-feature', 'library', '3.9', '3.10', '3.11']
title = 'Turn SIG_DFL and SIG_IGN into functions'
updated_at = <Date 2022-03-31.17:54:28.870>
user = 'https://github.com/serhiy-storchaka'

bugs.python.org fields:

activity = <Date 2022-03-31.17:54:28.870>
actor = 'brett.cannon'
assignee = 'none'
closed = False
closed_date = None
closer = None
components = ['Library (Lib)']
creation = <Date 2015-01-26.20:38:30.138>
creator = 'serhiy.storchaka'
dependencies = []
files = ['37868']
hgrepos = []
issue_num = 23325
keywords = ['patch']
message_count = 14.0
messages = ['234775', '238327', '238495', '238538', '324075', '324084', '324096', '411037', '411045', '411104', '414747', '414749', '414765', '414767']
nosy_count = 9.0
nosy_names = ['barry', 'brett.cannon', 'vstinner', 'giampaolo.rodola', 'christian.heimes', 'eli.bendersky', 'serhiy.storchaka', 'xiang.zhang', 'miss-islington']
pr_nums = ['8920', '31759', '31768']
priority = 'normal'
resolution = None
stage = 'patch review'
status = 'open'
superseder = None
type = 'enhancement'
url = 'https://bugs.python.org/issue23325'
versions = ['Python 3.9', 'Python 3.10', 'Python 3.11']

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:

They will have names and self-descriptive representation.
They will have docstrings.
The signature of signal() will be simpler. The type of second argument would be function.
Their pickling will be portable.

This patch depends on the backout of turning these constants to enums (bpo-21076).

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)'?

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.

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.

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.

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.

I miss that. So this change seems to be user transparent and make more sense then.

Serhiy, could you please rebase your PR to tip of main branch? I'd like to try it out.

It may take a time, because the module initialization code has been completely rewritten.

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)) {

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.

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.

New changeset c8a47e7 by Christian Heimes in branch 'main':
bpo-23325: Fix SIG_IGN and SIG_DFL int comparison in signal module (GH-31759)
c8a47e7

New changeset 95b001f by Miss Islington (bot) in branch '3.10':
bpo-23325: Fix SIG_IGN and SIG_DFL int comparison in signal module (GH-31759)
95b001f

Do we still want to do this?
While accepting 0 and 1 is undocumented, it's been that way for very long. IMO, it needs a deprecation period if it's changed.

That said, I don't think they should be changed. Counterpoints to the rationale:

Enums already have names and self-descriptive representation.
Docstrings don't seem very important to me
These are special. IMO, the fact that the signature of signal() includes these is a good thing, documentation-wise.
I'm not sure what you meant by pickle portability. Compatibility with what? In the proposed PR, it's pickled as _signal.SIG_DFL, depending on the CPython-specific C module.

I recommend closing.

@serhiy-storchaka

I agree to close the issue. I close it.

If tomorrow someone comes with a stronger use case, I can change my mind :-)

Anyway, @serhiy-storchaka, it was interesting to explore this idea. Maybe if it was done 10 or 15 years ago, it would make more sense. Today, we have to learn with these special cases.

If SIG_DFL and SIG_IGN become functions, people will want to call them, and I expect that it will complicated to implement SIG_DFL: you should call signal.raise_signal(signum)... but with which signal number? If we store the signal number, the same SIG_DFL object cannot be used for two signals. Or they should be fake functions which always fail, for example raise NotImplementedError(). But users may expect a different behavior.

I would be ok with replacing SIG_DFL and SIG_IGN with special non-callable objects, different than int. But I'm not sure the benefits are worth it compared to the risk of breaking existing code. Currently, it's possible to convert them to int. Should it still be the case if they are changed? If yes, what's the point?

They will have names and self-descriptive representation.
They will have docstrings.
The signature of signal() will be simpler. The type of second argument would be function.
Their pickling will be portable.

I'm not convinced that these problems are important enough compared to drawbacks.

I don't think that it's common to serialize/deserialize signal handlers! If your program does that, add special a case in your problem, no?

At the C level, these values are special function pointers:

/usr/include/bits/signum-generic.h:#define	SIG_ERR	 ((__sighandler_t) -1)	/* Error return.  */
/usr/include/bits/signum-generic.h:#define	SIG_DFL	 ((__sighandler_t)  0)	/* Default action.  */
/usr/include/bits/signum-generic.h:#define	SIG_IGN	 ((__sighandler_t)  1)	/* Ignore signal.  */

In Python 3.13, there are now enums:

$ ./python
Python 3.13.0a5+ (heads/pr/116482:582a42056e, Mar 22 2024, 15:44:14) [GCC 13.2.1 20240316 (Red Hat 13.2.1-7)] on linux
>>> import signal
>>> signal.SIG_DFL
<Handlers.SIG_DFL: 0>
>>> signal.SIG_IGN
<Handlers.SIG_IGN: 1>

pickle serializes them as signal.SIG_IGN as signal.Handlers(*(1,)):

import pickle, pickletools, signal
pickletools.dis(pickle.dumps(signal.SIG_IGN))

Output (commented):

    0: \x80 PROTO      4
    2: \x95 FRAME      29
   11: \x8c SHORT_BINUNICODE 'signal'
   19: \x94 MEMOIZE    (as 0)
   20: \x8c SHORT_BINUNICODE 'Handlers'    <== signal.Handlers
   30: \x94 MEMOIZE    (as 1)
   31: \x93 STACK_GLOBAL
   32: \x94 MEMOIZE    (as 2)
   33: K    BININT1    1   <== value 1
   35: \x85 TUPLE1
   36: \x94 MEMOIZE    (as 3)
   37: R    REDUCE
   38: \x94 MEMOIZE    (as 4)
   39: .    STOP
highest protocol among opcodes = 4

Their pickling will be portable.

As @ethanfurman wrote, Handlers can be modified for that.

I should have commit a patch 9 years ago without waiting for a review. Since then, the code has been significantly modified several times, making it difficult to keep the patch up-to-date. I strongly suspect that the current code contains severe bugs in the case when subinterpreters are involved.

Navigation Menu

Repository navigation

Turn SIG_DFL and SIG_IGN into functions #67514

Metadata

Assignees

Labels

Projects

Milestone

Relationships

Development

Turn SIG_DFL and SIG_IGN into functions #67514

Description

Activity

serhiy-storchaka commented on Jan 26, 2015

ethanfurman commented on Mar 17, 2015

serhiy-storchaka commented on Mar 19, 2015

ethanfurman commented on Mar 19, 2015

zhangyangyu commented on Aug 25, 2018

serhiy-storchaka commented on Aug 25, 2018

zhangyangyu commented on Aug 25, 2018

tiran commented on Jan 20, 2022

serhiy-storchaka commented on Jan 20, 2022

tiran commented on Jan 21, 2022

tiran commented on Mar 8, 2022

serhiy-storchaka commented on Mar 8, 2022

miss-islington commented on Mar 8, 2022

miss-islington commented on Mar 8, 2022

encukou commented on Mar 25, 2024

vstinner commented on Mar 25, 2024

serhiy-storchaka commented on Mar 26, 2024

Metadata

Metadata

Assignees

Labels

Projects

Milestone

Relationships

Development

Issue actions