8000 StreamSelectLoop: Timers with very small intervals do not work correctly · Issue #48 · reactphp/event-loop · GitHub
[go: up one dir, main page]
Skip to content

StreamSelectLoop: Timers with very small intervals do not work correctly #48

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.

Sign up for GitHub

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

Jump to bottom
Closed
joshdifabio opened this issue Apr 21, 2016 · 7 comments
Closed

StreamSelectLoop: Timers with very small intervals do not work correctly #48

joshdifabio opened this issue Apr 21, 2016 · 7 comments
Labels
bug easy pick

Comments

@joshdifabio
Copy link
joshdifabio commented Apr 21, 2016 •
edited
Loading

Within StreamSelectLoop::run() is the following code:

} elseif ($scheduledAt = $this->timers->getFirst()) { // float
    $timeout = $scheduledAt - $this->timers->getTime(); // float - float
    if ($timeout < 0) {
        $timeout = 0;
    } else {
        $timeout *= self::MICROSECONDS_PER_SECOND; // float * int
    }
}

In the case of a periodic timer with an interval equal to Timer::MIN_INTERVAL (0.000001), the above code, in conjunction with the Timers class, essentially does something like the following:

$currentTime = microtime(true);
$ourTimerInterval = 0.000001;
$nextTimerScheduledAt = $currentTime + $ourTimerInterval;
// $timeout is for stream_select()/usleep() call
$timeout = $nextTimerScheduledAt - $currentTime; // This is NOT equal to 0.000001 due to float error [1]
$timeout *= 1000000; // This looks like it should be (int)1, but it is actually approximately (float)0.95

$timeout is then passed to stream_select() and usleep(), both of which expect int and so cast (float)0.95 to (int)0, not waiting at all when the timer was intended to delay for 1 microsecond. Furthermore, the fix in #37 is bypassed when this issue occurs as (float)0.95 casts to boolean true.

A quick fix would be to apply rounding to $timeout *= self::MICROSECONDS_PER_SECOND;.

[1] https://3v4l.org/FvioX
@joshdifabio joshdifabio mentioned this issue Apr 21, 2016
Closed
@enumag
Copy link
enumag commented Oct 31, 2016

I'm having trouble with periodic timers with period lesser than 1 (1.0 is ok, 0.999 is not) and StreamSelectLoop (didn't try other loops yet). I'm stopping the loop after 1.5 second. With period set to 1 the timer is executed once. With period set to 0.999 it's executed several thousands times.

The suggested fix did not help at all.

@kelunik
Copy link
Contributor
kelunik commented Nov 16, 2016

Having a timer executed every microsecond is the first mistake. What's the purpose of that timer?

Apart from that I'm glad that we have int for async-interop/event-loop instead of floats.

@enumag
Copy link
enumag commented Nov 16, 2016 •
edited
Loading

I don't want timer executed every microsecond, just every half-second or so - for faster tests execution. But instead of being executed twice in one second it is executed like 3000 times.

Also how can I use the async-interop/event-loop library? There is no non-abstract driver implementation.

@kelunik
Copy link
Contributor
kelunik commented Nov 16, 2016

@enumag Amp will support it in version 2.0.0, currently in development. @WyriHaximus started to build an adapter for React as well.

@clue
Copy link
Member
clue commented Nov 16, 2016

@enumag Have you tried the suggested fix in #49 and this did not resolve your issue?

Can you post a gist that helps us reproduce the issue you're seeing?

@ivkalita
Copy link
Contributor
ivkalita commented Mar 20, 2017 •
edited
Loading

@clue, hi! Do you have any idea about how to test this thing? I stuck with the same problem as @joshdifabio in #49 – it's hard to test that this feature is working. We need to run timer with very-very small interval and loop's time cost itself is much more expensive and takes much more time than usleep(1).

@ivkalita ivkalita mentioned this issue Apr 3, 2017
Merged
@jsor
Copy link
Member
jsor commented Apr 4, 2017

Fixed in #93

@jsor jsor closed this as completed Apr 4, 2017
@wafray wafray mentioned this issue Apr 24, 2017
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
bug easy pick
Projects
None yet
Milestone
No milestone
Development

No branches or pull requests
6 participants
@jsor @enumag @clue @kelunik @ivkalita @joshdifabio
0