Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix canceled requests in IIS #52056

Merged
merged 1 commit into from
Nov 14, 2023
Merged

Fix canceled requests in IIS #52056

merged 1 commit into from
Nov 14, 2023

Conversation

wtgodbe
Copy link
Member

@wtgodbe wtgodbe commented Nov 14, 2023

Canceled requests can end up hanging a thread due to the std::conditional_variable never being triggered. This is because the previous assumption, that disconnect would be called once there was an http context set in the native layer, was wrong. Because NotifyDisconnect is called by IIS and SetManagedHttpContext is called by managed code, they can race and end up with m_disconnectFired as true, but m_pManagedHttpContext as false.

The fix is fairly simple, just don't gate unblocking the std::conditional_variable on whether the http context has been set.

Added comments as well to make it a little easier to understand some of the code, hopefully we'll slowly improve the maintainability by doing this with future changes as well.

Canceled requests can end up hanging a thread due to the `std::conditional_variable` never being triggered. This is because the previous assumption, that disconnect would be called once there was an http context set in the native layer, was wrong. Because `NotifyDisconnect` is called by IIS and `SetManagedHttpContext` is called by managed code, they can race and end up with `m_disconnectFired` as true, but `m_pManagedHttpContext` as false.

The fix is fairly simple, just don't gate unblocking the `std::conditional_variable` on whether the http context has been set.

Added comments as well to make it a little easier to understand some of the code, hopefully we'll slowly improve the maintainability by doing this with future changes as well.
@dotnet-issue-labeler dotnet-issue-labeler bot added the area-networking Includes servers, yarp, json patch, bedrock, websockets, http client factory, and http abstractions label Nov 14, 2023
@wtgodbe wtgodbe changed the title Merged PR 34318: Fix canceled requests in IIS Fix canceled requests in IIS Nov 14, 2023
@ghost ghost added the area-infrastructure Includes: MSBuild projects/targets, build scripts, CI, Installers and shared framework label Nov 14, 2023
@wtgodbe wtgodbe enabled auto-merge (squash) November 14, 2023 21:53
@wtgodbe wtgodbe merged commit 676418d into main Nov 14, 2023
26 checks passed
@wtgodbe wtgodbe deleted the wtgodbe/Port34318 branch November 14, 2023 23:22
@ghost ghost added this to the 9.0-preview1 milestone Nov 14, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area-infrastructure Includes: MSBuild projects/targets, build scripts, CI, Installers and shared framework area-networking Includes servers, yarp, json patch, bedrock, websockets, http client factory, and http abstractions
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants