Skip to content

Do not drain HttpContentReadStream if the connection is disposed #57287

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

Merged
antonfirsov merged 4 commits into from
Aug 16, 2021
Merged

Do not drain HttpContentReadStream if the connection is disposed #57287

Changes from 1 commit
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
d5cf967
do not drain HttpContentReadStream if the connection is disposed
antonfirsov Aug 12, 2021
9e8f5ba
move _connection checking to NeedsDrain
antonfirsov Aug 13, 2021
bf57a3d
safe check of _connection._disposed
antonfirsov Aug 13, 2021
57ed154
IsConnectionAlive -> CanReadFromConnection
antonfirsov Aug 13, 2021
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Next Next commit
do not drain HttpContentReadStream if the connection is disposed
  • Loading branch information
@antonfirsov
antonfirsov committed Aug 12, 2021
commit d5cf967a66280e5e2bf7699722930282609e0c05
3 changes: 2 additions & 1 deletion ...libraries/System.Net.Http/src/System/Net/Http/SocketsHttpHandler/HttpContentReadStream.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -46,7 +46,8 @@ protected override void Dispose(bool disposing)
return;
}

if (disposing && NeedsDrain)
// The connection might be disposed because of cancellation. We should not drain the response in this case.
if (disposing && NeedsDrain && _connection?._disposed != Status_Disposed)
Copy link
Member

@stephentoub stephentoub Aug 12, 2021

Choose a reason for hiding this comment

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

If _connection is null, it will still pass this check. Is that intended?

Also, is there something somewhere that prevents this transition immediately after this check happens?

Copy link
Contributor Author

@antonfirsov antonfirsov Aug 12, 2021 •
edited
Loading

Choose a reason for hiding this comment

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

If _connection is null, it will still pass this check. Is that intended?

No, I will fix this. (Although it doesn't cause any problems since _connection == null leads to NeedsDrain == false.)

Also, is there something somewhere that prevents this transition immediately after this check happens?

In the race I described in #56159 (comment) connection should be always disposed at this point, or won't be disposed later. I'm not aware of other cases. @geoffkizer @ManickaP thoughts?

Copy link
Member

@ManickaP ManickaP Aug 13, 2021

Choose a reason for hiding this comment

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

Also, is there something somewhere that prevents this transition immediately after this check happens?

Active prevention - I don't think so. But I also don't see any such transition in the code ATM.

Would a similar check for Status_Disposed help if it was added right before we return the connection to the pool? We're already checking for connection close there, so it might make sense...

Copy link
Contributor Author

@antonfirsov antonfirsov Aug 13, 2021 •
edited
Loading

Choose a reason for hiding this comment

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

I moved the _connection checking to NeedsDrain implementations in 9e8f5ba, I think the code looks more straightforward in the new form.

Regarding the second concern: if connection disposal could somehow kick in immediately after the disposal of HttpContentReadStream that could cause a problem. I don't see how to construct such a scenario, even if it's possible, it seems corner-casish to me.

The PR fixes a serious problem with a common Send(request, HttpCompletionOption.ResponseContentRead, cancellationToken) case as-is.

{
// Start the asynchronous drain.
// It may complete synchronously, in which case the connection will be put back in the pool synchronously.
Expand Down