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

Patch for signal.set_wakeup_fd #45924

Closed
Rhamphoryncus mannequin opened this issue Dec 10, 2007 · 23 comments
Closed

Patch for signal.set_wakeup_fd #45924

Rhamphoryncus mannequin opened this issue Dec 10, 2007 · 23 comments
Assignees
Labels
extension-modules C modules in the Modules dir

Comments

@Rhamphoryncus
Copy link
Mannequin

Rhamphoryncus mannequin commented Dec 10, 2007

BPO 1583
Nosy @birkenfeld, @vstinner, @jdemeyer
Files
  • python2.6-set_wakeup_fd1.diff
  • python2.6-set_wakeup_fd2.diff
  • python2.6-set_wakeup_fd3.diff
  • python2.6-set_wakeup_fd4.diff
  • 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/gvanrossum'
    closed_at = <Date 2007-12-19.19:41:39.688>
    created_at = <Date 2007-12-10.23:20:05.669>
    labels = ['extension-modules']
    title = 'Patch for signal.set_wakeup_fd'
    updated_at = <Date 2019-04-11.11:04:48.554>
    user = 'https://bugs.python.org/Rhamphoryncus'

    bugs.python.org fields:

    activity = <Date 2019-04-11.11:04:48.554>
    actor = 'vstinner'
    assignee = 'gvanrossum'
    closed = True
    closed_date = <Date 2007-12-19.19:41:39.688>
    closer = 'gvanrossum'
    components = ['Extension Modules']
    creation = <Date 2007-12-10.23:20:05.669>
    creator = 'Rhamphoryncus'
    dependencies = []
    files = ['8915', '8916', '8925', '8988']
    hgrepos = []
    issue_num = 1583
    keywords = ['patch']
    message_count = 23.0
    messages = ['58385', '58386', '58387', '58392', '58438', '58442', '58662', '58663', '58784', '58819', '339740', '339766', '339799', '339802', '339805', '339806', '339807', '339826', '339871', '339892', '339897', '339962', '339963']
    nosy_count = 5.0
    nosy_names = ['georg.brandl', 'Rhamphoryncus', 'gustavo', 'vstinner', 'jdemeyer']
    pr_nums = []
    priority = 'normal'
    resolution = 'accepted'
    stage = None
    status = 'closed'
    superseder = None
    type = None
    url = 'https://bugs.python.org/issue1583'
    versions = ['Python 2.6']

    @Rhamphoryncus
    Copy link
    Mannequin Author

    Rhamphoryncus mannequin commented Dec 10, 2007

    This adds signal.set_wakeup_fd(fd), which allows a single library to be
    woken when a signal comes in.

    @Rhamphoryncus
    Copy link
    Mannequin Author

    Rhamphoryncus mannequin commented Dec 10, 2007

    Guido, it looks like I can't alter the Assigned To field. You get the
    Nosy List instead. ;)

    @gvanrossum
    Copy link
    Member

    Thanks!

    Can you add a doc patch too? Doc/library/signal.rst

    @gvanrossum gvanrossum self-assigned this Dec 10, 2007
    @Rhamphoryncus
    Copy link
    Mannequin Author

    Rhamphoryncus mannequin commented Dec 10, 2007

    version 2, adds to Doc/library/signal.rst. It also tweaks the
    set_wakeup_fd's docstring.

    I haven't verified that my formatting in signal.rst is correct.
    Specifically, the '\0' should be checked.

    @birkenfeld
    Copy link
    Member

    '\0' will be correct.

    @Rhamphoryncus
    Copy link
    Mannequin Author

    Rhamphoryncus mannequin commented Dec 11, 2007

    Thanks georg.

    @gustavo
    Copy link
    Mannequin

    gustavo mannequin commented Dec 15, 2007

    The patch looks great.

    But I was wondering if there is any chance to export a C API in addition
    to the Python one? That is because PyGTK is mostly C, the code that
    needs this is C, it would be easier to call a C API.

    @Rhamphoryncus
    Copy link
    Mannequin Author

    Rhamphoryncus mannequin commented Dec 15, 2007

    The python API has the advantage that you can test for it at runtime,
    avoiding a compile-time check. I don't know if this is significant though.

    I don't see the big deal about a C API. All you need to do is call
    PyImport_ImportModule("signal") and PyObject_CallMethod(mod,
    "set_wakeup_fd", "i", fd);

    @gvanrossum
    Copy link
    Member

    Here's an updated patch that applies smoothly to the current trunk.

    @gvanrossum gvanrossum added the extension-modules C modules in the Modules dir label Dec 18, 2007
    @gvanrossum
    Copy link
    Member

    Committed revision 59574.

    I added a simple C API as well, PySignal_SetWakeupFd(fd).

    Thanks all!

    @jdemeyer
    Copy link
    Contributor

    jdemeyer commented Apr 9, 2019

    Why is this using type "sig_atomic_t" for a file descriptor instead of "int" (which is the type of file descriptors)? See #12670

    @Rhamphoryncus
    Copy link
    Mannequin Author

    Rhamphoryncus mannequin commented Apr 9, 2019

    The fd field may be written from the main thread simultaneous with the signal handler activating and reading it out. Back in 2007 the only POSIX-compliant type allowed for that was sig_atomic_t, anything else was undefined.

    Looks like pycore_atomic.h should have alternatives now but I'm not at all familiar with it.

    @jdemeyer
    Copy link
    Contributor

    jdemeyer commented Apr 9, 2019

    Back in 2007 the only POSIX-compliant type allowed for that was sig_atomic_t, anything else was undefined.

    Fair enough, but having a non-atomic type is still much better than a completely wrong type. In other words, the requirement of fitting a file descriptor is more important than being atomic.

    @Rhamphoryncus
    Copy link
    Mannequin Author

    Rhamphoryncus mannequin commented Apr 9, 2019

    Disagree; if you're writing signal-handling code you should be very careful to do it properly, even if that's only proper for your current platform. If you can't do it properly you should find an alternative that doesn't involve signals.

    The fact that sig_atomic_t is only 1 byte on VxWorks strongly implies using int WILL fail in strange ways on that platform. I can see three options:

    1. use pycore_atomic.h, implementing it for VxWorks if you haven't already. This also implies sig_atomic_t could have been int but wasn't for some reason, such as performance.
    2. disable wakeup_fd entirely. It's obscure, GNOME being the biggest user I can think of.
    3. unpack the int into an array of sig_atomic_t. Only the main thread writes to it so this method is ugly but viable.

    @jdemeyer
    Copy link
    Contributor

    jdemeyer commented Apr 9, 2019

    I'm not sure with what you disagree. At least, you have to admit that using sig_atomic_t is buggy for different reasons than signal safety, namely that there is no guarantee that one can safely convert back and forth to an "int".

    @jdemeyer
    Copy link
    Contributor

    jdemeyer commented Apr 9, 2019

    unpack the int into an array of sig_atomic_t.

    What do you mean with this? You can't write a complete array atomically, so I don't see how this would help.

    @Rhamphoryncus
    Copy link
    Mannequin Author

    Rhamphoryncus mannequin commented Apr 9, 2019

    Converting to/from sig_atomic_t could have a compile time check on currently supported platforms and isn't buggy for them. For platforms with a different size you could do a runtime check, only allowing a fd in the range of 0-254 (with 255 reserved); that could sometimes fail, yes, but at least it's explicit, easily understood failure. Just using int would fail in undefined ways down the road, likely writing to a random fd instead (corrupting whatever it was doing), with no way to trace it back.

    Unpacking the int would mean having one sig_atomic_t for 'invalid', using that instead of INVALID_FD, plus an array of sig_atomic_t for the fd itself. Every time you want to change the fd you first set the 'invalid' flag, then the individual bytes, then clear 'invalid'.

    @jdemeyer
    Copy link
    Contributor

    Unpacking the int would mean having one sig_atomic_t for 'invalid', using that instead of INVALID_FD, plus an array of sig_atomic_t for the fd itself. Every time you want to change the fd you first set the 'invalid' flag, then the individual bytes, then clear 'invalid'.

    I'm not sure that this is thread-safe as processors can reorder instructions, so there is no guarantee that memory is written in the expected order. That's one of the problems that C11/C++11 atomics solve.

    @Rhamphoryncus
    Copy link
    Mannequin Author

    Rhamphoryncus mannequin commented Apr 10, 2019

    signal-safe is different from thread-safe (despite conceptual similarities), but regardless it's been a long time since I last delved into this so I'm quite rusty. I could be doing it all wrong.

    @jdemeyer
    Copy link
    Contributor

    signal-safe is different from thread-safe

    I know, but I think that other threads can receive signals too. So in that case, it needs to be signal-safe as well as thread-safe.

    @Rhamphoryncus
    Copy link
    Mannequin Author

    Rhamphoryncus mannequin commented Apr 10, 2019

    signalmodule.c has a hack to limit it to the main thread. Otherwise there's all sorts of platform-specific behaviour.

    @jdemeyer
    Copy link
    Contributor

    signalmodule.c has a hack to limit it to the main thread.

    The Python signal handler always runs in the main thread, but the signal can be caught by any thread. In other words, trip_signal() can be run by any thread.

    @vstinner
    Copy link
    Member

    Please don't discuss on closed issues. Either reopen the issue or open a new issue.

    @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
    extension-modules C modules in the Modules dir
    Projects
    None yet
    Development

    No branches or pull requests

    4 participants