Fix periodic timer with zero interval for ExtEvLoop and legacy ExtLibevLoop
#243
Conversation
|
@lucasnetau thanks for coming up with a solution to #236 👍 This behavior isn't covered by our testsuite yet, can you add some tests to assure that your changes are working as expected? |
|
Hi @SimonFrings, I've added a test, it fails locally with the current code and passes with this PR. However it is failing with the CI tests. Any help would be appreciated. |
|
@lucasnetau I talked with @clue about this topic for like an hour ^^ The test looks fine but I think GitHub actions can't handle the speed of this interval (maybe only executes it once every millisecond, thus only increasing The thing is: If I add an 0 as It's definitely the wrong behavior when passing a 0 as an interval and only getting one increase out. We may can't guess the exact number that will come out but we know it has to be higher than 1. With that said I wrote a little test myself: public function testAddPeriodicTimerWithZeroIntervalWillExecuteCallbackFunctionAtLeastTwice()
{
$loop = $this->createLoop();
$i = 0;
$periodic = $loop->addPeriodicTimer(0, function ($periodic) use (&$i, $loop) {
++$i;
if ($i === 2) {
$loop->cancelTimer($periodic);
}
});
$loop->run();
$this->assertEquals(2, $i);
}I tested this and it worked for I hope this helps 👍 |
|
Thanks @SimonFrings @clue, I've added that test with an additional 2 second timeout timer. In my local testing the original ExtEvLoop code would cause the test loop to never exit run(), I'm assuming because while the EvTimer will never fire again the Loop still has it scheduled. I have also changed the deprecated ExtLibevLoop with the same getInterval() fix. There is an upper bounds to how many times the timer can fire. The highest resolution is 1 microsecond after this patch, so 1 million times a second. In reality we don't get anywhere near that. |
|
Nice work @lucasnetau, looks great so far 👍 I also thought about adding a second timer that cancels the periodic one if a certain amount of time has passed, so nice to see that we're on the same page ^^ The last thing before I approve this PR is that I don't think it's necessary to have 5 commits for this, could you squash them together into one? |
… should take the adjusted interval value from Timer. This takes into account any MIN_INTERVAL bounds are applied when initialising the Timer
|
@SimonFrings, sure, commits have been squashed |
ExtEvLoop and legacy ExtLibevLoop
There was a problem hiding this comment.
@lucasnetau Thank you very much for this high-quality PR, changes LGTM! 
Keep it up! 👍
When initialising a periodic EvTimer, the after and repeat parameters should take the adjusted interval value from Timer. This takes into account any MIN_INTERVAL bounds are applied when initialising the Timer.
Fixes #236