Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Turn SIG_DFL and SIG_IGN into functions #67514

Closed
serhiy-storchaka opened this issue Jan 26, 2015 · 17 comments
Closed

Turn SIG_DFL and SIG_IGN into functions #67514

serhiy-storchaka opened this issue Jan 26, 2015 · 17 comments
Labels
3.9 only security fixes 3.10 only security fixes 3.11 only security fixes stdlib Python modules in the Lib dir type-feature A feature request or enhancement

Comments

@serhiy-storchaka
Copy link
Member

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']

    @serhiy-storchaka
    Copy link
    Member Author

    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 (bpo-21076).

    @serhiy-storchaka serhiy-storchaka added stdlib Python modules in the Lib dir type-feature A feature request or enhancement labels Jan 26, 2015
    @ethanfurman
    Copy link
    Member

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

    @serhiy-storchaka
    Copy link
    Member Author

    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.

    @ethanfurman
    Copy link
    Member

    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.

    @zhangyangyu
    Copy link
    Member

    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.

    @zhangyangyu zhangyangyu added the 3.8 only security fixes label Aug 25, 2018
    @serhiy-storchaka
    Copy link
    Member Author

    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.

    @zhangyangyu
    Copy link
    Member

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

    @serhiy-storchaka serhiy-storchaka added 3.10 only security fixes and removed 3.8 only security fixes labels Jun 28, 2020
    @tiran
    Copy link
    Member

    tiran commented Jan 20, 2022

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

    @tiran tiran added 3.9 only security fixes 3.11 only security fixes labels Jan 20, 2022
    @serhiy-storchaka
    Copy link
    Member Author

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

    @tiran
    Copy link
    Member

    tiran commented Jan 21, 2022

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

    @tiran
    Copy link
    Member

    tiran commented Mar 8, 2022

    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.

    @serhiy-storchaka
    Copy link
    Member Author

    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.

    @miss-islington
    Copy link
    Contributor

    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

    @miss-islington
    Copy link
    Contributor

    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

    @encukou
    Copy link
    Member

    encukou commented Mar 25, 2024

    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.

    @vstinner
    Copy link
    Member

    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.

    @serhiy-storchaka
    Copy link
    Member Author

    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.

    Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
    Labels
    3.9 only security fixes 3.10 only security fixes 3.11 only security fixes stdlib Python modules in the Lib dir type-feature A feature request or enhancement
    Projects
    None yet
    Development

    No branches or pull requests

    7 participants