Fix HttpContext race condition by copying values to reader and writer #2294

JamesNK · 2023-10-13T05:55:16Z

Potential fix to HttpContext.RequestAborted race condition. Fix is to copy fields need from the context when reader and writer are created.

BrennanConroy · 2023-10-16T17:10:48Z

src/Grpc.AspNetCore.Server/Internal/HttpContextStreamReader.cs

+        // Copy HttpContext values.
+        // This is done to avoid a race condition when reading them from HttpContext later when running in a separate thread.
+        _bodyReader = _serverCallContext.HttpContext.Request.BodyReader;
+        // Copy lifetime feature because HttpContext.RequestAborted on .NET 6 doesn't return the real cancellation token.


Could you explain both the 6.0 comment as well as this whole PR?

From my understanding; If you're using the cancellation token on a background thread it might be disposed instead of canceled when the HttpContext is reused. And if you access the token property after it's been reset you might be using a token from another request.

Root problem: dotnet/aspnetcore#42040
The fix it to access the cancellation token on the feature directly. HttpContext.RequestAborted isn't thread-safe but the feature is.

re: .NET 6. I observed that the cancellation token returned by HttpContext.RequestAborted and cached at the start of the request behaved strangely. It wouldn't immediately be canceled when the request was aborted. The cached HttpContext.RequestAborted value appeared to be different from the value after the abort happened (but while the request was still in progress, so the value isn't coming from another request). I didn't investigate why I got this odd behavior in .NET 6 since there was a workaround - access the token from the feature - and .NET 7 and .NET 8 behaved how I expected.

Root problem: dotnet/aspnetcore#42040
The fix it to access the cancellation token on the feature directly. HttpContext.RequestAborted isn't thread-safe but the feature is.

Oh, so gRPC isn't using the token after the middleware returns? That's what my comment was focused on.

No. This is in the context of one request.

Fix HttpContext race condition by copying values to reader and writer

41d836f

JamesNK marked this pull request as draft October 13, 2023 07:04

JamesNK added 3 commits October 13, 2023 16:26

Comments and test

7fe04cb

Fix tests

0af3b5b

Fix tests

1ec8e68

JamesNK marked this pull request as ready for review October 14, 2023 04:22

JamesNK requested review from mgravell, ReubenBond, BrennanConroy and amcasey October 14, 2023 04:23

Clean up

a7d4ccd

BrennanConroy reviewed Oct 16, 2023

View reviewed changes

BrennanConroy approved these changes Oct 17, 2023

View reviewed changes

JamesNK merged commit 91be392 into grpc:master Oct 18, 2023
5 checks passed

JamesNK deleted the jamesnk/streaming-race branch October 18, 2023 00:26

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix HttpContext race condition by copying values to reader and writer #2294

Fix HttpContext race condition by copying values to reader and writer #2294

JamesNK commented Oct 13, 2023 •

edited

Loading

BrennanConroy Oct 16, 2023

JamesNK Oct 16, 2023 •

edited

Loading

BrennanConroy Oct 17, 2023

JamesNK Oct 17, 2023

Fix HttpContext race condition by copying values to reader and writer #2294

Fix HttpContext race condition by copying values to reader and writer #2294

Conversation

JamesNK commented Oct 13, 2023 • edited Loading

BrennanConroy Oct 16, 2023

Choose a reason for hiding this comment

JamesNK Oct 16, 2023 • edited Loading

Choose a reason for hiding this comment

BrennanConroy Oct 17, 2023

Choose a reason for hiding this comment

JamesNK Oct 17, 2023

Choose a reason for hiding this comment

JamesNK commented Oct 13, 2023 •

edited

Loading

JamesNK Oct 16, 2023 •

edited

Loading