Make StreamRetryPolicy.exponential() be actually exponential - #79
Conversation
Delays accumulated as step * n(n+1)/2 instead of doubling. Compute the delay from the retry index as step * 2^(n-1), guarding the shift against overflow at high retry counts.
PR checklist ✅All required conditions are satisfied:
🎉 Great job! This PR is ready for review. |
|
Important
This repository does not receive automatic reviews because it has fewer than 10 stars. ⚙️ Run configurationConfiguration used: Repository UI Review profile: CHILL Plan: Advanced Run ID: Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
aleksandar-apostolov
changed the title
StreamRetryPolicy.exponential() be actually exponential
andremion
left a comment
andremion
left a comment
There was a problem hiding this comment.
LGTM. Two nits inline, both optional.
One thing on the description: the behavior note is backwards. New delays are shorter up to retry 4 and only longer from retry 5, so feeds retries faster now, not slower (750 ms total before giving up instead of 1000 ms). Mind correcting that line?
Not for this PR, but two follow-ups if you want them: the README exponential example still uses StreamRetryPolicy.Exponential and maxDelayMillis, which don't exist, and stream-video-android has a copy of this class with the old formula plus a KDoc note saying it matches core deliberately. Happy to open tickets.
The branch guard already bounds the shifted value, so the coerceIn could never fire. Add a processor-level test that pins elapsed scheduler time across five attempts, where the accumulation bug actually lived.
|
Thanks — all three were right. |
|
2 participants
Goal
Fixes AND-1507 —
StreamRetryPolicy.exponential()produced quadratic growth, not exponential.nextBackOffDelayFunctionwasprev + retry * backoffStepMillis, andStreamRetryProcessorImplfeeds the previous delay back in, so delays accumulated asstep * n(n+1)/2. With the default 250 ms step that is250, 750, 1500, 2500, 3750where both the KDoc and the README promise250, 500, 1000, 2000.Implementation
exponential()now derives the delay from the retry index alone:backoffStepMillis * 2^(retryIndex - 1), clamped to[backoffStepMillis, maxBackoffMillis].maxBackoffMillisdirectly, so high retry counts saturate instead of wrapping negative.linear()andfixed()are unchanged.Behavior change for consumers of
@StreamInternalApi: delays are shorter through retry 4 and only longer from retry 5, so callers retry faster than before in the common case.stream-feeds-androidusesexponential(maxRetries = 3)inFeedOwnValuesRepositoryImplandFeedWatchHandler— that sleeps twice before giving up, so its total wait drops from 1000 ms to 750 ms.Testing
./gradlew :stream-android-core:testDebugUnitTest— 743 tests, 1 failure inStreamCompositeEventSerializationImplTest, confirmed pre-existing on a cleandevelopworktree at e80f8a4.StreamRetryPolicyTest(36 tests, all green) updated and extended:exponential backoff delay calculation increases exponentiallynow asserts 100/200/400/800 for a 100 ms step.exponential backoff delay does not overflow on very high retry countscovers retry 64 andInt.MAX_VALUE.exponential backoff delay ignores the previous delaypins the new index-based contract.