Skip to content

GetTimeoutException() is dead code: an RPC timeout surfaces as cancellation, never TimeoutException #1996

Description

@lukebakken

Note

This issue was written by Claude (Anthropic's Claude Code) under the direction of @lukebakken. The code references were verified against the #1994 branch, and the single-completion behaviour was confirmed with a probe; the options listed below are AI-drafted starting points, not settled decisions.

GetTimeoutException() in projects/RabbitMQ.Client/Impl/AsyncRpcContinuations.cs:246-253 never reaches an awaiter. Every call site follows the same pattern of cancelling the TCS first and only then trying to set the exception:

if (tcs.TrySetCanceled(cancellationToken))
{
    tcs.TrySetException(GetTimeoutException());
}

A TaskCompletionSource can only be completed once, so the TrySetCanceled that guards the block is also what makes the TrySetException a no-op. Verified with a probe: TrySetCanceled returns True, TrySetException returns False, and the awaiter observes a TaskCanceledException, for which is TimeoutException is False.

Three sites are affected: lines 176-184 (the OperationCanceledException catch in HandleCommandAsync) and both HandleContinuationTimeout overloads at 227-234 and 236-243.

The comment above the first one records the original intent:

// Cancellation was successful, does this mean we set a TimeoutException
// in the same manner as BlockingCell used to

6.x did surface a TimeoutException here, from BlockingCell.WaitForValue(TimeSpan). In 7.x callers see cancellation instead. That is the root cause of #1993: HandleTopologyRecoveryException listed TimeoutException as retryable, which looked correct but never matched, because the exception that actually arrives is a TaskCanceledException.

No present-day symptom now that #1994 classifies OperationCanceledException, so this is cleanup rather than a bug fix. Two directions:

  1. Delete GetTimeoutException() and the dead TrySetException calls, and document that an RPC timeout surfaces as OperationCanceledException. Honest about current behaviour, but a timeout is arguably not a cancellation from the caller's point of view.
  2. Actually surface a TimeoutException by setting it instead of cancelling. Better diagnostics and closer to 6.x, but a public behaviour change: anyone catching OperationCanceledException around an RPC would stop seeing it, and it would need the Retry recovery when a topology operation times out #1994 classification revisited.

The // TODO at line 248 suggests this was already known to be unfinished.

Activity

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Metadata

Metadata

Assignees

Labels

A-channelArea: Channel operations, RPC continuations, publisher confirms.C-refactorCategory: Code restructuring without behavior change

Type

No type

Projects

No projects

    Relationships

    None yet

    Development

    No branches or pull requests

    Issue actions