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

QuicConnection not being released after exception #87291

Closed
amcasey opened this issue Jun 8, 2023 · 2 comments · Fixed by #87328
Closed

QuicConnection not being released after exception #87291

amcasey opened this issue Jun 8, 2023 · 2 comments · Fixed by #87328
Assignees
Milestone

Comments

@amcasey
Copy link
Member

amcasey commented Jun 8, 2023

Description

The context is best explained by the thread of comments in the original PR but, to summarize, the aspnetcore test QuicConnectionListenerTests.AcceptAsync_NoCertificateCallback_RemovedFromPendingConnections started failing when we picked up #87057, which revealed a pre-existing bug - a QuicConnection that apparently keeps itself alive, preventing Kestrel's ConditionalWeakTable (referred to as CWT in the test) from being cleared, even though an exception prevented the connection.

@BrennanConroy goes into more detail here.

Reproduction Steps

QuicConnectionListenerTests.AcceptAsync_NoCertificateCallback_RemovedFromPendingConnections shows the failure, though there's probably a simpler repro.

Expected behavior

The QuicConnection is finalized and removed from aspnetcore's ConditionalWeakTable.

Actual behavior

The QuicConnection is still rooted by a GC handle.

Regression?

It predates the insertion, but we don't know by how much.

Known Workarounds

No response

Configuration

The PR has commit SHAs for both repos, but it's basically main as of 2023-06-08.

Other information

There's a tracking bug here for any compensating work required in aspnetcore.

@ghost ghost added the untriaged New issue has not been triaged by the area owner label Jun 8, 2023
@ghost
Copy link

ghost commented Jun 8, 2023

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

Issue Details

Description

The context is best explained by the thread of comments in the original PR but, to summarize, the aspnetcore test QuicConnectionListenerTests.AcceptAsync_NoCertificateCallback_RemovedFromPendingConnections started failing when we picked up #87057, which revealed a pre-existing bug - a QuicConnection that apparently keeps itself alive, preventing Kestrel's ConditionalWeakTable (referred to as CWT in the test) from being cleared, even though an exception prevented the connection.

@BrennanConroy goes into more detail here.

Reproduction Steps

QuicConnectionListenerTests.AcceptAsync_NoCertificateCallback_RemovedFromPendingConnections shows the failure, though there's probably a simpler repro.

Expected behavior

The QuicConnection is finalized and removed from aspnetcore's ConditionalWeakTable.

Actual behavior

The QuicConnection is still rooted by a GC handle.

Regression?

It predates the insertion, but we don't know by how much.

Known Workarounds

No response

Configuration

The PR has commit SHAs for both repos, but it's basically main as of 2023-06-08.

Other information

There's a tracking bug here for any compensating work required in aspnetcore.

Author: amcasey
Assignees: -
Labels:

area-System.Net.Quic

Milestone: -

@amcasey
Copy link
Member Author

amcasey commented Jun 8, 2023

FYI @karelz @eerhardt

@stephentoub stephentoub added this to the 8.0.0 milestone Jun 9, 2023
@ghost ghost removed the untriaged New issue has not been triaged by the area owner label Jun 9, 2023
@stephentoub stephentoub added bug untriaged New issue has not been triaged by the area owner labels Jun 9, 2023
@ManickaP ManickaP self-assigned this Jun 9, 2023
@ManickaP ManickaP removed the untriaged New issue has not been triaged by the area owner label Jun 9, 2023
@ghost ghost added the in-pr There is an active PR which will close this issue when it is merged label Jun 9, 2023
@ghost ghost removed the in-pr There is an active PR which will close this issue when it is merged label Jun 14, 2023
@ghost ghost locked as resolved and limited conversation to collaborators Jul 14, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants