bpo-31786: Make select.poll.poll block if ms < 0 for every value of ms #4003

pablogsal · 2017-10-15T02:57:06Z

Acording to the documentation select.poll.poll() is blocked for infinite timeout if the timeout argument is negative but:

>>> import select
>>> poll = select.poll()
>>> poll.poll(-0.000000000000000001)
[]

This is due to rounding issues when making the syscall. This PR fixes that so the call to poll.poll will block if the argument is negative.

https://bugs.python.org/issue31786

pablogsal · 2017-10-15T14:20:32Z

Following @Haypo and @serhiy-storchaka discussion in the issue: Please, if you prefer a better/different way of dealing with this issue, I am more than happy to update the PR with the required changes.

vstinner

ms > 0 doesn't seem to be correct to me.

I prefer to fix the rouding and fix all functions.

bedevere-bot · 2017-10-15T17:39:57Z

A Python core developer has requested some changes be made to your pull request before we can consider merging it. If you could please address their requests along with any other requests in other reviews from core developers that would be appreciated.

Once you have made the requested changes, please leave a comment on this pull request containing the phrase I have made the requested changes; please review again. I will then notify any core developers who have left a review that you're ready for them to take another look at this pull request.

serhiy-storchaka · 2017-10-15T17:53:22Z

This part doesn't look correct to me too. But the test may be useful if make it testing other infinity timeout values. They are not tested explicitly.

pablogsal · 2017-10-15T19:11:51Z

@Haypo I have implemented ROUND_UP in pytime.h and pytime.c following your instructions and substitute the usage of ROUND_CEILING in the select module. Please, if you are happy with this implementation, check that the usage of the new ROUND_UP is correct (I think it is but maybe I am missing something).

@serhiy-storchaka I have updated the test so it takes into account more infinite timeout values.

If you prefer a different implementation or use this in more/less places, say so and I will update this PR.

Thank you for your time reviewing this PR! 😄

pablogsal · 2017-10-15T19:14:43Z

I have made the requested changes; please review again

bedevere-bot · 2017-10-15T19:14:45Z

Thanks for making the requested changes!

@Haypo: please review the changes made to this pull request.

serhiy-storchaka · 2017-10-15T19:26:15Z

Python/pytime.c

        d = floor(d);
    }
+    else {
+        d = (d < 0.0) ? floor(d) : ceil(d);


Add assert(round == _PyTime_ROUND_UP).

Done in 5c58695

serhiy-storchaka · 2017-10-15T19:27:31Z

Lib/test/test_poll.py

+    @unittest.skipUnless(threading, 'Threading required for this test.')
+    @reap_threads
+    def test_poll_blocks_with_negative_ms(self):
+


There are too much empty lines. I think the code would look better without them. Empty lines should be used for separating functions, classes, and, sparingly, to indicate logical sections in functions.

Done in 5c58695

serhiy-storchaka · 2017-10-15T19:33:28Z

Lib/test/test_poll.py

+
+        for timeout_ms in [None, -1, -1.0, -0.1, -1e-100]:
+
+            # GIVEN


Sorry, but I don't understand what mean these GIVEN/WHEN/THEN.

Sorry, I add then for clarity when testing (example).

I will delete them if you want.

Done in 5c58695

serhiy-storchaka · 2017-10-15T19:34:29Z

Lib/test/test_poll.py

+            # Create two file descriptors. This will be used to unlock
+            # the blocking call to poll.poll inside the thread
+
+            r, w = os.pipe()


A pair of descriptors is created for every timeout_ms value, but they are closed only after finishing the test.

Done in 5c58695

… _PyTime_ROUND_CEILING

serhiy-storchaka · 2017-10-15T20:13:58Z

Modules/selectmodule.c

@@ -594,7 +594,7 @@ poll_poll(pollObject *self, PyObject *args)
                poll_result = 0;
                break;
            }
-            ms = _PyTime_AsMilliseconds(timeout, _PyTime_ROUND_CEILING);
+            ms = _PyTime_AsMilliseconds(timeout, _PyTime_ROUND_UP);


Since timeout >= 0, there is no difference between _PyTime_ROUND_CEILING and _PyTime_ROUND_UP here.

Done in 5c96cd6

serhiy-storchaka · 2017-10-15T20:14:15Z

Modules/selectmodule.c

@@ -282,7 +282,7 @@ select_select(PyObject *self, PyObject *args)
                n = 0;
                break;
            }
-            _PyTime_AsTimeval_noraise(timeout, &tv, _PyTime_ROUND_CEILING);
+            _PyTime_AsTimeval_noraise(timeout, &tv, _PyTime_ROUND_UP);


Since timeout >= 0, there is no difference between _PyTime_ROUND_CEILING and _PyTime_ROUND_UP here.

Done in 5c96cd6

serhiy-storchaka · 2017-10-15T20:18:43Z

Modules/selectmodule.c

@@ -221,7 +221,7 @@ select_select(PyObject *self, PyObject *args)
            return NULL;
        }

-        if (_PyTime_AsTimeval(timeout, &tv, _PyTime_ROUND_CEILING) == -1)
+        if (_PyTime_AsTimeval(timeout, &tv, _PyTime_ROUND_UP) == -1)


timeout above is rounded with _PyTime_ROUND_CEILING. Negative values between -1e-9 and 0 already are rounded to 0.

Done in 5c96cd6

Shouldn't _PyTime_ROUND_UP be used instead in _PyTime_FromSecondsObject()?

Yes! Corrected in 3f1f553. I am sorry if I miss a few places. A combination of ROUND_CEILING and ROUND_UP was causing a rounding error in test.eintrdata.eintr_tester.TimeEINTRTest and I was a bit paranoid and disoriented when changing these. 😞

pablogsal · 2017-10-15T21:32:23Z

Should I add a NEWS entry?

serhiy-storchaka · 2017-10-15T21:33:43Z

Yes, this change requires a NEWS entry.

serhiy-storchaka · 2017-10-15T21:41:23Z

Modules/selectmodule.c

@@ -213,7 +213,7 @@ select_select(PyObject *self, PyObject *args)
        tvp = (struct timeval *)NULL;
    else {
        if (_PyTime_FromSecondsObject(&timeout, timeout_obj,
-                                      _PyTime_ROUND_CEILING) < 0) {
+                                      _PyTime_ROUND_UP) < 0) {


Different direction of rounding shouldn't be mixed. Use _PyTime_ROUND_UP in _PyTime_AsTimeval() below, otherwise the timeout from -999 to -1 nanoseconds will be rounded to 0.

Solved in 3f7ccda.

No, this is not needed. At line 285 the value is not negative.

Thank you for the patience!

miss-islington · 2017-10-17T14:15:06Z

Thanks @pablogsal for the PR, and @serhiy-storchaka for merging it 🌮🎉.. I'm working now to backport this PR to: 2.7, 3.6.
🐍🍒⛏🤖

miss-islington · 2017-10-17T14:15:11Z

Sorry, @pablogsal and @serhiy-storchaka, I could not cleanly backport this to 3.6 due to a conflict.
Please backport using cherry_picker on command line.
cherry_picker 2c15b29aea5d6b9c61aa42d2c24a07ff1edb4b46 3.6

miss-islington · 2017-10-17T14:15:31Z

Thanks @pablogsal for the PR, and @serhiy-storchaka for merging it 🌮🎉.. I'm working now to backport this PR to: 2.7, 3.6.
🐍🍒⛏🤖

miss-islington · 2017-10-17T14:16:08Z

Sorry, @pablogsal and @serhiy-storchaka, I could not cleanly backport this to 2.7 due to a conflict.
Please backport using cherry_picker on command line.
cherry_picker 2c15b29aea5d6b9c61aa42d2c24a07ff1edb4b46 2.7

miss-islington · 2017-10-17T14:17:09Z

Sorry, @pablogsal and @serhiy-storchaka, I could not cleanly backport this to 2.7 due to a conflict.
Please backport using cherry_picker on command line.
cherry_picker 2c15b29aea5d6b9c61aa42d2c24a07ff1edb4b46 2.7

miss-islington · 2017-10-17T14:18:09Z

Sorry, @pablogsal and @serhiy-storchaka, I could not cleanly backport this to 2.7 due to a conflict.
Please backport using cherry_picker on command line.
cherry_picker 2c15b29aea5d6b9c61aa42d2c24a07ff1edb4b46 2.7

miss-islington · 2017-10-17T14:19:08Z

Sorry, @pablogsal and @serhiy-storchaka, I could not cleanly backport this to 3.6 due to a conflict.
Please backport using cherry_picker on command line.
cherry_picker 2c15b29aea5d6b9c61aa42d2c24a07ff1edb4b46 3.6

miss-islington · 2017-10-17T14:20:08Z

Sorry, @pablogsal and @serhiy-storchaka, I could not cleanly backport this to 3.6 due to a conflict.
Please backport using cherry_picker on command line.
cherry_picker 2c15b29aea5d6b9c61aa42d2c24a07ff1edb4b46 3.6

serhiy-storchaka · 2017-10-17T14:30:08Z

@Mariatta, seems @miss-islington tries to create backports again and again.

vstinner · 2017-10-17T14:45:23Z

@Mariatta, seems @miss-islington tries to create backports again and again.

I removed the "needs backport" labels to prevent the issue.

serhiy-storchaka · 2017-10-17T14:45:42Z

Do you mind to backport this to 3.6 and 2.7 @pablogsal?

pablogsal · 2017-10-17T15:02:41Z

@serhiy-storchaka No problem! Should I do this before fixing the other functions?

Thank you very much for your patience and time, @Haypo and @serhiy-storchaka ! 😄

serhiy-storchaka · 2017-10-17T15:14:02Z

Please do this before #3277 be merged.

Mariatta · 2017-10-17T16:26:26Z

😕 Thanks for the ping! I'll take a look at miss-islington's logs to see what's going on.

Mariatta · 2017-10-17T16:54:20Z

I opened a bug 🐛 python/miss-islington#35

…meout is a small negative value. (pythonGH-4003). (cherry picked from commit 2c15b29)

pablogsal · 2017-10-17T20:55:28Z

@Haypo, @serhiy-storchaka: I have backported this to 3.6: #4022 .

I am wondering if this can be packported directly to 2.7... The reason is that /Modules/selectmodule.c doesn't use any rounding method from pytime among other major differences. Am I missing something?

serhiy-storchaka · 2017-10-17T21:07:13Z

If this is not relevant to 2.7, it is worth to backport just tests.

pablogsal · 2017-10-17T22:07:39Z

Sorry, I did not explain correctly: 2.7 still has the same issue, but this cannot be backported directly to 2.7 due to very different strucutres. For example, /Modules/selectmodule.c doesn't use any rounding method from pytime.In order to backport this to 2.7 bigger changes should be performed (at least I don't know how to measure how many things should be changed).

…meout is a small negative value. (pythonGH-4003). (cherry picked from commit 2c15b29)

…meout is a small negative value. (GH-4003). (#4022) (cherry picked from commit 2c15b29)

…meout is a small negative value. (GH-4003). (#4031) (cherry picked from commit 2c15b29)

serhiy-storchaka · 2017-10-18T08:29:56Z

The test is backported by #4031.

the-knights-who-say-ni added the CLA signed label Oct 15, 2017

bedevere-bot added the awaiting review label Oct 15, 2017

vstinner requested changes Oct 15, 2017

View reviewed changes

bedevere-bot added awaiting changes and removed awaiting review labels Oct 15, 2017

pablogsal force-pushed the 31786 branch from e53fabf to ebac53a Compare October 15, 2017 19:07

bedevere-bot added awaiting change review and removed awaiting changes labels Oct 15, 2017

serhiy-storchaka reviewed Oct 15, 2017

View reviewed changes

pablogsal force-pushed the 31786 branch from 0793fa8 to 5c58695 Compare October 15, 2017 19:45

pablogsal added 5 commits October 15, 2017 21:05

Make select.poll.poll block if ms < 0 for every value of ms

17a220c

Add test for poll with negative ms

3520372

Fix poll.poll test so it doesn't leak resources

0555859

Implement _PyTime_ROUND_UP and use it in the select module instead of…

d47221a

… _PyTime_ROUND_CEILING

Add more timeout values to test_poll_blocks_with_negative_ms

ebac53a

serhiy-storchaka reviewed Oct 15, 2017

View reviewed changes

pablogsal force-pushed the 31786 branch from 9c20930 to 5c96cd6 Compare October 15, 2017 20:36

pablogsal added 2 commits October 15, 2017 21:45

Fix test_poll and add assertion in pytime.c

5c58695

Fix rounding error failure in test.eintrdata.eintr_tester.TimeEINTRTest

80fa13e

Use ROUND_CEILING again in some places where ROUND_UP is not needed

5c96cd6

serhiy-storchaka reviewed Oct 15, 2017

View reviewed changes

Replace ROUND_CEILING for ROUND_UP in select.select check

3f1f553

serhiy-storchaka added the type-bug An unexpected behavior, bug, or error label Oct 17, 2017

vstinner removed needs backport to 2.7 labels Oct 17, 2017

Mariatta mentioned this pull request Oct 17, 2017

Multiple backport attempts python/miss-islington#35

Closed

pablogsal added a commit to pablogsal/cpython that referenced this pull request Oct 17, 2017

[3.6] bpo-31786: Make functions in the select module blocking when ti…

2da7d04

…meout is a small negative value. (pythonGH-4003). (cherry picked from commit 2c15b29)

pablogsal mentioned this pull request Oct 17, 2017

bpo-31806: Use _PyTime_ROUND_TIMEOUT in _threadmodule.c, timemodule.c and socketmodule.c #4026

Merged

serhiy-storchaka pushed a commit to serhiy-storchaka/cpython that referenced this pull request Oct 18, 2017

[2.7] bpo-31786: Make functions in the select module blocking when ti…

c2786e0

…meout is a small negative value. (pythonGH-4003). (cherry picked from commit 2c15b29)

serhiy-storchaka pushed a commit that referenced this pull request Oct 18, 2017

[3.6] bpo-31786: Make functions in the select module blocking when ti…

95602b3

…meout is a small negative value. (GH-4003). (#4022) (cherry picked from commit 2c15b29)

serhiy-storchaka added a commit that referenced this pull request Oct 18, 2017

[2.7] bpo-31786: Make functions in the select module blocking when ti…

ed267e3

…meout is a small negative value. (GH-4003). (#4031) (cherry picked from commit 2c15b29)

pablogsal deleted the 31786 branch October 18, 2017 08:44

bpo-31786: Make select.poll.poll block if ms < 0 for every value of ms #4003

bpo-31786: Make select.poll.poll block if ms < 0 for every value of ms #4003

Conversation

pablogsal commented Oct 15, 2017 • edited Loading

pablogsal commented Oct 15, 2017 • edited Loading

vstinner left a comment

Choose a reason for hiding this comment

bedevere-bot commented Oct 15, 2017

serhiy-storchaka commented Oct 15, 2017

pablogsal commented Oct 15, 2017

pablogsal commented Oct 15, 2017

bedevere-bot commented Oct 15, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pablogsal Oct 15, 2017 • edited Loading

Choose a reason for hiding this comment

pablogsal commented Oct 15, 2017

serhiy-storchaka commented Oct 15, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

miss-islington commented Oct 17, 2017

miss-islington commented Oct 17, 2017

miss-islington commented Oct 17, 2017

miss-islington commented Oct 17, 2017

miss-islington commented Oct 17, 2017

miss-islington commented Oct 17, 2017

miss-islington commented Oct 17, 2017

miss-islington commented Oct 17, 2017

serhiy-storchaka commented Oct 17, 2017

vstinner commented Oct 17, 2017

serhiy-storchaka commented Oct 17, 2017

pablogsal commented Oct 17, 2017

serhiy-storchaka commented Oct 17, 2017

Mariatta commented Oct 17, 2017

Mariatta commented Oct 17, 2017

pablogsal commented Oct 17, 2017 • edited Loading

serhiy-storchaka commented Oct 17, 2017

pablogsal commented Oct 17, 2017 • edited Loading

serhiy-storchaka commented Oct 18, 2017

pablogsal commented Oct 15, 2017 •

edited

Loading

pablogsal commented Oct 15, 2017 •

edited

Loading

pablogsal Oct 15, 2017 •

edited

Loading

pablogsal commented Oct 17, 2017 •

edited

Loading

pablogsal commented Oct 17, 2017 •

edited

Loading