Skip to content

Fix data race leading to a deadlock when opening QuicStream - #101250

Merged
rzikm merged 7 commits into
dotnet:mainfrom
rzikm:quic-stream-abort-from-api
Apr 25, 2024
Merged

Fix data race leading to a deadlock when opening QuicStream#101250
rzikm merged 7 commits into
dotnet:mainfrom
rzikm:quic-stream-abort-from-api

Conversation

@rzikm

@rzikm rzikm commented Apr 18, 2024

Copy link
Copy Markdown
Member

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

See original issue for the description of the deadlock scenario.

@ghost ghost added the area-System.Net.Quic label Apr 18, 2024
@rzikm
rzikm requested a review from a team April 18, 2024 16:43
@dotnet-policy-service

Copy link
Copy Markdown
Contributor

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

Comment thread src/libraries/System.Net.Quic/src/System/Net/Quic/QuicStream.cs Outdated

@ManickaP ManickaP left a comment

Copy link
Copy Markdown
Member

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!

Comment thread src/libraries/System.Net.Quic/src/System/Net/Quic/QuicStream.cs Outdated
Comment thread src/libraries/System.Net.Quic/src/System/Net/Quic/QuicStream.cs Outdated
Comment thread src/libraries/System.Net.Quic/src/System/Net/Quic/QuicConnection.cs Outdated
Comment thread src/libraries/System.Net.Quic/src/System/Net/Quic/QuicConnection.cs Outdated
@rzikm

rzikm commented Apr 23, 2024

Copy link
Copy Markdown
Member Author

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
Comment thread src/libraries/System.Net.Quic/src/System/Net/Quic/QuicConnection.cs Outdated
@rzikm

rzikm commented Apr 23, 2024

Copy link
Copy Markdown
Member Author

/azp run runtime-libraries-coreclr outerloop

@azure-pipelines

Copy link
Copy Markdown
Azure Pipelines successfully started running 1 pipeline(s).
Comment thread src/libraries/System.Net.Quic/src/System/Net/Quic/QuicConnection.cs Outdated
Comment thread src/libraries/System.Net.Quic/src/System/Net/Quic/QuicConnection.cs Outdated
Comment thread src/libraries/System.Net.Quic/src/System/Net/Quic/QuicConnection.cs Outdated
@rzikm
rzikm force-pushed the quic-stream-abort-from-api branch from 69d4dc1 to 5586d18 Compare April 25, 2024 09:19

@ManickaP ManickaP left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

🥳 :shipit:

@CarnaViire CarnaViire left a comment

Copy link
Copy Markdown
Member

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

rzikm commented Apr 25, 2024

Copy link
Copy Markdown
Member Author

no QUIC or HTTP/3 failures in CI.

@rzikm
rzikm merged commit 127844c into dotnet:main Apr 25, 2024
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

rzikm commented May 13, 2024

Copy link
Copy Markdown
Member Author

/backport to release/8.0-staging

@github-actions

Copy link
Copy Markdown
Contributor

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

@github-actions

Copy link
Copy Markdown
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!

@github-actions

Copy link
Copy Markdown
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.