classification
Title: In select.poll.poll() ms can be 0 if timeout < 0
Type: behavior Stage: resolved
Components: Extension Modules Versions: Python 3.7, Python 3.6
process
Status: closed Resolution: fixed
Dependencies: Superseder:
Assigned To: Nosy List: Riccardo Coccioli, berker.peksag, pablogsal, serhiy.storchaka, vstinner
Priority: normal Keywords: patch

Created on 2017-10-14 08:07 by serhiy.storchaka, last changed 2017-10-18 08:34 by serhiy.storchaka. This issue is now closed.

Pull Requests
URL Status Linked Edit
PR 4003 merged pablogsal, 2017-10-15 02:57
PR 4022 merged pablogsal, 2017-10-17 20:40
PR 4031 merged serhiy.storchaka, 2017-10-18 08:08
Messages (12)
msg304386 - (view) Author: Serhiy Storchaka (serhiy.storchaka) * (Python committer) Date: 2017-10-14 08:07
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.
msg304427 - (view) Author: STINNER Victor (vstinner) * (Python committer) Date: 2017-10-15 09:40
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.
msg304431 - (view) Author: Serhiy Storchaka (serhiy.storchaka) * (Python committer) Date: 2017-10-15 11:20
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 issue31334.
msg304439 - (view) Author: Pablo Galindo Salgado (pablogsal) * (Python committer) Date: 2017-10-15 14:13
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:

https://github.com/python/cpython/pull/4003
msg304440 - (view) Author: Serhiy Storchaka (serhiy.storchaka) * (Python committer) Date: 2017-10-15 14:32
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?
msg304445 - (view) Author: Pablo Galindo Salgado (pablogsal) * (Python committer) Date: 2017-10-15 19:18
I have updated both the test and the implementation to address your feedback.
msg304456 - (view) Author: STINNER Victor (vstinner) * (Python committer) Date: 2017-10-16 07:42
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.
msg304463 - (view) Author: STINNER Victor (vstinner) * (Python committer) Date: 2017-10-16 08:28
> It seems like _PyTime_ROUND_CEILING is not needed in Python currently.

Oops, my pending PR 3983 uses _PyTime_ROUND_CEILING :-) Please keep it.
msg304511 - (view) Author: Serhiy Storchaka (serhiy.storchaka) * (Python committer) Date: 2017-10-17 14:40
New changeset 2c15b29aea5d6b9c61aa42d2c24a07ff1edb4b46 by Serhiy Storchaka (Pablo Galindo) in branch 'master':
bpo-31786: Make functions in the select module blocking when timeout is a small negative value. (#4003)
https://github.com/python/cpython/commit/2c15b29aea5d6b9c61aa42d2c24a07ff1edb4b46
msg304559 - (view) Author: Serhiy Storchaka (serhiy.storchaka) * (Python committer) Date: 2017-10-18 08:12
New changeset 95602b368b87da3702a0f340ded2a23e823bb104 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). (#4022)
https://github.com/python/cpython/commit/95602b368b87da3702a0f340ded2a23e823bb104
msg304562 - (view) Author: Serhiy Storchaka (serhiy.storchaka) * (Python committer) Date: 2017-10-18 08:28
New changeset ed267e3305a54eddae8106bdaae2c62d4c3b7db6 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). (#4031)
https://github.com/python/cpython/commit/ed267e3305a54eddae8106bdaae2c62d4c3b7db6
msg304564 - (view) Author: Serhiy Storchaka (serhiy.storchaka) * (Python committer) Date: 2017-10-18 08:34
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.
History
Date User Action Args
2017-10-18 08:34:19serhiy.storchakasetstatus: open -> closed
resolution: fixed
messages: + msg304564

stage: patch review -> resolved
2017-10-18 08:28:41serhiy.storchakasetmessages: + msg304562
2017-10-18 08:12:49serhiy.storchakasetmessages: + msg304559
2017-10-18 08:08:37serhiy.storchakasetpull_requests: + pull_request4005
2017-10-17 20:40:08pablogsalsetstage: backport needed -> patch review
pull_requests: + pull_request3997
2017-10-17 14:46:13serhiy.storchakasetstage: patch review -> backport needed
2017-10-17 14:40:16serhiy.storchakasetmessages: + msg304511
2017-10-16 13:27:06Riccardo Cocciolisetnosy: + Riccardo Coccioli
2017-10-16 08:28:51vstinnersetmessages: + msg304463
2017-10-16 07:42:38vstinnersetmessages: + msg304456
2017-10-15 19:18:04pablogsalsetmessages: + msg304445
2017-10-15 14:32:50serhiy.storchakasetmessages: + msg304440
2017-10-15 14:13:38pablogsalsetnosy: + pablogsal
messages: + msg304439
2017-10-15 11:20:28serhiy.storchakasetmessages: + msg304431
2017-10-15 10:20:10berker.peksagsetnosy: + berker.peksag
2017-10-15 09:40:42vstinnersetmessages: + msg304427
2017-10-15 02:57:08pablogsalsetkeywords: + patch
stage: patch review
pull_requests: + pull_request3976
2017-10-14 08:07:08serhiy.storchakacreate