Normalise the strategy's wait time to int before jitter - #23
Open
noahott wants to merge 1 commit into
Open
Conversation
A strategy may be any callable — buildStrategy() returns callables untouched and testClosureStrategy covers that — but nothing requires one to return an int. cap() is type-preserving, so a float-returning strategy reaches mt_rand() unchanged, and mt_rand() takes int parameters. Two symptoms, depending on magnitude: - An in-range float deprecates on PHP 8.1+: "Implicit conversion from float 15.667 to int loses precision". With jitter disabled the float is returned as-is, so getWaitTime() also contradicts its own @return int. - A float beyond PHP_INT_MAX throws outright: "mt_rand(): Argument stechstudio#2 ($max) must be of type int, float given". Reachable whenever no wait cap is set, since cap() then returns the strategy value untouched. The bundled strategies all cast to int already, which is why this has gone unnoticed. min(PHP_INT_MAX, ...) before the cast keeps the large-float case in range rather than relying on an undefined float-to-int conversion, and casting before jitter() means both the jittered and non-jittered paths honour the documented int return.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Fixes #22.
A strategy may be any callable, and nothing requires it to return an int.
cap()is type-preserving, so a float-returning strategy reachesmt_rand()unchanged — andmt_rand()takes int parameters.Three symptoms, all from the same line:
15.667Deprecated: Implicit conversion from float 15.667 to int loses precision(PHP 8.1+)15.667float(15.667), contradictinggetWaitTime()'s@return int1e20TypeError: mt_rand(): Argument #2 ($max) must be of type int, float givenThe last one is a hard crash, and it is reachable whenever no wait cap is set —
cap()returns the strategy value untouched in that case, sinceis_int(null)is false.The bundled strategies all cast to int already, which is why none of this shows up in normal use.
The change
Two deliberate details:
cap(), not before, so the negative-overflow guard added in Added protection against overflows in the wait time calculation. #20 still runs first.testWaitCapOverflowstill passes.min(PHP_INT_MAX, ...)rather than a bare cast. Casting a float abovePHP_INT_MAXis undefined in PHP; clamping first makes the1e20case defined instead of platform-dependent. If you'd rather keep the diff minimal,(int) $this->cap($waitTime)fixes the deprecation and the TypeError too — it just leaves the out-of-range value undefined rather than clamped. Happy to switch.Casting before
jitter()rather than after also means the non-jittered path returns an int, matching the docblock.Tests
Three added to
BackoffTest, following the existing style (no attributes,$this->assertX()):testFloatReturningStrategyProducesIntegerWaitTime— jitter off, assertsassertIsInt(). This is the one that needs no error-handler juggling, so it's the durable regression test.testFloatReturningStrategyDoesNotTriggerDeprecationUnderJitter— installs a temporaryE_DEPRECATEDhandler, restores it in afinally. Worth notingphpunit.xmlsets neitherfailOnDeprecationnorfailOnWarning, so without an explicit handler a test here cannot observe the deprecation at all.testFloatStrategyBeyondIntMaxDoesNotThrow— theTypeErrorcase.Each fails on
masterand passes with the patch. Onbaab60f+ tests only:With the patch:
Verified on PHP 8.3.31. Your CI matrix is PHP 8.2, where the deprecation also applies (8.1+).