fix(glide): prevent stale failure callbacks across request swaps#943
fix(glide): prevent stale failure callbacks across request swaps#943frozenstar80 wants to merge 1 commit into
Conversation
WalkthroughRefactors Glide failure propagation: ChangesGlide stale failure fix
Estimated code review effort: 3 (Moderate) | ~20 minutes Sequence Diagram(s)sequenceDiagram
participant Compose as GlideImage (Compose)
participant Listener as FlowRequestListener
participant Target as FlowCustomTarget
participant Scope as producerScope
Compose->>Listener: request image load (model)
alt load fails
Listener->>Scope: trySendBlocking(Failure(data=null, reason=e))
Listener->>Scope: close channel
Listener-->>Compose: return true
else load fails at target
Target->>Scope: close channel (no Failure emitted)
end
Compose->>Scope: switch to new model (success)
Scope-->>Compose: emit Success state
Suggested reviewers: Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (1)
glide/src/androidTest/kotlin/com/skydoves/landscapist/glide/GlideImageTest.kt (1)
218-236: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAssertions may run before a stale, in-flight failure has time to arrive.
waitUntilstops as soon as the first terminal state is recorded. Since the new model here is a local drawable (near-instant, no network I/O) while the stale request is a real network URL (needs DNS/connection failure to resolve), the Success is very likely to land well before the earlier failure would ever complete — so assertions may pass even if the stale-failure race were reintroduced, undermining the purpose of this regression test.Consider adding a short grace period after the first terminal state is observed, before asserting, to give a late/stale callback a realistic chance to surface.
♻️ Proposed adjustment
composeTestRule.waitUntil(10_000) { terminalStatesForLatestModel.isNotEmpty() } + + // Give any stale/delayed callback from the superseded request a chance to arrive + // before asserting, so a regression would actually be caught here. + Thread.sleep(2_000) + composeTestRule.waitForIdle() composeTestRule.runOnIdle {🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@glide/src/androidTest/kotlin/com/skydoves/landscapist/glide/GlideImageTest.kt` around lines 218 - 236, The regression test in GlideImageTest is asserting immediately after the first terminal state, which can miss a late stale failure from the previous request. Update the test around imageModel, waitUntil, and terminalStatesForLatestModel to add a short grace period after the first terminal state is observed before running the Success/Failure assertions, so a delayed callback has time to surface and the race is actually validated.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In
`@glide/src/androidTest/kotlin/com/skydoves/landscapist/glide/GlideImageTest.kt`:
- Around line 218-236: The regression test in GlideImageTest is asserting
immediately after the first terminal state, which can miss a late stale failure
from the previous request. Update the test around imageModel, waitUntil, and
terminalStatesForLatestModel to add a short grace period after the first
terminal state is observed before running the Success/Failure assertions, so a
delayed callback has time to surface and the race is actually validated.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 1a20512d-f182-4ae3-ad12-dd6c4b1d8fe0
📒 Files selected for processing (4)
glide/src/androidTest/kotlin/com/skydoves/landscapist/glide/GlideImageTest.ktglide/src/main/kotlin/com/skydoves/landscapist/glide/FlowCustomTarget.ktglide/src/main/kotlin/com/skydoves/landscapist/glide/FlowRequestListener.ktglide/src/main/kotlin/com/skydoves/landscapist/glide/GlideImage.kt
1 participant
What
This PR fixes a stale-state issue in the Glide integration where a failure callback from an earlier request could affect the currently active request state.
Changes
FlowRequestListener.onLoadFailed(...).truefrom GlideonLoadFailed(...)to prevent duplicate/late target-level failure routing.failException/updateFailException) fromFlowCustomTarget.staleFailure_fromPreviousRequest_doesNotOverrideLatestSuccessglide/src/androidTest/kotlin/com/skydoves/landscapist/glide/GlideImageTest.ktWhy
FlowCustomTargetcan be reused across recompositions. When failure propagation depends on target-side mutable state, late callbacks from previous requests can leak into the active request flow.By handling failures in
FlowRequestListener(request-scoped), terminal states stay aligned to the current request.Validation
Executed successfully:
./gradlew :glide:compileDebugKotlin --no-daemon./gradlew :glide:testDebugUnitTest --no-daemon./gradlew :glide:compileDebugAndroidTestKotlin --no-daemon./gradlew spotlessApply --no-daemon./gradlew apiDump --no-daemonRisk
Low-to-medium. Scope is limited to Glide failure-path state propagation and includes regression coverage.
Summary by CodeRabbit