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

gevent.select.poll handles negative timeouts wrong, especially on libuv #1127

Closed
jamadden opened this Issue Mar 1, 2018 · 0 comments

Comments

Projects
None yet
1 participant
@jamadden
Member

jamadden commented Mar 1, 2018

gevent.select.poll.poll([timeout]) contains this code snippet:

                if timeout is not None and timeout > -1:
                    timeout /= 1000.0
                result.event.wait(timeout=timeout)

When called with -1, that has the effect of passing -1 and so forth on down through Event -> Timeout -> loop.timer().

Timeout does not specify what happens with negative values, it just notes that they are poorly defined, so it's up to the loop to interpret it.

On libuv, timeout values < 0 cause a check watcher to be used, which wakes up on the next iteration of the loop.

On libev it's not (immediately) clear what a negative value does. At the very least, it's going to allocate an extra C-level watcher object.

The stdlib documentation for select.poll.poll since at least 3.5 says:

If timeout is omitted, negative, or None, the call will block until there is an event for this poll object.

So we're definitely wrong on libuv. We're possibly also wrong on libev, depending on its implementation. At the very least, we're creating C objects we don't need.

A negative timeout should be handled the same as None.

(Discovered while tracing through #1126 )

@jamadden jamadden added this to the 1.3 milestone Mar 1, 2018

jamadden added a commit that referenced this issue Mar 2, 2018

Fix #1127 by interpreting -1 as None under libuv for select.poll.poll…
…; should probably start having Timeout do that itself.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment