Fix periodic timer with zero interval for ExtEvLoop and legacy ExtLibevLoop

[lucasnetau]

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

[SimonFrings]

@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?

[lucasnetau]

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.

[SimonFrings]

@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 $i once)

The thing is: If I add an 0 as $interval for my periodicTimer and increase a value every time (like in your test) I would expect that in an ideal execution this value would hit a number near infinity (theoretically). What I want to say with this is that you can't say what number exactly will come out in the end, you can only guess the range and it depends on the system you're executing the code. This makes testing this actually pretty hard and unstable.

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 ExtEvLoop but failed for ExtLibevLoop (because old PHP uses ExtLibev). That means you have to add the same fix into the addPeriodicTimer() function inside ExtLibevLoop.php.

I hope this helps 👍

[lucasnetau]

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.

[SimonFrings]

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?

[lucasnetau]

@SimonFrings, sure, commits have been squashed

[SimonFrings]

Awesome 🔥

[clue]

@lucasnetau Thank you very much for this high-quality PR, changes LGTM!

Keep it up! 👍

[WyriHaximus]

Good catch, thank you for taking the same to PR a fix with test 👍