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

Expect 100 Continue Hang #38774

Merged
merged 12 commits into from
Jul 28, 2020
Merged

Expect 100 Continue Hang #38774

merged 12 commits into from
Jul 28, 2020

Conversation

ManickaP
Copy link
Member

@ManickaP ManickaP commented Jul 3, 2020

Inside sendRequestContentTask recognizes that it's invoked from a timer thread and if request content fails, unblocks SendAsyncCore and eventually propagates the exception from the content to the outside.

Fixes #36717

@ManickaP ManickaP requested a review from a team July 3, 2020 21:22
@ManickaP ManickaP force-pushed the mapichov/36717_expect100continue branch 3 times, most recently from 5a90ae0 to 9bcbf28 Compare July 4, 2020 15:04
@wfurt
Copy link
Member

wfurt commented Jul 7, 2020

test failure seems related

@wfurt
Copy link
Member

wfurt commented Jul 7, 2020

Why this is different than case without 100Continue? If server does not respond at all, we would also block until timeout fires, right?

@ManickaP
Copy link
Member Author

ManickaP commented Jul 7, 2020

Why this is different than case without 100Continue? If server does not respond at all, we would also block until timeout fires, right?

The hang happens only if we fail to send request content after expect100Continue timer expires. When the timer expires, we try to send the content anyway (without 100 from the server), but it fails (due to exception in custom HttpContent). At that moment, in non-100 scenario, we would have already reported that exception to the caller (content gets send before reading response headers). In 100 we don't, we wait for the timeout and eventually report timeout instead of the error in the content. Also, if the content sending didn't fail, the server would reply and we wouldn't end up with timeout.

For customers, the covering the content exception with timeout is making this issue supper hard to troubleshoot.
And since we added sync scenario and throwing in case of missing sync overrides, this hang is now much more probable to happen.

@ManickaP ManickaP force-pushed the mapichov/36717_expect100continue branch from 9bcbf28 to 05d4cad Compare July 7, 2020 14:39
stephentoub
stephentoub reviewed Jul 7, 2020
View reviewed changes
src/libraries/System.Net.Http/src/System/Net/Http/SocketsHttpHandler/HttpConnection.cs Outdated
{
await SendRequestContentAsync(request, stream, async, cancellationToken).ConfigureAwait(false);
}
catch (Exception) when (timerExpired)
Copy link
Member

Choose a reason for hiding this comment

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

We're adding a decent amount of complication to be able to pass around both sendRequestContent and timerExpired. Could we instead just always call Dispose in this case if SendRequestContentAsync throws? As you said, we're talking about the combination of a faulty request content + Expect: 100-continue. If we get an exception in that case, regardless of the timer, seems like it'd be fine to just get rid of the connection in that case.

@ManickaP ManickaP force-pushed the mapichov/36717_expect100continue branch from 05d4cad to d674782 Compare July 7, 2020 20:58
@ghost
Copy link

ghost commented Jul 9, 2020

Tagging subscribers to this area: @dotnet/ncl
Notify danmosemsft if you want to be subscribed.

alnikola
alnikola approved these changes Jul 11, 2020
View reviewed changes
Copy link
Contributor

@alnikola alnikola left a comment

Choose a reason for hiding this comment

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

LGTM except for some small issues commented on below.

src/libraries/System.Net.Http/src/System/Net/Http/SocketsHttpHandler/HttpConnection.cs Show resolved Hide resolved
src/libraries/Common/tests/System/Net/Http/HttpClientHandlerTest.cs Outdated Show resolved Hide resolved
src/libraries/Common/tests/System/Net/Http/HttpClientHandlerTest.cs Outdated Show resolved Hide resolved
@ManickaP ManickaP force-pushed the mapichov/36717_expect100continue branch from cbfb705 to 5826443 Compare July 18, 2020 21:02
@ManickaP
Copy link
Member Author

Failing tests are unrelated.

scalablecory
scalablecory approved these changes Jul 28, 2020
View reviewed changes
Copy link
Contributor

@scalablecory scalablecory left a comment

Choose a reason for hiding this comment

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

lgtm

src/libraries/System.Net.Http/tests/FunctionalTests/ThrowingContent.netcore.cs
{
public partial class ThrowingContent : HttpContent
{
protected override void SerializeToStream(Stream stream, TransportContext context, CancellationToken cancellationToken)
Copy link
Contributor

Choose a reason for hiding this comment

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

Is this needed?

Shouldn't the default SerializeToStream(stream, context, cancellationToken) end up calling SerializeToStream(stream, context)?

Copy link
Member Author

Choose a reason for hiding this comment

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

TL;DR yes

This is the only sync implementation we provide for ThrowingContent. The one in shared code is async and there's now automatic/default sync-over-async in any of our HttpContent classes.

@ManickaP
Copy link
Member Author

triggering CI

@ManickaP ManickaP closed this Jul 28, 2020
@ManickaP ManickaP reopened this Jul 28, 2020
@ManickaP ManickaP merged commit 7a6ca80 into dotnet:master Jul 28, 2020
@ManickaP ManickaP deleted the mapichov/36717_expect100continue branch July 28, 2020 14:58
@jaredpar jaredpar mentioned this pull request Jul 28, 2020
Closed
@ManickaP ManickaP mentioned this pull request Jul 29, 2020
Merged
Jacksondr5 pushed a commit to Jacksondr5/runtime that referenced this pull request Aug 10, 2020
@ManickaP @Jacksondr5
Expect 100 Continue Hang (dotnet#38774)
6a379bb 
Inside sendRequestContentTask recognizes that it's invoked from a timer thread and if request content fails, unblocks SendAsyncCore and eventually propagates the exception from the content to the outside.
Fixes the issue for H2 as well.

Fixes dotnet#36717
@karelz karelz added this to the 5.0.0 milestone Aug 18, 2020
@ManickaP ManickaP mentioned this pull request Sep 3, 2020
Merged
@github-actions github-actions bot mentioned this pull request Sep 9, 2020
Merged
@ManickaP ManickaP mentioned this pull request Sep 18, 2020
Closed
@ghost ghost locked as resolved and limited conversation to collaborators Dec 8, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@stephentoub stephentoub stephentoub left review comments

@alnikola alnikola alnikola approved these changes

@scalablecory scalablecory scalablecory approved these changes

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

Successfully merging this pull request may close these issues.

Faulty request HttpContent might causes a hang in expect 100 Continue scenario
7 participants
@ManickaP @wfurt @stephentoub @scalablecory @alnikola @karelz @Dotnet-GitSync-Bot