bpo-31334: Fix timeout in select.poll.poll() #3277

volans- · 2017-09-03T22:38:39Z

Always pass -1, or INFTIM where defined, to the poll() system call when
a negative timeout is passed to the poll.poll([timeout]) method in the
select module. Various OSes throw an error with arbitrary negative
values.

https://bugs.python.org/issue31334

Always pass -1, or INFTIM where defined, to the poll() system call when a negative timeout is passed to the poll.poll([timeout]) method in the select module. Various OSes throw an error with arbitrary negative values.

the-knights-who-say-ni · 2017-09-03T22:38:41Z

Hello, and thanks for your contribution!

I'm a bot set up to make sure that the project can legally accept your contribution by verifying you have signed the PSF contributor agreement (CLA).

Unfortunately our records indicate you have not signed the CLA. For legal reasons we need you to sign this before we can look at your contribution. Please follow the steps outlined in the CPython devguide to rectify this issue.

Thanks again to your contribution and we look forward to looking at it!

volans- · 2017-09-06T21:44:01Z

For the record I've signed the CLA and my status has been updated on bugs.python.org.

berkerpeksag · 2017-09-07T11:50:59Z

Modules/selectmodule.c

+        if (ms >= 0)
+            deadline = _PyTime_GetMonotonicClock() + timeout;
+        else    /* On many OSes timeout must be -1 or INFTIM, issue 31334 */
+#ifdef INFTIM


Can't we do this in the following block?

if (timeout_obj == NULL || timeout_obj == Py_None) { timeout = -1; #ifdef INFTIM ms = INFTIM; #else ms = -1; #endif deadline = 0; }

Then we can just change

deadline = _PyTime_GetMonotonicClock() + timeout;

to

if (ms >= 0) { deadline = _PyTime_GetMonotonicClock() + timeout; }

I don't see how what you're proposing can fix the issue. When a user passes a negative timeout parameter it enters the else block at line 542, so ms will be negative and the system call will still fail. The problem is not the deadline but the ms variable.

But your comment made me realise that I should add the #ifdef to the if block too, for coherence and in case an OS will ever have INFTIM != -1. To be DRY I'll move the #ifdef after the else block.

berkerpeksag · 2017-09-07T11:50:59Z

Modules/selectmodule.c

    /* Check values for timeout */
    if (timeout_obj == NULL || timeout_obj == Py_None) {
        timeout = -1;
        ms = -1;
-        deadline = 0;   /* initialize to prevent gcc warning */


See my comment about INFTIM below. I think we don't need to move this line out of this if branch.

berkerpeksag · 2017-09-07T11:50:59Z

Misc/NEWS.d/next/Library/2017-09-04-00-22-31.bpo-31334.9WYRfi.rst

@@ -0,0 +1,2 @@
+Fix ``poll.poll([timeout])`` in the ``select`` module for arbitrary negative
+timeouts on all OSes where it can only be a non-negative integer or -1.


Please add "Patch by Riccardo Coccioli.".

Ok, thanks.

berkerpeksag · 2017-09-07T11:50:59Z

Lib/test/test_poll.py

@@ -130,7 +130,7 @@ def test_poll2(self):
        p = proc.stdout
        pollster = select.poll()
        pollster.register( p, select.POLLIN )
-        for tout in (0, 1000, 2000, 4000, 8000, 16000) + (-1,)*10:
+        for tout in (0, 1000, 2000, 4000, 8000, 16000, -1000) + (-1,)*10:


Style nitpick: I'd move -1000 to the beginning of the tuple to make it ordered.

berkerpeksag · 2017-09-07T11:50:59Z

Modules/selectmodule.c

    /* Check values for timeout */
    if (timeout_obj == NULL || timeout_obj == Py_None) {
        timeout = -1;
        ms = -1;
-        deadline = 0;   /* initialize to prevent gcc warning */
    }
    else {
        if (_PyTime_FromMillisecondsObject(&timeout, timeout_obj,


Another question: Can't we just set timeout to INFTIM or -1 if timeout_obj is less than -1 instead of messing with ms?

First of all thanks for the review.

I'm not getting why this would be better. The current checks against INT_MIN and INT_MAX are done with ms, so I followed that pattern. Also ms is the actual parameter passed to the system call and the one causing the issue right now, it seemed more logic to me to change directly this one.

bedevere-bot · 2017-09-07T11:51:01Z

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 didn't expect the Spanish Inquisition!. 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.

volans- · 2017-09-11T14:36:43Z

@berkerpeksag did you had any chance to look at my replies and changes?

volans- · 2017-09-11T14:36:52Z

I didn't expect the Spanish Inquisition!

bedevere-bot · 2017-09-11T14:36:54Z

Nobody expects the Spanish Inquisition!

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

volans- · 2017-10-09T21:29:58Z

@berkerpeksag do you have any additional feedback on this change?

berkerpeksag · 2017-10-10T01:11:27Z

@volans- sorry for the delay. I need to take a closer look at this (and a BSD to test it locally) Please ping me again if I don't get to it by the next week.

berkerpeksag · 2017-10-13T12:10:57Z

Modules/selectmodule.c

@@ -532,11 +532,12 @@ poll_poll(pollObject *self, PyObject *args)
        return NULL;
    }

+    deadline = 0;   /* initialize to prevent gcc warning */


We can set this in deadline initialization:

_PyTime_t deadline = 0;

And the following comment is not needed anymore:

/* initialize to prevent gcc warning */

berkerpeksag · 2017-10-13T12:10:57Z

Modules/selectmodule.c

@@ -532,11 +532,12 @@ poll_poll(pollObject *self, PyObject *args)
        return NULL;
    }

+    deadline = 0;   /* initialize to prevent gcc warning */
+
    /* Check values for timeout */
    if (timeout_obj == NULL || timeout_obj == Py_None) {


Looking at this again, we can get rid of this if branch if we move out default values to variable declarations:

_PyTime_t timeout = -1, ms = -1, deadline = 0;

Then we can get rid of the following comment too:

/* Check values for timeout */

berkerpeksag · 2017-10-13T12:10:57Z

Modules/selectmodule.c

@@ -554,9 +555,17 @@ poll_poll(pollObject *self, PyObject *args)
            return NULL;
        }

-        deadline = _PyTime_GetMonotonicClock() + timeout;
+        if (timeout >= 0)


Style nit: Per PEP 7, we now use the following style in new C code:

if (spam) { return 42; }

instead of

if (spam) return 42;

berkerpeksag · 2017-10-13T12:10:57Z

Modules/selectmodule.c

    }

+    if (ms < 0) /* On many OSes timeout must be INFTIM or -1, issue 31334 */


Style nit: Same as above.

Can we make this comment clearer? For example, it would be nice to mention this is the case if negative timeout is less than -1 on some OSes.

Also, I'd replace "many" with "some" or "some BSD OSes" since this is the first time this issue has been reported.

berkerpeksag · 2017-10-13T12:10:57Z

Lib/test/test_poll.py

@@ -130,7 +130,7 @@ def test_poll2(self):
        p = proc.stdout
        pollster = select.poll()
        pollster.register( p, select.POLLIN )
-        for tout in (0, 1000, 2000, 4000, 8000, 16000) + (-1,)*10:
+        for tout in (-1000, 0, 1000, 2000, 4000, 8000, 16000) + (-1,)*10:


This is not directly related to this ticket, but I realized that None is not tested in test_poll. Could you also add None while we are here?

bedevere-bot · 2017-10-13T12:10:59Z

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.

volans- · 2017-10-13T16:58:45Z

I have made the requested changes; please review again.

@berkerpeksag : Thank you very much for the review and the suggestions to simplify the code. My first patch was as less impactful as possible because I've found in other communities harder to get patches approved if they were also refactoring existing code. I'm happy to see a different approach here.

bedevere-bot · 2017-10-13T16:58:47Z

Thanks for making the requested changes!

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

berkerpeksag

This looks pretty good to me, thank you! I just left a comment about the ms < 0 check.

I will ask for another pair of eyes from another core developer then merge it.

berkerpeksag · 2017-10-14T03:08:48Z

Modules/selectmodule.c

+    /* On some OSes, typically BSD-based ones, the timeout parameter of the
+       poll() syscall, when negative, must be exactly INFTIM, where defined,
+       or -1. See issue 31334. */
+    if (ms < 0) {


Nitpick: Can we change this to ms < -1? Since we set ms = -1 in line 528, we shouldn't get any "invalid argument" exception if we don't pass a timeout value to p.poll().

@berkerpeksag what if a OS defines INFTIM different from -1? This ensures that we set ms to INFTIM in that case too. I can change it to ms <= -1 if you think it's more readable.

On a side note, the documentation says only that the timeout parameter is in milliseconds, but doesn't say it must be an integer. I've noticed that passing a float works too and in the special case of passing a float in the range (-1, 0), that is converted to 0 by ms = _PyTime_AsMilliseconds(timeout, _PyTime_ROUND_CEILING);. I've tested that this behaviour does not change with my patch, but is arguably the desired one.

I concur with @volans-. It is better to keep ms < 0.

As for rounding problem, I just have opened bpo-31786. I didn't mentioned it here because seems this change doesn't change this behavior.

@volans- fair enough, thanks for the explanation. I think ms < 0 can stay as is, no need to change it to ms <= -1.

miss-islington · 2017-10-17T19:45:09Z

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

vstinner · 2017-10-17T19:45:37Z

Wow, 3 approvals from 3 different core developers. Well done @volans- ! Thanks for this nice bug fix ;-)

miss-islington · 2017-10-17T19:46:14Z

Sorry, @volans- and @Haypo, I could not cleanly backport this to 3.6 due to a conflict.
Please backport using cherry_picker on command line.
cherry_picker 6cfa927ceb931ad968b5b03e4a2bffb64a8a0604 3.6

miss-islington · 2017-10-17T19:47:11Z

Sorry, @volans- and @Haypo, I could not cleanly backport this to 2.7 due to a conflict.
Please backport using cherry_picker on command line.
cherry_picker 6cfa927ceb931ad968b5b03e4a2bffb64a8a0604 2.7

vstinner · 2017-10-17T19:54:30Z

I removed the "needs backport" labels to prevent the bot to try again to create backport fixes.

volans- · 2017-10-17T19:55:45Z

@Haypo thank you all for the review, comments and for approving/merging it.

Should I create manual PRs for the backports given that the bot seems to have some issues today?

serhiy-storchaka · 2017-10-17T19:56:59Z

First #4003 should be backported.

miss-islington · 2017-10-18T08:10:08Z

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

miss-islington · 2017-10-18T08:10:10Z

Sorry, @volans- and @Haypo, I could not cleanly backport this to 3.6 due to a conflict.
Please backport using cherry_picker on command line.
cherry_picker 6cfa927ceb931ad968b5b03e4a2bffb64a8a0604 3.6

miss-islington · 2017-10-18T08:14:04Z

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

Always pass -1, or INFTIM where defined, to the poll() system call when a negative timeout is passed to the poll.poll([timeout]) method in the select module. Various OSes throw an error with arbitrary negative values. (cherry picked from commit 6cfa927)

bedevere-bot · 2017-10-18T08:14:34Z

GH-4033 is a backport of this pull request to the 3.6 branch.

miss-islington · 2017-10-18T08:31:44Z

Thanks @volans- for the PR, and @Haypo for merging it 🌮🎉.. I'm working now to backport this PR to: 2.7.
🐍🍒⛏🤖 I'm not a witch! I'm not a witch!

miss-islington · 2017-10-18T08:32:49Z

Sorry, @volans- and @Haypo, I could not cleanly backport this to 2.7 due to a conflict.
Please backport using cherry_picker on command line.
cherry_picker 6cfa927ceb931ad968b5b03e4a2bffb64a8a0604 2.7

Always pass -1, or INFTIM where defined, to the poll() system call when a negative timeout is passed to the poll.poll([timeout]) method in the select module. Various OSes throw an error with arbitrary negative values.. (cherry picked from commit 6cfa927)

bedevere-bot · 2017-10-18T11:24:06Z

GH-4034 is a backport of this pull request to the 2.7 branch.

volans- · 2017-10-18T11:31:58Z

@serhiy-storchaka I've ported this to the 2.7 branch with GH-4034, it's a smaller patch given the code in the 2.7 branch.

Always pass -1, or INFTIM where defined, to the poll() system call when a negative timeout is passed to the poll.poll([timeout]) method in the select module. Various OSes throw an error with arbitrary negative values.. (cherry picked from commit 6cfa927)

Always pass -1, or INFTIM where defined, to the poll() system call when a negative timeout is passed to the poll.poll([timeout]) method in the select module. Various OSes throw an error with arbitrary negative values. (cherry picked from commit 6cfa927)

serhiy-storchaka · 2017-10-18T12:09:00Z

Thanks @volans-!

bpo-31334: Fix timeout in select.poll.poll()

7de82e8

Always pass -1, or INFTIM where defined, to the poll() system call when a negative timeout is passed to the poll.poll([timeout]) method in the select module. Various OSes throw an error with arbitrary negative values.

the-knights-who-say-ni added the CLA not signed label Sep 3, 2017

bedevere-bot added the awaiting review label Sep 3, 2017

volans- mentioned this pull request Sep 3, 2017

Python 3 on macOS/BSDs: Clush and library fails with no timeout cea-hpc/clustershell#353

Closed

berkerpeksag removed the CLA not signed label Sep 7, 2017

the-knights-who-say-ni added the CLA signed label Sep 7, 2017

berkerpeksag requested changes Sep 7, 2017

View changes

bedevere-bot removed the awaiting review label Sep 7, 2017

bedevere-bot added the awaiting changes label Sep 7, 2017

bpo-31334: Address some PR comments and simplify

3230460

bedevere-bot added awaiting change review and removed awaiting changes labels Sep 11, 2017

berkerpeksag requested changes Oct 13, 2017

View changes

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

bpo-31334: Address PR comments

58e0300

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

berkerpeksag approved these changes Oct 14, 2017

View changes

bedevere-bot removed the awaiting merge label Oct 17, 2017

vstinner removed needs backport to 2.7 labels Oct 17, 2017

serhiy-storchaka added the needs backport to 3.6 label Oct 18, 2017

serhiy-storchaka added needs backport to 3.6 and removed needs backport to 3.6 labels Oct 18, 2017

bedevere-bot removed the needs backport to 3.6 label Oct 18, 2017

serhiy-storchaka added the needs backport to 2.7 label Oct 18, 2017

bedevere-bot removed the needs backport to 2.7 label Oct 18, 2017

volans- deleted the fix-select-poll branch Oct 18, 2017

		@@ -0,0 +1,2 @@
		Fix ``poll.poll([timeout])`` in the ``select`` module for arbitrary negative
		timeouts on all OSes where it can only be a non-negative integer or -1.

		}

		if (ms < 0) /* On many OSes timeout must be INFTIM or -1, issue 31334 */

bpo-31334: Fix timeout in select.poll.poll() #3277

bpo-31334: Fix timeout in select.poll.poll() #3277

Conversation

volans- commented Sep 3, 2017 • edited by bedevere-bot

the-knights-who-say-ni commented Sep 3, 2017

volans- commented Sep 6, 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

bedevere-bot commented Sep 7, 2017

volans- commented Sep 11, 2017

volans- commented Sep 11, 2017

bedevere-bot commented Sep 11, 2017

volans- commented Oct 9, 2017

berkerpeksag commented Oct 10, 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

bedevere-bot commented Oct 13, 2017

volans- commented Oct 13, 2017

bedevere-bot commented Oct 13, 2017

berkerpeksag left a comment • edited

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

vstinner commented Oct 17, 2017

miss-islington commented Oct 17, 2017

miss-islington commented Oct 17, 2017

vstinner commented Oct 17, 2017

volans- commented Oct 17, 2017

serhiy-storchaka commented Oct 17, 2017

miss-islington commented Oct 18, 2017

miss-islington commented Oct 18, 2017

miss-islington commented Oct 18, 2017

bedevere-bot commented Oct 18, 2017

miss-islington commented Oct 18, 2017

miss-islington commented Oct 18, 2017

bedevere-bot commented Oct 18, 2017

volans- commented Oct 18, 2017

serhiy-storchaka commented Oct 18, 2017

volans- commented Sep 3, 2017 •

edited by bedevere-bot

berkerpeksag left a comment •

edited