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

signal.set_wakeup_fd(fd): raise an exception if the fd is in blocking mode #66241

Closed
vstinner opened this issue Jul 23, 2014 · 10 comments
Closed

Comments

@vstinner
Copy link
Member

BPO 22042
Nosy @pitrou, @vstinner
Files
  • signal_nonblock.patch
  • signal_check_nonblocking.patch
  • signal_check_nonblocking-2.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 = <Date 2014-08-27.11:00:31.507>
    created_at = <Date 2014-07-23.00:49:30.863>
    labels = []
    title = 'signal.set_wakeup_fd(fd): raise an exception if the fd is in blocking mode'
    updated_at = <Date 2014-08-27.12:02:46.889>
    user = 'https://github.com/vstinner'

    bugs.python.org fields:

    activity = <Date 2014-08-27.12:02:46.889>
    actor = 'python-dev'
    assignee = 'none'
    closed = True
    closed_date = <Date 2014-08-27.11:00:31.507>
    closer = 'vstinner'
    components = []
    creation = <Date 2014-07-23.00:49:30.863>
    creator = 'vstinner'
    dependencies = []
    files = ['36039', '36131', '36156']
    hgrepos = []
    issue_num = 22042
    keywords = ['patch']
    message_count = 10.0
    messages = ['223710', '223712', '223717', '223777', '223785', '224132', '224263', '224264', '225971', '225977']
    nosy_count = 4.0
    nosy_names = ['pitrou', 'vstinner', 'neologix', 'python-dev']
    pr_nums = []
    priority = 'normal'
    resolution = 'fixed'
    stage = None
    status = 'closed'
    superseder = None
    type = None
    url = 'https://bugs.python.org/issue22042'
    versions = ['Python 3.5']

    @vstinner
    Copy link
    Member Author

    I think that signal.set_wakeup_fd(fd) must set the file descriptor to the non-blocking mode, instead of requiring the file descriptor mode to be non-blocking.

    Atttached patch implements this idea.

    See also the issue bpo-22018 which proposes to support sockets in signal.set_wakeup_fd(fd) on Windows.

    We have to decide if signal.set_wakeup_fd() is supposed to support files or not on Windows. Non-blocking mode does not exist for files on Windows, only for sockets.

    I didn't test my patch on Windows yet.

    Note: the new _Py_set_blocking() function tries to use ioctl() instead of fnctl() when available to only use one syscall instead of two. I used the same optimization in _Py_set_inheritable().

    @pitrou
    Copy link
    Member

    pitrou commented Jul 23, 2014

    I think it would be much better to expose a generic function to make a fd non-blocking, rather than bake it inside signal.set_wakeup_fd().

    Also, given set_wakeup_fd() is rather specialized and uncommon, I don't see much point in making it more convenient.

    @vstinner
    Copy link
    Member Author

    "Also, given set_wakeup_fd() is rather specialized and uncommon, I don't see much point in making it more convenient."

    The problem is that passing a blocking file descriptor can block the whole Python process. The hang does not occur immediatly, but only when the pipe is full for example, which may only occur in some corner cases (ex: heavy system load, slow network, etc.).

    The minimum would be to raise an error if the file descriptor is in blocking mode.

    @python-dev
    Copy link
    Mannequin

    python-dev mannequin commented Jul 23, 2014

    New changeset 9dc66b3a1d0d by Victor Stinner in branch 'default':
    Issue bpo-22042: Avoid dangerous C cast in socket.setblocking()
    http://hg.python.org/cpython/rev/9dc66b3a1d0d

    @vstinner
    Copy link
    Member Author

    As suggested by Antoine, I created the issue bpo-22054 to add os.get_blocking() and os.set_blocking() functions.

    Here is the simplified patch for signal.set_wakeup_fd(fd). It now depends on the patch of the issue bpo-22054.

    @vstinner vstinner changed the title signal.set_wakeup_fd(fd): set the fd to non-blocking mode signal.set_wakeup_fd(fd): raise an exception if the fd is in blocking mode Jul 27, 2014
    @vstinner
    Copy link
    Member Author

    signal_check_nonblocking.patch: signal.set_wakeup_fd() now raises a ValueError if the file descriptor is in blocking mode.

    The patch depends on the patch of the issue bpo-22054.

    I didn't test the patch on Windows, but all calls to signal.set_wakeup_fd() must fail with ValueError since it's not possible to set a file in non-blocking mode. See the issue bpo-22018 which proposes to support sockets for signal.set_wakeup_fd().

    @vstinner
    Copy link
    Member Author

    On Windows, it looks like it's not possible to test if a socket handle (int, not a socket object) is blocking or not. The WSAIsBlocking() function was removed, it's only possible to set the flag using ioctlsocket().

    @vstinner
    Copy link
    Member Author

    I commited my patch to support sockets in signal.set_wakeup_fd() on Windows.

    I updated my patch. It doesn't change signal.set_wakeup_fd() on Windows anymore. It only raises an exception on POSIX if the file descriptor is blocking.

    On Windows, it's not possible to make a file non-blocking and so the signal handler may block on writing in the file. Antoine Pitrou doesn't want to change the behaviour, he prefers to still support files even if there is the possible hang.

    On Windows, it's not possible to check if a socket handle is blocking or not. So set_wakeup_fd() doesn't check if the socket is blocking or not on Windows.

    At least, the check can be implemented on POSIX.

    --

    An alternative is to make the file descriptor non-blocking in set_wakeup_fd(). In this case, I suggest to drop support of files on Windows because files cannot be set in non-blocking mode.

    @python-dev
    Copy link
    Mannequin

    python-dev mannequin commented Aug 27, 2014

    New changeset d984dfe8c34e by Victor Stinner in branch 'default':
    Issue bpo-22042: signal.set_wakeup_fd(fd) now raises an exception if the file
    http://hg.python.org/cpython/rev/d984dfe8c34e

    @python-dev
    Copy link
    Mannequin

    python-dev mannequin commented Aug 27, 2014

    New changeset f5f5553f219e by Victor Stinner in branch 'default':
    Issue bpo-22042: Fix test_signal on Windows
    http://hg.python.org/cpython/rev/f5f5553f219e

    @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
    None yet
    Projects
    None yet
    Development

    No branches or pull requests

    2 participants