StreamSelectLoop: Int overflow for big timer intervals by ivkalita · Pull Request #96 · reactphp/event-loop

ivkalita · 2017-04-05T09:20:15Z

The problem

As described here, current StreamSelectLoop implementation restricts using time intervals more than PHP_INT_MAX microseconds. For 32-bit systems it's only 2148 seconds.

Suggested solution

Change StreamSelectLoop $timeout type from int (in microseconds) to float (in seconds);
Replace usleep with time_nanosleep, cause usleep takes int argument in microseconds, and we can't keep more than PHP_INT_MAX microseconds. But time_nanosleep takes 2 arguments: int amount of seconds and int amount of nanoseconds, that allows StreamSelectLoop to handle PHP_INT_MAX seconds intervals;
Extract stream_select and time_nanosleep methods calls from StreamSelectLoop::streamSelect(...) to test timeout values.

jsor · 2017-04-05T18:33:39Z

src/StreamSelectLoop.php

+    protected function sleep($seconds, $nanoseconds = 0)
+    {
+        if ($seconds > 0 || $nanoseconds > 0) {
+            time_nanosleep($seconds, $nanoseconds);


I'd like to see explicit handling of $secondsor $nanosecondsbeing null here.

jsor · 2017-04-05T18:33:47Z

src/StreamSelectLoop.php

+        if ($time == PHP_INT_MAX) {
+            return PHP_INT_MAX;
+        }
+


Minor nitpicking: for consistency with the other 2 methods, i'd like to an early return for $time === null:

if ($time === null) { return null; } // ... return intval(floor($time));

…th time_nanosleep.

jsor · 2017-04-10T17:59:31Z

src/StreamSelectLoop.php

+     */
+    private static function getMicroseconds($time)
+    {
+        if ($time === null) {


Should probably return 0. stream_select accepts null only for the $seconds parameter.

jsor · 2017-04-10T17:59:46Z

src/StreamSelectLoop.php

+     */
+    private static function getNanoseconds($time)
+    {
+        if ($time === null) {


Should probably return 0, see above.

jsor · 2017-04-10T18:01:06Z

src/StreamSelectLoop.php

+     * @param array $read
+     * @param array $write
+     * @param array $except
+     * @param int|null $seconds


Should only accept int, see also above.

ivkalita · 2017-04-10T18:50:05Z

@jsor, thank you for the review. I think that previous implementation was not explicit, cause $timeout checking for null value was duplicated in 4 places (getSeconds, getMicroseconds, getNanoseconds and sleep). I changed this behavior in 4347fa3: I removed all unnecessary null checks and moved them inside streamSelect method. Also, I fixed the things you pointed ;)

jsor · 2017-04-11T09:17:02Z

src/StreamSelectLoop.php

    protected function streamSelect(array &$read, array &$write, $timeout)
    {
+        $seconds = $timeout === null ? null : self::getSeconds($timeout);
+        $microseconds = $timeout === null ? 0 : self::getMicroseconds($timeout);


Calculation of $microseconds and $nanoseconds can probably be moved into the ifs so they are only calculated when needed.

@jsor, done in bb81383 😃

jsor · 2017-04-11T09:17:39Z

src/StreamSelectLoop.php

+         * (float)PHP_INT_MAX == PHP_INT_MAX => true
+         * (int)(float)PHP_INT_MAX == PHP_INT_MAX => false
+         * (int)(float)PHP_INT_MAX == PHP_INT_MIN => true
+         */


Strict comparision (===)?

@jsor, $time is float, so strict comparison will always return false.

(float) PHP_INT_MAX === PHP_INT_MAX; //is false (float) PHP_INT_MAX == PHP_INT_MAX; //is true

👍 (maybe add a comment that loose comparision is intentional)

@jsor I added more description about this tricky if in 3c8a5d6 😄

…round.

…n into the "if"s.

jsor

LGTM

WyriHaximus

LGTM

jsor · 2017-04-25T18:04:24Z

ping @clue

ivkalita · 2017-04-25T19:30:33Z

@clue, can you please describe, why did you mark this PR as BC Break? Imo, only internal representation of time changed and no public interfaces modified...

clue

Thanks for filing this PR! These are some high quality changes and I'd actually like to get this in to solve this rare issue 👍

The issue only affects 32 bit systems, yet this PR proposes some major changes to the whole StreamSelectLoop, which contains some very performance-critical code.

Does it make sense to perform some benchmarks to check its impact?

clue · 2017-04-26T08:08:24Z

src/StreamSelectLoop.php

     *
     * @param array        &$read   An array of read streams to select upon.
     * @param array        &$write  An array of write streams to select upon.
-     * @param integer|null $timeout Activity timeout in microseconds, or null to wait forever.


This is a rather subtle BC break as consumers could possibly rely on this protected API. Not saying this is an issue with your proposed change, it's more that we ~~screwed up~~ could have done better with defining our external API.

Add StreamSelectLoop time conversion methods.

9021dc8

ivkalita force-pushed the stream-select-loop-int-overflow branch from 31a613a to a4bf806 Compare April 5, 2017 11:27

jsor reviewed Apr 5, 2017

View reviewed changes

ivkalita added 3 commits April 6, 2017 00:06

Change StreamSelectLoop to keep timeout in seconds, replace usleep wi…

8279d9b

…th time_nanosleep.

Fix StreamSelectLoopTest::testSmallTimerInterval to expect nanoseconds.

4400ede

Add StreamSelectLoop test for PHP_INT_MAX seconds timer interval.

08c709b

ivkalita force-pushed the stream-select-loop-int-overflow branch from a4bf806 to 08c709b Compare April 5, 2017 21:07

WyriHaximus requested review from WyriHaximus, clue and jsor April 7, 2017 19:28

jsor requested changes Apr 10, 2017

View reviewed changes

Change StreamSelectLoop::streamSelect null-timeout handling.

4347fa3

ivkalita force-pushed the stream-select-loop-int-overflow branch from c78624d to 4347fa3 Compare April 10, 2017 18:59

jsor reviewed Apr 11, 2017

View reviewed changes

ivkalita added 2 commits April 11, 2017 12:29

Add explanation about StreamSelectLoop::getSeconds int overflow worka…

3c8a5d6

…round.

Move StreamSelectLoop::streamSelect micro- and nanoseconds calculatio…

bb81383

…n into the "if"s.

jsor approved these changes Apr 11, 2017

View reviewed changes

WyriHaximus approved these changes Apr 11, 2017

View reviewed changes

clue added BC break bug labels Apr 25, 2017

clue reviewed Apr 26, 2017

View reviewed changes

clue mentioned this pull request Dec 4, 2017

Ensure large timer interval does not overflow on 32bit systems #132

Merged

jsor closed this in #132 Dec 4, 2017

Uh oh!

Conversation

ivkalita commented Apr 5, 2017

The problem

Suggested solution

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ivkalita commented Apr 10, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jsor left a comment

Choose a reason for hiding this comment

Uh oh!

WyriHaximus left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jsor commented Apr 25, 2017

Uh oh!

ivkalita commented Apr 25, 2017

Uh oh!

clue left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

ivkalita commented Apr 10, 2017 •

edited

Loading

WyriHaximus left a comment •

edited

Loading