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 signal.SIG* constants into enums #65275

Closed
giampaolo opened this issue Mar 27, 2014 · 22 comments
Closed

Turn signal.SIG* constants into enums #65275

giampaolo opened this issue Mar 27, 2014 · 22 comments
Assignees
Labels
stdlib Python modules in the Lib dir type-bug An unexpected behavior, bug, or error

Comments

@giampaolo
Copy link
Contributor

BPO 21076
Nosy @gvanrossum, @vstinner, @giampaolo, @serhiy-storchaka
Files
  • signals.patch
  • signals2.patch
  • signals3.patch
  • signals4.patch
  • signal_no_enum_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 = 'https://github.com/giampaolo'
    closed_at = <Date 2020-06-25.09:30:58.351>
    created_at = <Date 2014-03-27.15:48:50.909>
    labels = ['type-bug', 'library']
    title = 'Turn signal.SIG* constants into enums'
    updated_at = <Date 2020-06-25.09:30:58.350>
    user = 'https://github.com/giampaolo'

    bugs.python.org fields:

    activity = <Date 2020-06-25.09:30:58.350>
    actor = 'vstinner'
    assignee = 'giampaolo.rodola'
    closed = True
    closed_date = <Date 2020-06-25.09:30:58.351>
    closer = 'vstinner'
    components = ['Library (Lib)']
    creation = <Date 2014-03-27.15:48:50.909>
    creator = 'giampaolo.rodola'
    dependencies = []
    files = ['34636', '34643', '34653', '34655', '37865']
    hgrepos = []
    issue_num = 21076
    keywords = ['patch']
    message_count = 22.0
    messages = ['214959', '214960', '214985', '214993', '215003', '215042', '215059', '215072', '215332', '215519', '215521', '215526', '215528', '234608', '234610', '234768', '234770', '234776', '238317', '238319', '239556', '372322']
    nosy_count = 6.0
    nosy_names = ['gvanrossum', 'vstinner', 'giampaolo.rodola', 'neologix', 'python-dev', 'serhiy.storchaka']
    pr_nums = []
    priority = 'normal'
    resolution = 'fixed'
    stage = 'resolved'
    status = 'closed'
    superseder = None
    type = 'behavior'
    url = 'https://bugs.python.org/issue21076'
    versions = ['Python 3.5']

    @giampaolo
    Copy link
    Contributor Author

    Relevant discussion + BDFL approval:
    https://mail.python.org/pipermail/python-ideas/2014-March/027286.html
    Patch (not tested on Windows) is in attachment.

    @giampaolo giampaolo added the stdlib Python modules in the Lib dir label Mar 27, 2014
    @gvanrossum
    Copy link
    Member

    OK, somebody please review this (not me).

    @neologix
    Copy link
    Mannequin

    neologix mannequin commented Mar 27, 2014

    This patch can't be reviewed: please re-generate without --git.

    @giampaolo
    Copy link
    Contributor Author

    This time I made it without --git but that didn't help either.
    Not sure what to do. :-\

    Note: the devguide recommends using --git BTW: http://docs.python.org/devguide/committing.html
    Should that be changed?

    @giampaolo
    Copy link
    Contributor Author

    OK, it appears it works now. Sorry for the notification noise.

    @giampaolo
    Copy link
    Contributor Author

    (replying here 'cause rietveld keeps giving me an exception when I submit a reply)

    On 2014/03/28 00:49:47, haypo wrote:

    http://bugs.python.org/review/21076/diff/11457/Lib/signal.py
    File Lib/signal.py (right):

    http://bugs.python.org/review/21076/diff/11457/Lib/signal.py#newcode7
    Lib/signal.py:7: if name.isupper()
    You can probably remove this test.

    http://bugs.python.org/review/21076/diff/11457/Lib/signal.py#newcode8
    Lib/signal.py:8: and (name.startswith('SIG') and not name.startswith('SIG_'))
    Why do you ignore SIG_DFL, SIG_IGN, SIG_BLOCK, SIG_UNBLOCK, SIG_SETMASK? It may
    also be interesting to provide enums for them. They are just integers (0, 1 or
    2) on my Linux.

    I guess it makes sense.
    I'm now realizing that the patch as-is is incomplete as the other "get" APIs (signal.getsignal() and others) still return integers: they should be overridden in order to convert integers into enums, similarly to http://hg.python.org/cpython/file/d8659dbebfd1/Lib/socket.py#l262
    I will do that.

    @giampaolo
    Copy link
    Contributor Author

    New patch in attachment provides 3 enum containers and overrides all the necessary signal module APIs so that they interoperate with enums on both "get" and "set".
    I decided *not* to rename signamodule.c to _signamodule.c in this patch so that it is easier to review (I will do the renaming as a second step).

    @giampaolo
    Copy link
    Contributor Author

    Updated patch following Victor's suggestions is in attachment.

    @giampaolo
    Copy link
    Contributor Author

    If there are no other concerns I will commit latest patch tomorrow, then do the renaming.

    @python-dev
    Copy link
    Mannequin

    python-dev mannequin commented Apr 4, 2014

    New changeset c9239171e429 by Giampaolo Rodola' in branch 'default':
    fix bpo-21076: turn signal module constants into enums
    http://hg.python.org/cpython/rev/c9239171e429

    @serhiy-storchaka
    Copy link
    Member

    http://buildbot.python.org/all/builders/AMD64%20Ubuntu%20LTS%203.x/builds/4131/steps/compile/logs/stdio

    gcc -pthread -Xlinker -export-dynamic -o Modules/_testembed Modules/_testembed.o libpython3.5dm.a -lpthread -ldl -lutil -lm
    libpython3.5dm.a(config.o):(.data+0x18): undefined reference to `PyInit_signal'
    collect2: ld returned 1 exit status
    make: *** [Modules/_testembed] Error 1

    @python-dev
    Copy link
    Mannequin

    python-dev mannequin commented Apr 4, 2014

    New changeset df5120efb86e by Victor Stinner in branch 'default':
    Issue bpo-21076: the C signal module has been renamed to _signal
    http://hg.python.org/cpython/rev/df5120efb86e

    @python-dev
    Copy link
    Mannequin

    python-dev mannequin commented Apr 4, 2014

    New changeset b1f5b5d7997f by Victor Stinner in branch 'default':
    Issue bpo-21076: sigpending() is not available on Windows
    http://hg.python.org/cpython/rev/b1f5b5d7997f

    @giampaolo giampaolo self-assigned this Apr 4, 2014
    @serhiy-storchaka
    Copy link
    Member

    Now signal.signal() accepts inappropriate types.

    >>> signal.signal(signal.SIGHUP, 0.0)
    <Handlers.SIG_DFL: 0>
    >>> signal.signal(signal.SIGHUP, '0')
    <Handlers.SIG_DFL: 0>

    In 3.4 it raised an exception.

    @serhiy-storchaka
    Copy link
    Member

    And more:

    >>> import signal
    >>> signal.signal(1.2, signal.SIG_DFL)
    <Handlers.SIG_DFL: 0>
    >>> signal.signal('1', signal.SIG_DFL)
    <Handlers.SIG_DFL: 0>

    @serhiy-storchaka
    Copy link
    Member

    And more, as far as standard signal handler is tested for identity, signal.SIG_DFL and _signal.SIG_DFL should be the same object. Current code works only due to coincidence of two circumstances:

    1. Small integers are cached in CPython.
    2. SIG_DFL and SIG_IGN are small integers on common platforms.

    When small integer caching is disabled (NSMALLPOSINTS == NSMALLPOSINTS == 0) or when platform depended macros SIG_DFL and SIG_IGN are not small integers, the signal module will mystically fail.

    The simplest way to solve this issue is to backout turning SIG_DFL and SIG_IGN into enums.

    @serhiy-storchaka serhiy-storchaka added the type-bug An unexpected behavior, bug, or error label Jan 26, 2015
    @ethanfurman
    Copy link
    Member

    I know nothing about this part of CPython, but wouldn't the correct solution be to not compare by identity?

    @serhiy-storchaka
    Copy link
    Member

    I have other proposition -- turn them into functions (bpo-23325).

    @ethanfurman
    Copy link
    Member

    Working on bpo-23673 I saw this in the new signal.py:

    +def _enum_to_int(value):
    + """Convert an IntEnum member to a numeric value.
    + If it's not a IntEnum member return the value itself.
    + """
    + try:
    + return int(value)
    + except (ValueError, TypeError):
    + return value

    The SIG, etc, constants are based on IntEnum, so they are already numeric values, and this function is unnecessary.

    @ethanfurman
    Copy link
    Member

    Removing the 'enum_to_int' function would also take care of the accepting inappropriate types problem.

    @ethanfurman
    Copy link
    Member

    Okay, in a perfect world that _enum_to_int function would be unnecessary, but as Serhiy pointed out the C code is doing pointer comparison, so unless the exact same int is passed in it does not work.

    I'm looking at adjusting the C code.

    @vstinner
    Copy link
    Member

    signal constants are now enum, I close the issue. For further enhancements, please open a separated issue.

    $ python3
    Python 3.8.3 (default, May 15 2020, 00:00:00) 
    >>> import signal; signal.SIGTERM
    <Signals.SIGTERM: 15>

    @ezio-melotti ezio-melotti transferred this issue from another repository Apr 10, 2022
    Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
    Labels
    stdlib Python modules in the Lib dir type-bug An unexpected behavior, bug, or error
    Projects
    None yet
    Development

    No branches or pull requests

    5 participants