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 data race leading to a deadlock when opening QuicStream #101250

Merged
merged 7 commits into from
Apr 25, 2024

Conversation

rzikm
Copy link
Member

@rzikm rzikm commented Apr 18, 2024

Fixes #101233.
Fixes #100971.
Contributes to #101463

See original issue for the description of the deadlock scenario.

Copy link
Contributor

Tagging subscribers to this area: @dotnet/ncl
See info in area-owners.md if you want to be subscribed.

Copy link
Member

@ManickaP ManickaP left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good, thanks for finding and fixing this!

@rzikm
Copy link
Member Author

rzikm commented Apr 23, 2024

The original version was not a complete fix, it turns out that calling Complete on a Channel<T> will not complete the Completion if there are unread items in the channel.

if (_acceptQueue.Reader.Completion.IsFaulted || connectionAbortedByPeer)
{
await _acceptQueue.Reader.Completion.ConfigureAwait(false);
}

So I added logic which will drain the channel once we are sure the streams inside have been shutdown. Please take another look.

@rzikm rzikm requested a review from ManickaP April 23, 2024 08:03
@rzikm
Copy link
Member Author

rzikm commented Apr 23, 2024

/azp run runtime-libraries-coreclr outerloop

Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@rzikm rzikm force-pushed the quic-stream-abort-from-api branch from 69d4dc1 to 5586d18 Compare April 25, 2024 09:19
Copy link
Member

@ManickaP ManickaP left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🥳 :shipit:

Copy link
Member

@CarnaViire CarnaViire left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Given the urgency, I won't poke at nits. Otherwise LGTM :shipit:

@rzikm
Copy link
Member Author

rzikm commented Apr 25, 2024

no QUIC or HTTP/3 failures in CI.

@rzikm rzikm merged commit 127844c into dotnet:main Apr 25, 2024
78 of 84 checks passed
matouskozak pushed a commit to matouskozak/runtime that referenced this pull request Apr 30, 2024
…01250)

* Fix data race leading to a deadlock.

* Remove unwanted change

* Code review feedback

* Fix hang

* Add assert

* Fix potential crash

* Code review feedback
michaelgsharp pushed a commit to michaelgsharp/runtime that referenced this pull request May 9, 2024
…01250)

* Fix data race leading to a deadlock.

* Remove unwanted change

* Code review feedback

* Fix hang

* Add assert

* Fix potential crash

* Code review feedback
@rzikm
Copy link
Member Author

rzikm commented May 13, 2024

/backport to release/8.0-staging

Copy link
Contributor

Started backporting to release/8.0-staging: https://github.com/dotnet/runtime/actions/runs/9059881753

Copy link
Contributor

@rzikm backporting to release/8.0-staging failed, the patch most likely resulted in conflicts:

$ git am --3way --ignore-whitespace --keep-non-patch changes.patch

Applying: Fix data race leading to a deadlock.
Using index info to reconstruct a base tree...
M	src/libraries/System.Net.Http/tests/FunctionalTests/HttpClientHandlerTest.Http3.cs
M	src/libraries/System.Net.Quic/src/System/Net/Quic/QuicConnection.cs
M	src/libraries/System.Net.Quic/src/System/Net/Quic/QuicStream.cs
Falling back to patching base and 3-way merge...
Auto-merging src/libraries/System.Net.Quic/src/System/Net/Quic/QuicStream.cs
Auto-merging src/libraries/System.Net.Quic/src/System/Net/Quic/QuicConnection.cs
CONFLICT (content): Merge conflict in src/libraries/System.Net.Quic/src/System/Net/Quic/QuicConnection.cs
Auto-merging src/libraries/System.Net.Http/tests/FunctionalTests/HttpClientHandlerTest.Http3.cs
error: Failed to merge in the changes.
hint: Use 'git am --show-current-patch=diff' to see the failed patch
Patch failed at 0001 Fix data race leading to a deadlock.
When you have resolved this problem, run "git am --continue".
If you prefer to skip this patch, run "git am --skip" instead.
To restore the original branch and stop patching, run "git am --abort".
Error: The process '/usr/bin/git' failed with exit code 128

Please backport manually!

Copy link
Contributor

@rzikm an error occurred while backporting to release/8.0-staging, please check the run log for details!

Error: git am failed, most likely due to a merge conflict.

rzikm added a commit to rzikm/dotnet-runtime that referenced this pull request May 13, 2024
…01250)

* Fix data race leading to a deadlock.

* Remove unwanted change

* Code review feedback

* Fix hang

* Add assert

* Fix potential crash

* Code review feedback
@karelz karelz added this to the 9.0.0 milestone May 13, 2024
rzikm added a commit that referenced this pull request May 13, 2024
…ream (#102147)

* Fix data race leading to a deadlock when opening QuicStream (#101250)

* Fix data race leading to a deadlock.

* Remove unwanted change

* Code review feedback

* Fix hang

* Add assert

* Fix potential crash

* Code review feedback

* Fix thrown exception.
Ruihan-Yin pushed a commit to Ruihan-Yin/runtime that referenced this pull request May 30, 2024
…01250)

* Fix data race leading to a deadlock.

* Remove unwanted change

* Code review feedback

* Fix hang

* Add assert

* Fix potential crash

* Code review feedback
@github-actions github-actions bot locked and limited conversation to collaborators Jun 14, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
6 participants