Threading refactor (6/7): Tracer correctness & chain.isDone()#6
Open
igorbernstein2 wants to merge 3 commits into
Open
Threading refactor (6/7): Tracer correctness & chain.isDone()#6igorbernstein2 wants to merge 3 commits into
igorbernstein2 wants to merge 3 commits into
Conversation
igorbernstein2
force-pushed
the
threading-refactor-phase-6
branch
3 times, most recently
from
June 17, 2026 20:02
fb931b0 to
c212e84
Compare
igorbernstein2
force-pushed
the
threading-refactor-phase-5
branch
from
June 17, 2026 20:02
6a94038 to
49dd066
Compare
nimf
approved these changes
Jun 22, 2026
| private boolean attemptFinished = false; | ||
|
|
||
| private void finishAttempt(VRpcResult result) { | ||
| if (!attemptFinished) { |
There was a problem hiding this comment.
can this be called from different threads? compareAndSet?
Owner
Author
There was a problem hiding this comment.
nope, the primary purpose of this refactor is to make all of this be single threaded
mutianf
approved these changes
Jun 22, 2026
igorbernstein2
force-pushed
the
threading-refactor-phase-5
branch
from
June 22, 2026 21:41
49dd066 to
de62fa1
Compare
igorbernstein2
force-pushed
the
threading-refactor-phase-6
branch
2 times, most recently
from
June 23, 2026 20:55
852ed0b to
c503a03
Compare
igorbernstein2
force-pushed
the
threading-refactor-phase-5
branch
from
June 29, 2026 17:20
b82a480 to
8f643ad
Compare
igorbernstein2
force-pushed
the
threading-refactor-phase-6
branch
from
June 29, 2026 17:20
c503a03 to
7864221
Compare
… Active State machine now has an onExit hook called from onStateChange before the swap. States that hold cleanup-worthy resources override it. Active owns its own tracer-pairing flag + finishAttempt helper (no longer a RetryingVRpc-level bool). Listener paths and onCancel call finishAttempt with the right result; onExit is a safety net that guarantees the pairing is balanced even if a future exit path forgets. Fixes the attempt.start synchronous-throw leak. Also resolves two latent tracer hazards: - tracer.onAttemptFinish now fires when an in-flight attempt is cancelled (previously the late server onClose was dropped by the stale-state guard, so the cancelled attempt's tracer span leaked). - The listener-path tracer.onAttemptFinish is gated on the stale-state check first, matching onMessage above — a discarded onClose can no longer double-fire the tracer. Scheduled cleanup (timer + stop-hook unregister) consolidates from three parallel sites (onCancel, timer-fire body, stop-hook body) into one onExit. onCancel drops to the default no-op. Done.onStart no longer balances per-attempt tracer state — that lives on Active now.
VOperationImpl.start needs to detect whether chain.start synchronously delivered a terminal onClose, so it can skip registering the gRPC cancellation listener (which would otherwise leak onto grpcContext). Previously this was tracked via a 'closed' flag on CleanupListener — a piggyback bookkeeping field on the listener wrapper that exists only for VOperationImpl's coordination. Add isDone() to the VRpc interface and ask the chain directly. The chain is the natural source of truth for its own terminal state. CleanupListener shrinks back to its single concern: relay events and unhook the gRPC cancellation listener on close. Implementations: RetryingVRpc delegates to currentState.isDone(); VRpcImpl reports state==CLOSED; ForwardingVRpc forwards; PendingVRpc defers to realCall once handed off, otherwise reports isCancelled. Test fakes (DelayedVRpc, FakeVRpc, anonymous VOperationImplTest chains) implement the new method. Drive-by: drop the defensive handling of tracer.onOperationStart throws from RetryingVRpc.start, and the symmetric `!started` early-return in Done.onStart that paired with it. CompositeVRpcTracer catches throws from every child tracer, so the only way tracer.onOperationStart reaches RetryingVRpc with a real throw is a test that bypasses Composite. Dead code in production; relying on the existing chain.cancel cascade is simpler than maintaining a separate short-circuit path. Also: the already-started error in RetryingVRpc.start now dispatches listener.onClose through ctx.getExecutor() rather than invoking it synchronously on the caller, matching the dispatch convention used everywhere else in the chain.
igorbernstein2
force-pushed
the
threading-refactor-phase-6
branch
from
June 30, 2026 17:49
7864221 to
be9a7b3
Compare
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
3 participants
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.
Summary
Phase 6 of 7. Two structural cleanups motivated by the new threading model:
Activestate with anonExitsafety net — fixes three tracer-leak hazards (synchronousattempt.startthrow, cancelled in-flight attempts, double-fire on staleonClose).What's in here
refactor: add State.onExit and localize per-attempt tracer pairing to Active(+ two tracer fixes)onExithook called fromonStateChangebefore the swap.Activeowns its own tracer-pairingfinishAttempthelper. Listener paths,onCancel, andonExitsafety net all callfinishAttemptsotracer.onAttemptFinishis always balanced — including cancelled in-flight attempts (no longer dropped by theattempt.startthrows. Listener-pathtracer.onAttemptFinishis gated on the stale-state check first, matchingonMessage. Scheduled cleanup (timer + stop-hook unregister) consolidatesonExit.refactor: replace CleanupListener.closed flag with chain.isDone()VOperationImpl.startneeds to detect whetherchain.startsynchronously delivered a terminalonClose, to skip registering the gRPC cancellationgrpcContext). Previously tracked via aclosedflag onCleanupListener— a piggyback field that existed only forVOperationImpl's coordination. AddisDone()to theVRpcCleanupListenershrinks back to its single concern: relay events and unhook the gRPC cancellation listener on close.Stack position
threading-refactor-phase-5threading-refactor-phase-7— documentation