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

Fix Hang on when GetRequestStreamAsync #113409

Draft
wants to merge 5 commits into
base: main
Choose a base branch
from
Draft

Fix Hang on when GetRequestStreamAsync #113409

Show file tree
Hide file tree
Changes from 4 commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
2607a60
Fix Hang on GetRequestStreamAsync in case underlying HttpClient.SendA…
liveans Mar 12, 2025
169cc5b
Update src/libraries/System.Net.Requests/src/System/Net/HttpWebReques…
liveans Mar 12, 2025
63b5bcb
Linux returns UnknownError for exception
liveans Mar 12, 2025
8612b61
Merge branch 'fix_httpwebrequest_nonbufferingonwrite_hang' of github.…
liveans Mar 12, 2025
f38057a
Merge branch 'main' into fix_httpwebrequest_nonbufferingonwrite_hang
liveans Mar 24, 2025
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
15 changes: 14 additions & 1 deletion src/libraries/System.Net.Requests/src/System/Net/HttpWebRequest.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1099,7 +1099,20 @@ private async Task<Stream> InternalGetRequestStream()
TaskCompletionSource<Stream> getStreamTcs = new();
TaskCompletionSource completeTcs = new();
_sendRequestTask = SendRequest(async: true, new RequestStreamContent(getStreamTcs, completeTcs));
_requestStream = new RequestStream(await getStreamTcs.Task.ConfigureAwait(false), completeTcs);
Task<Stream> getStreamTask = getStreamTcs.Task;
try
{
Task result = await Task.WhenAny((Task)getStreamTask, (Task)_sendRequestTask).WaitAsync(TimeSpan.FromMilliseconds(Timeout)).ConfigureAwait(false);
Copy link
Member Author

Choose a reason for hiding this comment

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

The one thing that I've noticed is that, when user is using async path already they should implement their own timeout mechanism according to the doc: HttpWebRequest.Timeout - Remarks

Not sure, if we should stick to that behavior and limit propagating of Timeout here.

Copy link
Member

Choose a reason for hiding this comment

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

WaitAsync will not cancel / stop the underlying tasks. What happens with them? I think this will at minimum generate unobserved exceptions.
It would be better to pass the timeout to the tasks themselves via cancellation token and them catch the exception and remap to TimeoutException if necessary. (we do this a lot around HttpClient).

Copy link
Member Author

Choose a reason for hiding this comment

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

Looks like the only thing what needed in here is: removing WaitAsync because handling of Timeout will be covered by underlying HttpClient timeout anyway.

There are few cases where getStreamTask can never be completed.
One of them is sendRequest is completed with cancellation or faulted, and it's already covered in this code and exception will be thrown.

Second thing is that: When we set Expect100Continue to true, sendRequestTask will return successfully before getStreamTask and if server says go away, we're going to end up waiting on getStreamTask forever, that's the case I need to figure out what framework does in that particular case.

if (result == _sendRequestTask && !_sendRequestTask.IsCompletedSuccessfully)
{
await _sendRequestTask.ConfigureAwait(false); // Propagate the exception
}
_requestStream = new RequestStream(await getStreamTask.ConfigureAwait(false), completeTcs);
}
catch (TimeoutException)
{
throw new WebException(SR.net_timeout, WebExceptionStatus.Timeout);
}
}
else
{
Expand Down
10 changes: 10 additions & 0 deletions src/libraries/System.Net.Requests/tests/HttpWebRequestTest.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2094,6 +2094,16 @@ await server.AcceptConnectionAsync(async connection =>
});
}

[Fact]
public async Task SendHttpPostRequest_BufferingDisabledWithInvalidHost_ShouldThrow()
{
HttpWebRequest request = WebRequest.CreateHttp("http://anything-unusable-blabla");
request.Method = "POST";
request.AllowWriteStreamBuffering = false;
WebException webException = await Assert.ThrowsAnyAsync<WebException>(() => request.GetRequestStreamAsync());
Assert.Equal(PlatformDetection.IsLinux ? WebExceptionStatus.UnknownError : WebExceptionStatus.NameResolutionFailure, webException.Status);
}

[Fact]
public async Task SendHttpPostRequest_BufferingDisabled_ConnectionShouldStartWithRequestStream()
{
Expand Down
Loading