WyriHaximus
0-second timer issue and infinite recursion bug in Scheduler class
Hi Cees-Jan,
I think I've stumbled across a couple of issues in your Scheduler class.
In this area in align() at line 82:
if ($currentSecond >= ONE && $currentSecond <= self::TIER_SLOW) {
$this->timer = Loop::addTimer(self::TIER_SLOW - $currentSecond, …
}
If the current second is 55 at the time of evaluation, then that feeds 0 into addTimer, which makes the timer fire immediately. In my project this results in a sudden rapid increase in memory consumption as the align() method ends up being invoked many (hundreds?) of times in quick succession. This issue became readily apparent when using the ExtUVLoop event loop adapter - I haven't noticed it with StreamSelectLoop.
I tried adjusting the boundaries in my local copy, so I used this instead:
if ($currentSecond >= ONE && $currentSecond < self::TIER_SLOW)...
But doing this introduces a secondary issue, an infinite recursion bug at the 55 second mark...
The problem is that tick() can call align() again if the time has "drifted". So you end up with align() -> tick() -> align() -> tick() … over and over until Xdebug freaks out with a stack overflow.
I think something additional needs to be added to break this recursion at line 108... perhaps replacing:
$this->tick();
... with:
Loop::futureTick(function (): void {
$this->tick();
});
Maybe I'm off track here :smiley: - I figured I'd raise this suggestion for consideration anyway.
Hey William,
Thanks for bringing this to my attention, will have a look, hopefully, tomorrow to prove this in tests and then fix it 👍 .
Maybe I'm off track here 😃 - I figured I'd raise this suggestion for consideration anyway.
Maybe, but let's validate this to be sure
@williameggers Thought it was going to be simple proofing this in unit tests. Can you try out the branch and see if this resolves the problem for you locally? Going to end up mocking both a clock and event loop here so I can test several scenarios 😅
@WyriHaximus It’s definitely an improvement!
I added some extra print statements in the reactphp-cron code within the align() and tick() methods to log the invocation count and current memory usage.
Running on v5.1.0 with PHP 8.2, I observed that align() was called more than 18,619 times, with peak memory usage reaching 87.47 MiB. The cron library is being used in a personal project of mine.
https://github.com/user-attachments/assets/66b5e76e-b31f-4351-9571-8321e3357af9
The test output below was run using the fix-scheduler-recursion branch on PHP 8.4, which shows significantly improved behavior. The invocation count remains at a sane level after several test iterations.
https://github.com/user-attachments/assets/89d23e6e-bc53-4273-b872-645f52fb5dea
Sorry for the delay getting back to you. Lots of things on the go at the moment.
@WyriHaximus It’s definitely an improvement!
Just made it less trying to be smart, and that is paying off.
Running on v5.1.0 with PHP 8.2, I observed that align() was called more than 18,619 times, with peak memory usage reaching 87.47 MiB. The cron library is being used in a personal project of mine.
existing-code-php8.2-reactphp-cron-v5.1.0.mov The test output below was run using the fix-scheduler-recursion branch on PHP 8.4, which shows significantly improved behavior. The invocation count remains at a sane level after several test iterations.
revised-code-php8.4-reactphp-cron-fix-scheduler-recursion.mov
That's an awesome improvement to see. I just merged #105 to test the improvements for #104.
Sorry for the delay getting back to you. Lots of things on the go at the moment.
No worries, life happens.
@williameggers Just released 5.3.0 with the fix for this. There are some more improvements I want to look into, but this should fix the biggest immidiate issue
@WyriHaximus thanks for the update - awesome work! -- I'll apply the changes to the affected project shortly.