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

[HTTP/3] HttpConnectionPool may leak authorityExpireTimer / hit ObjectDisposedException #66782

Closed
MihaZupan opened this issue Mar 17, 2022 · 3 comments · Fixed by #72615
Closed
Assignees
Labels
area-System.Net.Http bug tenet-reliability Reliability/stability related issue (stress, load problems, etc.)
Milestone

Comments

@MihaZupan
Copy link
Member

MihaZupan commented Mar 17, 2022

Based on code inspection.

HttpConnectionPool.HandleAltSvc may create a Timer if Dispose runs during/before it, and it will never be disposed.
The method should check _disposed in this branch (while holding the lock), before creating a new Timer:

The good news is that the timer won't root the HttpConnectionPool instance, but the timer entry may remain queued for a month.

A related issue is that the same method will also access the _authorityExpireTimer outside a lock a few lines above, which could lead to an unexpected ObjectDisposedException leaking out:

_authorityExpireTimer.Change(Timeout.Infinite, Timeout.Infinite);

@ghost
Copy link

ghost commented Mar 17, 2022

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

Issue Details

Based on code inspection.

HttpConnectionPool.HandleAltSvc may create a Timer if Dispose runs during/before it.
The method should check _disposed in this branch (while holding the lock), before creating a new Timer:

The good news is that the timer won't root the HttpConnectionPool instance, but the timer entry may remain queued for a month.

A related issue is that the same method will also access the _authorityExpireTimer outside a lock a few lines above, which could lead to an unexpected ObjectDisposedException leaking out:

_authorityExpireTimer.Change(Timeout.Infinite, Timeout.Infinite);

Author: MihaZupan
Assignees: -
Labels:

bug, area-System.Net.Http

Milestone: -

@MihaZupan
Copy link
Member Author

MihaZupan commented Mar 18, 2022

Looks like this AltSvc logic around the blocklist is also racy.
Tasks may not be canceled or you may try using a disposed CTS.

In this case, the whole HttpConnectionPool is gonna be rooted by the 10 minute timer.

@MihaZupan MihaZupan added untriaged New issue has not been triaged by the area owner tenet-reliability Reliability/stability related issue (stress, load problems, etc.) labels Mar 21, 2022
@karelz karelz removed the untriaged New issue has not been triaged by the area owner label Mar 22, 2022
@karelz karelz added this to the 7.0.0 milestone Mar 22, 2022
@karelz
Copy link
Member

karelz commented Mar 22, 2022

Triage: Affects HTTP/3, reliability - race condition. We should fix it in 7.0.

@CarnaViire CarnaViire self-assigned this Jul 19, 2022
@karelz karelz changed the title HttpConnectionPool may leak authorityExpireTimer / hit ObjectDisposedException [HTTP/3] HttpConnectionPool may leak authorityExpireTimer / hit ObjectDisposedException Jul 21, 2022
@ghost ghost added the in-pr There is an active PR which will close this issue when it is merged label Jul 21, 2022
@ghost ghost removed the in-pr There is an active PR which will close this issue when it is merged label Jul 22, 2022
@ghost ghost locked as resolved and limited conversation to collaborators Aug 21, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
area-System.Net.Http bug tenet-reliability Reliability/stability related issue (stress, load problems, etc.)
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants