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

Do not drain HttpContentReadStream if the connection is disposed #57287

Merged
merged 4 commits into from
Aug 16, 2021
Merged

Do not drain HttpContentReadStream if the connection is disposed #57287

merged 4 commits into from
Aug 16, 2021

Conversation

antonfirsov
Copy link
Member

See #56159 (comment) for more details.

Fixes #56159

@ghost
Copy link

ghost commented Aug 12, 2021

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

Issue Details

See #56159 (comment) for more details.

Fixes #56159

Author: antonfirsov
Assignees: -
Labels:

area-System.Net.Http
Milestone: -

@antonfirsov

This comment has been minimized.

Sign in to view
@azure-pipelines

This comment has been minimized.

Sign in to view
@antonfirsov antonfirsov requested a review from a team August 12, 2021 15:18
@antonfirsov

This comment has been minimized.

Sign in to view
@azure-pipelines

This comment has been minimized.

Sign in to view
stephentoub
stephentoub reviewed Aug 12, 2021
View reviewed changes
src/libraries/System.Net.Http/src/System/Net/Http/SocketsHttpHandler/HttpContentReadStream.cs Outdated
@@ -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

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
Member 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

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
Member 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.

ManickaP
ManickaP reviewed Aug 13, 2021
View reviewed changes
Copy link
Member

@ManickaP ManickaP left a comment

Choose a reason for hiding this comment

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

Do we have any stress runs from this change?

Otherwise, LGTM.
I'll approve once we have some runs and the discussion is resolved. I don't have any qualms about the change as it is now.

@antonfirsov

This comment has been minimized.

Sign in to view
@azure-pipelines

This comment has been minimized.

Sign in to view
@antonfirsov
Copy link
Member Author

antonfirsov commented Aug 13, 2021 •
edited
Loading

Do we have any stress runs from this change?

I have 2 hours of local Linux runs with 0 errors (used to produce an error every 15 minutes). CI run with last changes is on the way.

ManickaP
ManickaP approved these changes Aug 13, 2021
View reviewed changes
Copy link
Member

@ManickaP ManickaP left a comment

Choose a reason for hiding this comment

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

I have 2 hours of local Linux runs with 0 errors

Awesome! Thank you for hunting this down and fixing it!

@antonfirsov
Copy link
Member Author

I meant if _connection might be concurrently nulled out between check and the immediate subsequent usage. I know in some places we read/write this field concurrently.

@stephentoub pushed changes to deal with such possibility.

@antonfirsov

This comment has been minimized.

Sign in to view
@azure-pipelines

This comment has been minimized.

Sign in to view
@antonfirsov

This comment has been minimized.

Sign in to view
@azure-pipelines

This comment has been minimized.

Sign in to view
@stephentoub
Copy link
Member

Do we have a way to write a deterministic test for this?

@antonfirsov
Copy link
Member Author

/azp run runtime-libraries stress-http

@azure-pipelines
Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@antonfirsov
Copy link
Member Author

I don't think it's possible to implement deterministic functional tests for this. What we need to validate here is that drain doesn't start in case of cancellation. We can not do this from server side since the connection is already closed when drain could possibly start. In fact, DrainAsync can only "drain" the connection's buffer which is an event we cannot hook into in functional tests.

We can probably invest some time in the future to enable unit testing such behaviors though.

/cc @geoffkizer

@antonfirsov
Copy link
Member Author

The only CI failure is HTTP/3 stress. Merging.

@antonfirsov antonfirsov merged commit a3612a4 into dotnet:main Aug 16, 2021
thaystg added a commit to thaystg/runtime that referenced this pull request Aug 16, 2021
@thaystg
Merge remote-tracking branch 'origin/main' into thays_cache_debugger_…
58cffee 
…information

# By dotnet-maestro[bot] (4) and others
# Via GitHub
* origin/main: (58 commits)
  Localized file check-in by OneLocBuild Task (dotnet#57384)
  [debugger][wasm] Support DebuggerProxyAttribute (dotnet#56872)
  Account for type mismatch of `FIELD_LIST` members in LSRA (dotnet#57450)
  Qualify `sorted_table` allocation with `nothrow` (dotnet#57467)
  Rename transport packages to follow convention (dotnet#57504)
  Generate proper DWARF reg num for ARM32 (dotnet#57443)
  Enable System.Linq.Queryable and disable dotnet#50712 (dotnet#57464)
  Mark individual tests for 51211 (dotnet#57463)
  Fix Length for ReadOnlySequence created out of sliced Memory owned by MemoryManager (dotnet#57479)
  Add JsonConverter.Write/ReadAsPropertyName APIs (dotnet#57302)
  Remove workaround for dotnet/sdk#19482 (dotnet#57453)
  Do not drain HttpContentReadStream if the connection is disposed (dotnet#57287)
  [mono] Fix a few corner case overflow operations (dotnet#57407)
  make use of ports in SPN optional (dotnet#57159)
  Fixed H/3 stress server after the last Kestrel change (dotnet#57356)
  disable a failing stress test. (dotnet#57473)
  Eliminate temporary byte array allocations in the static constructor of `IPAddress`. (dotnet#57397)
  Update dependencies from https://github.com/dotnet/emsdk build 20210815.1 (dotnet#57447)
  [main] Update dependencies from mono/linker (dotnet#57344)
  Improve serializer performance (dotnet#57327)
  ...

# Conflicts:
#	src/mono/wasm/debugger/BrowserDebugProxy/MemberReferenceResolver.cs
#	src/mono/wasm/debugger/BrowserDebugProxy/MonoProxy.cs
#	src/mono/wasm/debugger/BrowserDebugProxy/MonoSDBHelper.cs
@karelz karelz added this to the 6.0.0 milestone Aug 17, 2021
@antonfirsov antonfirsov mentioned this pull request Aug 24, 2021
Open
@ghost ghost locked as resolved and limited conversation to collaborators Sep 16, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@stephentoub stephentoub stephentoub approved these changes

@ManickaP ManickaP ManickaP approved these changes

Assignees
No one assigned
Labels
area-System.Net.Http
Projects
None yet
Milestone
6.0.0
Development

Successfully merging this pull request may close these issues.

Regression in HTTP/1.1 stress tests: ObjectDisposedException ('SslStream')
4 participants
@antonfirsov @stephentoub @ManickaP @karelz