chore: collapse duplicated helpers in recovery-aware pipeline + retry#194
Merged
Conversation
Three call sites independently re-implemented the same interrupt-aware failure wrapper — wrap a throwable in ResponseOutcome.Failure, restoring the thread interrupt flag first when it is an InterruptedException. Hoist it to a single module-internal failureOf() next to ResponseOutcome (the type it constructs), and route ExecutionPipeline, ResponsePipeline, and RetryStep.executeOnce through it. RetryStep.executeOnce folds its two catch arms into one as a result. RetryStep.attempt() is deliberately left untouched: it rethrows the wrapped throwable to its caller, so an InterruptedException propagates as itself and carries the cancellation directly — pre-restoring the flag there is unnecessary. failureOf() exists for the opposite path, where the failure is buried in an outcome that flows on through the pipeline and may be swallowed or recovered, so the flag must be restored to preserve cancellation. Also collapse three more local duplications in the retry package: - RetrySettings' totalTimeout/initialDelay/maxDelay setters shared the same non-negative + nanosecond-representability checks; extract a requireRepresentable(name, value) helper that names the offending field. Rejection messages are preserved verbatim. - RetryAfterParser's retry-after-ms and x-ms-retry-after-ms branches were verbatim copies; iterate the two header names instead. Array order keeps retry-after-ms ahead of x-ms-retry-after-ms, preserving precedence. - resolveScheduler() collapses to an elvis expression. All changes are behavior-preserving. failureOf and requireRepresentable are internal/private, so the public API surface is unchanged.
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.
What
Collapses four small, self-contained duplications that crept into the recovery-aware pipeline
primitives (
pipeline) and the retry step (pipeline/step/retry) as the two stacks grew. None ofthese change behavior — each removes copy-pasted logic in favor of a single definition.
1. One interrupt-aware failure wrapper (×3 → 1)
ExecutionPipeline,ResponsePipeline, andRetryStep.executeOnceeach independently re-implementedthe same step: wrap a throwable in
ResponseOutcome.Failure, restoring the thread interrupt flag firstwhen it is an
InterruptedException. This is now a single module-internalfailureOf(t)placed next toResponseOutcome(the type it constructs); all three call sites route through it, andRetryStep.executeOncefolds its twocatcharms into one.The cancellation contract is preserved exactly.
RetryStep.attempt()is deliberately not routedthrough
failureOf: it rethrows the wrapped throwable to its caller, so anInterruptedExceptionpropagates as itself and carries the cancellation directly — pre-restoring the flag there is
unnecessary.
failureOfexists for the opposite path, where the failure is buried in an outcome thatflows on through the pipeline and may be swallowed or recovered, so the flag must be restored.
2. One
Durationvalidator for the threeDurationsettersRetrySettings'totalTimeout,initialDelay, andmaxDelaysetters carried the same tworequirechecks (non-negative + nanosecond-representable). Extracted into a private
requireRepresentable(name, value)that names the offending field; the rejection messages are carried verbatim (so a rejectedinitialDelaystill readsinitialDelay must be representable in nanoseconds (≤ ~292 years); got …).The
delayMultipliersetter validates aDoubleand is left as-is.3. Loop over the millisecond-variant headers
RetryAfterParser'sretry-after-msandx-ms-retry-after-msbranches were verbatim copies. They nowiterate the two header names. Array order keeps
retry-after-msahead ofx-ms-retry-after-ms, and thefirst usable value still wins, so header precedence is unchanged.
4. Elvis for the scheduler fallback
RetryStep.resolveScheduler()collapses tosettings.scheduler ?: DEFAULT_SCHEDULER. Theby lazydefault is still only dereferenced when no caller scheduler is supplied, so the once-per-VM lazy-init
timing is preserved.
Why
One definition of each helper instead of two or three byte-for-byte copies, following the existing
top-level
internal Duration.toNanosSaturating()precedent in the same area of the codebase.API / build
failureOfandrequireRepresentableareinternal/private, and all setter/parser/schedulersignatures are unchanged, so the public
.apisurface is untouched — noapiDump../gradlew :sdk-core:buildpasses (ktlint, detekt,apiCheck, tests, and the coverage gate).Closes #172