Fix the five second stall in lifecycle monitor start and stop - #77
Conversation
The posted Runnable stayed queued after the five second wait expired, so it ran once the looper freed up, long after the caller had already failed and moved on. Keep a reference to the message and remove it on timeout.
StreamLifecycleMonitorImpl bridged addObserver/removeObserver with a blocking main-looper round trip. A caller that already occupies the main looper can never release it, so the wait could only end in the five second timeout. Post the attach/detach without awaiting completion, and run inline only when the caller is on the main looper with nothing of ours already queued -- an inline call behind a queued one would invert the two.
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: Team 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 |
andremion
left a comment
andremion
left a comment
There was a problem hiding this comment.
Looks good, this is the right place for the fix. Two comments inline, neither blocking.
|
Filed the video follow-up for once this is released: https://linear.app/stream/issue/AND-1484. It bumps |
The compareAndSet on `started` and the inline-or-post decision were separate steps. A background start() that won the flip but had not yet incremented pendingOnMain looked like nothing was queued, so a stop() arriving on the main thread in that window detached inline and the attach landed after it, leaving the observer attached with started == false. Take both under one lock. The block itself still runs outside it: the inline path holds the main thread, so nothing of ours can overtake it there, and the lock stays clear of the listener callbacks the attach fans out to.
The attach now lands after start() returns, so a listener subscribed in between is already in place for the ON_RESUME that LifecycleRegistry replays to a new observer. Nothing acts on it -- the recovery evaluator needs an earlier successful connection -- but the delivery is behaviour now, so pin it.
|
2 participants
Goal
Fixes AND-1468
StreamLifecycleMonitorImpl.start()/stop()bridgedaddObserver/removeObserverwith ablocking main-looper round trip: post, then wait on a
CountDownLatchwith a five secondsafety timeout. A caller that already occupies the main looper can never release it, so the
posted message never runs and the wait can only end in the timeout. Surfaced in video as a
five second UI freeze on every logout (AND-1466, stream-video-android#1792, worked around
there at one call site).
Two callers exist, both here, and the connect side has the same shape as the disconnect side.
Implementation
StreamLifecycleMonitorImpl— attach and detach are dispatched to the main looper withoutwaiting for completion. The call runs inline only when the caller is already on the main
looper and nothing of ours is queued: an inline call sitting behind a message posted
earlier from another thread would run ahead of it and invert the pair, leaving
startedand the observer disagreeing. A
pendingOnMaincounter tracks the outstanding messages.runOn— the postedRunnablewas never removed when the wait timed out, so it stayedqueued and executed once the looper freed up, long after the caller had already failed and
moved on. It is now held in a local and dropped with
removeCallbackson timeout.start()andstop()now mean "queued", not "applied", when called off the main looper.Nothing depends on the old ordering:
getCurrentState()readsLifecycle.currentStatedirectly, and
startedis already CAS-guarded.After this change
runOn/runOnMainLooperhave no production callers left in this repo.They stay as-is for downstream
@StreamInternalApiconsumers.Testing
:stream-android-core:testDebugUnitTestgreen with--rerun-tasks.Two new tests: one proves
start()returns while the main looper is never idled, one provesa main-thread call queues behind a registration posted from another thread instead of
overtaking it. One new
runOntest occupies aHandlerThreadpast the five second bound andasserts the timed-out block never runs afterwards.
Each guard was reverted individually to confirm its test goes red. Removing
removeCallbacksreddens only the drop test; removing the pending-message check reddens only the ordering test;
restoring the blocking hop reddens five tests, including the four existing ones updated here —
which confirms those updates assert the non-blocking behaviour rather than absorbing it.
Four existing tests changed: they waited on a loop that idled the looper until the worker
returned, which no longer idles anything now that the call returns immediately. They now
assert the call returned promptly, then idle explicitly.
Detekt: 1309 weighted issues, matching the
developbaseline measured in a clean worktree.The repo's detekt gate is already red on
develop; this change adds nothing to it.Checklist