fix: deliver a completed response after the request timeout expires - #151
Draft
AndreiSingeorzan wants to merge 1 commit into
Draft
fix: deliver a completed response after the request timeout expires#151AndreiSingeorzan wants to merge 1 commit into
AndreiSingeorzan wants to merge 1 commit into
Conversation
A handler that takes no CancellationToken keeps running past the request timeout, so `response` is non-null by the time SendResponse is reached. Sending it on the already-canceled token throws in Connection.SendMessage before a byte reaches the wire, and the `when (response is null)` filter does not match, so no Response.Fail is sent either. The caller is told nothing and waits forever on a healthy connection. Send the completed response on `default` instead, as OnError already does. This is narrow: Connection.SendMessage already writes with CancellationToken.None once it holds the send lock, so the token only ever gated acquiring that lock. Adds RequestTimeoutTests over named pipes, plus an ISlowService whose method deliberately declares no CancellationToken - no existing test service had that shape, which is why this had no coverage. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
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.
Problem
A request handler that outlives the server's request timeout still runs to completion, but its response is then silently discarded. The connection stays healthy, no error frame is sent, and the caller is told nothing — so a client without a
RequestTimeoutwaits forever.This is not hypothetical. It is the root cause of a UiPath Studio customer issue where opening a project hung indefinitely on "Loading project…". Studio's
IProjectProcessControlService.OpenProjecttakes noCancellationTokenand, on a cold machine, loads 681 dependency assemblies plus Roslyn compilations — comfortably past the 60 s request timeout that pipe used. Paired memory dumps showed the project process holding a completedTask<AssemblyLoadResult[]>with all 681 results while the shell still had the request pending on aConnectedpipe.Root cause
Server.OnRequestReceived:CancellationToken, it never notices and keeps working — the timeout stops nothing.responseis non-null. The answer exists.SendResponse(response, token)is called with the already-canceled token.Connection.SendMessageobserves it at_sendLock.WaitAsync(cancellationToken)and throws before a byte reaches the wire.when (response is null)filter does not match, soOnError— which would have sent aResponse.Failon an uncancelable token — never runs.Note the asymmetry this creates: a handler that does take a
CancellationTokenthrows at step 1, leavesresponsenull, matches the filter, and the caller gets a cleanTimeoutException. Identical timeout, opposite outcome, decided purely by whether the contract method happens to declare the parameter.Fix
Once the handler has produced a response, the peer is owed it — cancelling delivery discards work that is already done. This makes the success path consistent with
OnError, which has always sent ondefaultfor exactly this reason.Worth noting how narrow the behavioural change is:
Connection.SendMessagealready writes withCancellationToken.Noneonce it holds the send lock. The token only ever gated acquiring that lock. So this does not make the write less interruptible than it already was.Test
RequestTimeoutTests+RequestTimeoutTestsOverNamedPipes, with a newISlowService.EchoAfterIgnoringCancellationthat deliberately declares noCancellationToken— the shape of the contracts that hit this. No existing test service had that shape, which is why the bug had no coverage.Server timeout 1 s, handler 3 s, client sends no timeout.
Before the fix — fails at the 10 s lease:
with the library logging its own failure:
After the fix — passes in 3 s, i.e. the moment the handler finishes:
Steps 1-6 are identical in both runs — the timeout still fires at 1 s and the handler still ignores it. Only the delivery changes.
The test keeps its step tracing deliberately: the failure mode is a silence, and the trace is what makes the chronology legible to the next person who reads it. On the failure path it also probes the connection afterwards and shows a second call on the same pipe succeeding, which is the part that makes this bug so confusing in the field.
Existing coverage confirms timeouts are not weakened —
SystemTests.cs:73,SystemTests.cs:80andComputingTests.cs:84still assertTimeoutException, and the full suite is green: 99 passed on net6.0, 97 on net461.Not included, deliberately
when (response is null)filter is still wrong in the general case. Any send failure after a successful handler — not just a timeout — is swallowed the same way. The robust form is asentflag so no path can consume a request without answering it. That needs its own test with an induced send failure, so it belongs in a separate PR rather than riding on this one.Server, above the transport, so one variant demonstrates it, but the repo convention is three. Happy to add Tcp and WebSockets here if preferred.For reviewers
SendStreampasses the token to the write and registerscancellationToken.UnsafeRegister(… Connection.Dispose()). Today a timed-out streaming response tears down the whole connection; under this change it is delivered normally. I believe that is the correct and consistent outcome, but it is a real behavioural change forStream-returning contracts and deserves a second opinion.Request.GetTimeout: a client-suppliedTimeoutInSecondsoverrides the server'sRequestTimeoutwith no ceiling, so a peer can pin a server-side request slot for as long as it likes. Worth its own issue.🤖 Generated with Claude Code