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.

Sign up for GitHub

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

Jump to bottom

Http2's SendPingAsync may leak UnobservedTaskExceptions #64450

Closed
MihaZupan opened this issue Jan 28, 2022 · 4 comments · Fixed by #64494
Closed

Http2's SendPingAsync may leak UnobservedTaskExceptions #64450

MihaZupan opened this issue Jan 28, 2022 · 4 comments · Fixed by #64494
Assignees
@shubhanshu02
Labels
area-System.Net.Http bug help wanted [up-for-grabs] Good issue for external contributors
Milestone
7.0.0

Comments

@MihaZupan
Copy link
Member

(based on code inspection)

The SendPingAsync call that we do as part of the ConnectionPoolManager's heartbeat isn't awaited and so any exceptions from it would bubble up as UnobservedTaskExceptions.

runtime/src/libraries/System.Net.Http/src/System/Net/Http/SocketsHttpHandler/Http2Connection.cs

Line 2041 in 82c45ca

SendPingAsync(pingPayload);

We should consume the task with LogExceptions(SendPingAsync(pingPayload)) as we do when sending out pings normally.

Should be an easy fix - marking as up for grabs.
@MihaZupan MihaZupan added bug area-System.Net.Http help wanted [up-for-grabs] Good issue for external contributors labels Jan 28, 2022
@MihaZupan MihaZupan added this to the 7.0.0 milestone Jan 28, 2022
@dotnet-issue-labeler dotnet-issue-labeler bot added the untriaged New issue has not been triaged by the area owner label Jan 28, 2022
@ghost
Copy link

ghost commented Jan 28, 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)

The SendPingAsync call that we do as part of the ConnectionPoolManager's heartbeat isn't awaited and so any exceptions from it would bubble up as UnobservedTaskExceptions.

runtime/src/libraries/System.Net.Http/src/System/Net/Http/SocketsHttpHandler/Http2Connection.cs

Line 2041 in 82c45ca

SendPingAsync(pingPayload);

We should consume the task with LogExceptions(SendPingAsync(pingPayload)) as we do when sending out pings normally.

Should be an easy fix - marking as up for grabs.

Author: MihaZupan
Assignees: -
Labels:

bug, area-System.Net.Http, up-for-grabs
Milestone: 7.0.0

@MihaZupan MihaZupan removed the untriaged New issue has not been triaged by the area owner label Jan 28, 2022
@shubhanshu02
Copy link
Contributor

Hi @MihaZupan, can I take this up? If I understood correct, all that's needed to do is to wrap the function SendPingAsync with LogExceptions so that the exceptions (if occurred) are caught and logged?

@MihaZupan
Copy link
Member Author

That's right. I'll assign the issue to you :)

@shubhanshu02 shubhanshu02 mentioned this issue Jan 29, 2022
Merged
@ghost ghost added the in-pr There is an active PR which will close this issue when it is merged label Jan 29, 2022
@shubhanshu02
Copy link
Contributor

Thanks, I have put up a PR regarding this.

@ghost ghost removed the in-pr There is an active PR which will close this issue when it is merged label Jan 30, 2022
MihaZupan pushed a commit that referenced this issue Jan 30, 2022
@shubhanshu02
Fix UnobservedTaskException on Http2's SendPingAsync (#64494)
ce15dc7 
Wrap the SendPingAsync function with LogExceptions in Http2Connection.cs
to avoid any exception here being caught as an UnobservedTaskException.

Fix #64450

Signed-off-by: Shubhanshu Saxena <shubhanshu.e01@gmail.com>
@ghost ghost locked as resolved and limited conversation to collaborators Mar 1, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Assignees

@shubhanshu02 shubhanshu02

Labels
area-System.Net.Http bug help wanted [up-for-grabs] Good issue for external contributors
Projects
None yet
Milestone
7.0.0
Development

Successfully merging a pull request may close this issue.

Fix UnobservedTaskException on Http2's SendPingAsync shubhanshu02/runtime
2 participants
@MihaZupan @shubhanshu02