Skip to content

Avoid unobserved ReadAheadTask exceptions in HttpConnection#80214

Merged
MihaZupan merged 6 commits intodotnet:mainfrom
MihaZupan:http-readaheadtask-rewrite
Feb 1, 2023
Merged

Avoid unobserved ReadAheadTask exceptions in HttpConnection#80214
MihaZupan merged 6 commits intodotnet:mainfrom
MihaZupan:http-readaheadtask-rewrite

Conversation

@MihaZupan
Copy link
Member

@MihaZupan MihaZupan commented Jan 4, 2023 •
edited
Loading

Closes #61256

This change adds synchronization between the read-ahead task and SendAsync to avoid a scenario where we don't observe the task's exception.

@MihaZupan MihaZupan added this to the 8.0.0 milestone Jan 4, 2023
@MihaZupan MihaZupan requested review from a team and stephentoub January 4, 2023 23:59
@MihaZupan MihaZupan self-assigned this Jan 4, 2023
@ghost
Copy link

ghost commented Jan 5, 2023

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

Issue Details

Closes #61256
Closes #60729

This changes the ReadAheadWithZeroByteReadAsync implementation to immediately react to read exceptions/premature data if we're not in the middle of a request.

To address #60729, we also issue the read-ahead once a connection becomes idle (gets returned to the pool with no remaining requests). If we don't want to do this it's a matter of deleting this.

Author: MihaZupan
Assignees: MihaZupan
Labels:

area-System.Net.Http
Milestone: 8.0.0

@MihaZupan MihaZupan force-pushed the http-readaheadtask-rewrite branch from c52af6a to ab437e3 Compare January 16, 2023 19:01
@MihaZupan MihaZupan force-pushed the http-readaheadtask-rewrite branch from ab437e3 to 47948da Compare January 26, 2023 21:59
@MihaZupan
Copy link
Member Author

MihaZupan commented Jan 26, 2023

Removed the proactive-EOF-reaction part of the PR now

@build-analysis build-analysis bot mentioned this pull request Jan 27, 2023
Closed
@MihaZupan
Copy link
Member Author

MihaZupan commented Jan 31, 2023

@stephentoub @ManickaP please take another look at this one when you get a chance, the change is smaller now.

stephentoub
stephentoub approved these changes Jan 31, 2023
View reviewed changes
src/libraries/System.Net.Http/src/System/Net/Http/SocketsHttpHandler/HttpConnection.cs
#pragma warning disable CA2012 // we're very careful to ensure the ValueTask is only consumed once, even though it's stored into a field
_readAheadTask ??= ReadAheadWithZeroByteReadAsync();
#pragma warning restore CA2012
EnsureReadAheadTaskHasStarted();
Copy link
Member

@stephentoub stephentoub Jan 31, 2023

Choose a reason for hiding this comment

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

Is it possible scavenges could end up overlapping such that two threads concurrently try to initialize the read-ahead? (Your PR wouldn't make that worse.)

Copy link
Member Author

@MihaZupan MihaZupan Jan 31, 2023

Choose a reason for hiding this comment

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

Shouldn't be as all the scavenging logic runs while holding the connection pool lock

@MihaZupan MihaZupan merged commit 120b721 into dotnet:main Feb 1, 2023
@ghost ghost locked as resolved and limited conversation to collaborators Mar 3, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Reviewers

@ManickaP ManickaP ManickaP left review comments

@stephentoub stephentoub stephentoub approved these changes

Assignees

@MihaZupan MihaZupan

Labels

area-System.Net.Http

Projects

None yet

Milestone

8.0.0

Development

Successfully merging this pull request may close these issues.

HttpConnection leaks UnobservedTaskExceptions

3 participants

@MihaZupan @stephentoub @ManickaP