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

In select.poll.poll() ms can be 0 if timeout < 0 #75967

Closed
serhiy-storchaka opened this issue Oct 14, 2017 · 12 comments
Closed

In select.poll.poll() ms can be 0 if timeout < 0 #75967

serhiy-storchaka opened this issue Oct 14, 2017 · 12 comments
Labels
3.7 (EOL) end of life extension-modules C modules in the Modules dir type-bug An unexpected behavior, bug, or error

Comments

@serhiy-storchaka
Copy link
Member

BPO 31786
Nosy @vstinner, @berkerpeksag, @serhiy-storchaka, @pablogsal, @volans-
PRs
  • bpo-31786: Make select.poll.poll block if ms < 0 for every value of ms #4003
  • [3.6] bpo-31786: Make select.poll.poll block if ms < 0 for every value of ms (GH-4003) #4022
  • [2.7] bpo-31786: Add test for select.poll.poll() with infinite timeout. (GH-4003). #4031
  • 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 2017-10-18.08:34:19.835>
    created_at = <Date 2017-10-14.08:07:08.015>
    labels = ['extension-modules', 'type-bug', '3.7']
    title = 'In select.poll.poll() ms can be 0 if timeout < 0'
    updated_at = <Date 2017-10-18.08:34:19.834>
    user = 'https://github.com/serhiy-storchaka'

    bugs.python.org fields:

    activity = <Date 2017-10-18.08:34:19.834>
    actor = 'serhiy.storchaka'
    assignee = 'none'
    closed = True
    closed_date = <Date 2017-10-18.08:34:19.835>
    closer = 'serhiy.storchaka'
    components = ['Extension Modules']
    creation = <Date 2017-10-14.08:07:08.015>
    creator = 'serhiy.storchaka'
    dependencies = []
    files = []
    hgrepos = []
    issue_num = 31786
    keywords = ['patch']
    message_count = 12.0
    messages = ['304386', '304427', '304431', '304439', '304440', '304445', '304456', '304463', '304511', '304559', '304562', '304564']
    nosy_count = 5.0
    nosy_names = ['vstinner', 'berker.peksag', 'serhiy.storchaka', 'pablogsal', 'Riccardo Coccioli']
    pr_nums = ['4003', '4022', '4031']
    priority = 'normal'
    resolution = 'fixed'
    stage = 'resolved'
    status = 'closed'
    superseder = None
    type = 'behavior'
    url = 'https://bugs.python.org/issue31786'
    versions = ['Python 3.6', 'Python 3.7']

    @serhiy-storchaka
    Copy link
    Member Author

    According to the documentation select.poll.poll() is blocked for infinite timeout if the timeout argument is negative. But due to rounding it is possible that timeout < 0, but an integer ms passed to the poll() syscall is 0, and poll() is not blocked.

    @serhiy-storchaka serhiy-storchaka added 3.7 (EOL) end of life extension-modules C modules in the Modules dir type-bug An unexpected behavior, bug, or error labels Oct 14, 2017
    @vstinner
    Copy link
    Member

    I suggest to implement ROUND_UP in pytime and use it in all functions
    accepting a timeout, not only poll. Search for ROUND_CEILING in the code to
    find them. But functions accepting time like clock_settime() must continue
    to use ROUND_CEILING.

    Does someone want to work on such patch? The new rounding mode must be test
    in test_time, you should just add the new mode at the top once if I recall
    correctly.

    ---

    select.poll currently uses ROUND_CEILING: round towards +infinity.

    It looks like you would prefer ROUND_UP: round away from zero, right?

    In the history of CPython, the rounding mode of functions accepting time,
    timeout, duration, etc. changed many times, mostly because the C code
    changed in subtle ways to fix different bugs. I mean that the exact
    rounding mode wasn't really chosen on purpose. I'm now trying to get better
    defined rouding modes in pytime, but it seems poll uses the wrong one.

    https://haypo.github.io/pytime.html

    I think that my problem is that I wanted to reuse the same rounding modes
    for different use cases. It seems like clocks need ROUND_CEILING but
    timeouts need ROUND_UP. Well, in the case of poll, the previous code before
    moving to pytime was using ceil() which uses ROUND_CEILING.

    @serhiy-storchaka
    Copy link
    Member Author

    This particular issue can be fixed by adding the condition (PyFloat_Check(timeout_obj) && PyFloat_AsDouble(timeout_obj) < 0). The problem is only with writing good test for infinity timeout.

    This test could be also used for testing bpo-31334.

    @pablogsal
    Copy link
    Member

    I have added a Pull Request fixing this issue. The current implementation is checking in the syscall if the value for ms before rounding was negative.

    To test for this, I call poll.poll in a thread and check that the thread is alive after a join with timeout (so this means it has blocked). Then to clean up, I write to a pipe and therefore it unblocks.

    The implementation is available in the PR:

    #4003

    @serhiy-storchaka
    Copy link
    Member Author

    Thank you Pablo. The initial test leaked a thread, now it is much better.

    Could you please make the test testing different timeout values: None, -1, -1.0, -0.1, -1e-100?

    @pablogsal
    Copy link
    Member

    I have updated both the test and the implementation to address your feedback.

    @vstinner
    Copy link
    Member

    Ok, so it seems like we need 3 rounding modes:

    • _PyTime_ROUND_FLOOR: read a clock, like datetime.datetime.now(). We need to round nanoseconds since datetime.datetime only supports 1 us resolution

    • _PyTime_ROUND_HALF_EVEN: "round from a Python float" like round(float), used by datetime.datetime.fromtimestamp()

    • _PyTime_ROUND_UP: round timeouts, socket.settimeout(), lock.acquire(timeout), poll(timeout), etc.

    _PyTime_ROUND_UP and _PyTime_ROUND_CEILING are the same for positive numbers, but using _PyTime_ROUND_CEILING causes this bug: values in ]-0.5; 0.0[ are rounding to zero which gives the wrong behaviour. It seems like _PyTime_ROUND_CEILING is not needed in Python currently.

    @vstinner
    Copy link
    Member

    It seems like _PyTime_ROUND_CEILING is not needed in Python currently.

    Oops, my pending PR 3983 uses _PyTime_ROUND_CEILING :-) Please keep it.

    @serhiy-storchaka
    Copy link
    Member Author

    New changeset 2c15b29 by Serhiy Storchaka (Pablo Galindo) in branch 'master':
    bpo-31786: Make functions in the select module blocking when timeout is a small negative value. (bpo-4003)
    2c15b29

    @serhiy-storchaka
    Copy link
    Member Author

    New changeset 95602b3 by Serhiy Storchaka (Pablo Galindo) in branch '3.6':
    [3.6] bpo-31786: Make functions in the select module blocking when timeout is a small negative value. (GH-4003). (bpo-4022)
    95602b3

    @serhiy-storchaka
    Copy link
    Member Author

    New changeset ed267e3 by Serhiy Storchaka in branch '2.7':
    [2.7] bpo-31786: Make functions in the select module blocking when timeout is a small negative value. (GH-4003). (bpo-4031)
    ed267e3

    @serhiy-storchaka
    Copy link
    Member Author

    Thank you for your contribution Pablo.

    The issue is fixed in 3.6 and 3.7. It is hard to fix it in 2.7.

    @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
    3.7 (EOL) end of life extension-modules C modules in the Modules dir type-bug An unexpected behavior, bug, or error
    Projects
    None yet
    Development

    No branches or pull requests

    3 participants