aspnetcore icon indicating copy to clipboard operation
aspnetcore copied to clipboard

Avoid race that can cause Kestrel's RequestAborted to not fire

Open halter73 opened this issue 5 months ago • 2 comments

Before this change, Kestrel had a narrow race where it could fail to trigger the RequestAborted token despite the underlying connection closing. Because CancellationTokenSource.TryReset() is not thread-safe, concurrent calls to Cancel() made by CancelRequestAbortedTokenCallback() on a background thread can end up being ignored.

https://github.com/dotnet/runtime/blob/2dbdff1a7aebd64b4b265d99b173cd77f088c36e/src/libraries/System.Private.CoreLib/src/System/Threading/CancellationTokenSource.cs#L477-L479

I observed exactly this when debugging a hanging test from Send MCP-Protocol-Version header in Streamable HTTP client transport · modelcontextprotocol/csharp-sdk@735be0f by looking at the memory dump from testresults-windows-latest-Release. There you can see that there's a SSE request hanging because RequestAborted never firing despite ConnectionContext.ConnectionClosed firing and Http1Connection._connectionAborted being set.

I think this is definitely the right change for HTTP/1.1, and I copied the CanReuse logic from HTTP/2 for HTTP/3 so it hopefully shouldn't cause issues there, but please double check this @JamesNK. If the new logic does cause issues, it likely means the previous logic was even more broken for HTTP/3, because that would mean _connectionAborted could be Reset() to false while CancelRequestAbortedTokenCallback() from a call to HttpContext.Abort() from a previous request was still scheduled on the thread pool.

To fix this, I made _connectionAborted a terminal state and never reset _abortedCts to null. I think the only alternatives would be to somehow synchronize with the CancelRequestAbortedTokenCallback() and delay resetting until that completes or to not pool altogether.

halter73 avatar Jun 18 '25 01:06 halter73

I think we should also backport this to .NET 8 and 9.

halter73 avatar Jun 18 '25 01:06 halter73

Add a test that a HTTP/3 stream isn't reused when aborted?

JamesNK avatar Jun 18 '25 02:06 JamesNK