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

[WinHTTP] Make concurrent IO check thread safe #111750

Merged
merged 8 commits into from
Jan 27, 2025
Merged

[WinHTTP] Make concurrent IO check thread safe #111750

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
8 commits
Select commit Hold shift + click to select a range
dce53ff
Check for concurrent IO is now thread safe.
ManickaP Jan 23, 2025
3c9b2a6
Add test
ManickaP Jan 23, 2025
508cb3c
Use compare exchange, fix cancellation token registration
ManickaP Jan 23, 2025
b41362a
Typo
ManickaP Jan 24, 2025
5e64c61
Fixed unit tests.
ManickaP Jan 24, 2025
85479e6
Test exception narrow down
ManickaP Jan 24, 2025
e62290b
Narrow down the exception
ManickaP Jan 24, 2025
26bd042
Don't dispose handle outside of lock
ManickaP Jan 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
36 changes: 20 additions & 16 deletions src/libraries/System.Net.Http.WinHttpHandler/src/System/Net/Http/WinHttpResponseStream.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -94,12 +94,6 @@ public override Task CopyToAsync(Stream destination, int bufferSize, Cancellatio
// Validate arguments as would base CopyToAsync
StreamHelpers.ValidateCopyToArgs(this, destination, bufferSize);

// Check that there are no other pending read operations
if (_state.AsyncReadInProgress)
{
throw new InvalidOperationException(SR.net_http_no_concurrent_io_allowed);
}

// Early check for cancellation
if (cancellationToken.IsCancellationRequested)
{
Expand All @@ -112,11 +106,19 @@ public override Task CopyToAsync(Stream destination, int bufferSize, Cancellatio

private async Task CopyToAsyncCore(Stream destination, byte[] buffer, CancellationToken cancellationToken)
{
_state.PinReceiveBuffer(buffer);
CancellationTokenRegistration ctr = cancellationToken.Register(s => ((WinHttpResponseStream)s!).CancelPendingResponseStreamReadOperation(), this);
_state.AsyncReadInProgress = true;
lock (_state.Lock)
{
// Check that there are no other pending read operations
if (_state.AsyncReadInProgress)
{
throw new InvalidOperationException(SR.net_http_no_concurrent_io_allowed);
}
_state.AsyncReadInProgress = true;
}
stephentoub marked this conversation as resolved.
Show resolved Hide resolved
Copy link
Member

Choose a reason for hiding this comment

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

We'll end up not returning the rented buffer in this case, but that's fine; this only happens in incorrectly structured code, and in general we're ok with array pool buffers not being returned in exceptional situations.

try
{
_state.PinReceiveBuffer(buffer);
// Loop until there's no more data to be read
while (true)
{
Expand Down Expand Up @@ -201,11 +203,6 @@ public override Task<int> ReadAsync(byte[] buffer, int offset, int count, Cancel

CheckDisposed();

if (_state.AsyncReadInProgress)
{
throw new InvalidOperationException(SR.net_http_no_concurrent_io_allowed);
}

return ReadAsyncCore(buffer, offset, count, token);
}

Expand All @@ -221,12 +218,19 @@ private async Task<int> ReadAsyncCore(byte[] buffer, int offset, int count, Canc
{
return 0;
}

_state.PinReceiveBuffer(buffer);
var ctr = token.Register(s => ((WinHttpResponseStream)s!).CancelPendingResponseStreamReadOperation(), this);
Copy link
Member

Choose a reason for hiding this comment

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

If there is a read in progress, and we now throw an exception outside of the try block, this will leak into the cancellation token.

_state.AsyncReadInProgress = true;
lock (_state.Lock)
{
// Check that there are no other pending read operations
if (_state.AsyncReadInProgress)
{
throw new InvalidOperationException(SR.net_http_no_concurrent_io_allowed);
}
_state.AsyncReadInProgress = true;
}
try
{
_state.PinReceiveBuffer(buffer);
lock (_state.Lock)
{
Debug.Assert(!_requestHandle.IsInvalid);
Expand Down
23 changes: 23 additions & 0 deletions src/libraries/System.Net.Http.WinHttpHandler/tests/FunctionalTests/WinHttpHandlerTest.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -46,6 +46,29 @@ public void SendAsync_SimpleGet_Success()
}
}

[OuterLoop("Uses external server.")]
[Fact]
public async Task GetAsync_ConcurrentRead_ThrowsInvalidOperationException()
Copy link
Member

Choose a reason for hiding this comment

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

I'm unclear what this test is validating. Without your changes, won't this still generally pass? Is this a stress test masquerading as a unit test?

Copy link
Member Author

Choose a reason for hiding this comment

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

Without your changes, won't this still generally pass?

Nope, I'm getting asserts in the awaitable:

 Process terminated. Assertion failed.
  !_resultSet && _error == null
     at System.Threading.Tasks.RendezvousAwaitable`1.AssertResultConsistency(Boolean expectedCompleted) in C:\Users\mapichov\Repositories\runtime\src\libraries\Common\src\System\Threading\Tasks\RendezvousAwaitable.cs:line 149
     at System.Threading.Tasks.RendezvousAwaitable`1.SetResult(TResult result) in C:\Users\mapichov\Repositories\runtime\src\libraries\Common\src\System\Threading\Tasks\RendezvousAwaitable.cs:line 79
     at System.Net.Http.WinHttpRequestCallback.OnRequestDataAvailable(WinHttpRequestState state, Int32 bytesAvailable) in C:\Users\mapichov\Repositories\runtime\src\libraries\System.Net.Http.WinHttpHandler\src\System\Net\Http\WinHttpRequestCallback.cs:line 146
     at System.Net.Http.WinHttpRequestCallback.RequestCallback(WinHttpRequestState state, UInt32 internetStatus, IntPtr statusInformation, UInt32 statusInformationLength) in C:\Users\mapichov\Repositories\runtime\src\libraries\System.Net.Http.WinHttpHandler\src\Syste
  m\Net\Http\WinHttpRequestCallback.cs:line 70
     at System.Net.Http.WinHttpRequestCallback.WinHttpCallback(IntPtr handle, IntPtr context, UInt32 internetStatus, IntPtr statusInformation, UInt32 statusInformationLength) in C:\Users\mapichov\Repositories\runtime\src\libraries\System.Net.Http.WinHttpHandler\src\S
  ystem\Net\Http\WinHttpRequestCallback.cs:line 46

as well as unexpected exception (I had to narrow down the catch block to filter only concurrent IO errors):

 System.InvalidOperationException : Handle is not initialized.
        Stack Trace:
             at System.Runtime.InteropServices.GCHandle.Free()
          C:\Users\mapichov\Repositories\runtime\src\libraries\System.Net.Http.WinHttpHandler\src\System\Net\Http\WinHttpRequestState.cs(168,0): at System.Net.Http.WinHttpRequestState.PinReceiveBuffer(Byte[] buffer)
          C:\Users\mapichov\Repositories\runtime\src\libraries\System.Net.Http.WinHttpHandler\src\System\Net\Http\WinHttpResponseStream.cs(225,0): at System.Net.Http.WinHttpResponseStream.<ReadAsyncCore>d__24.MoveNext()
          --- End of stack trace from previous location where exception was thrown ---
             at System.Runtime.ExceptionServices.ExceptionDispatchInfo.Throw()
             at System.Runtime.CompilerServices.TaskAwaiter.HandleNonSuccessAndDebuggerNotification(Task task)
             at System.Runtime.CompilerServices.ValueTaskAwaiter`1.GetResult()
          C:\Users\mapichov\Repositories\runtime\src\libraries\System.Net.Http.WinHttpHandler\tests\FunctionalTests\WinHttpHandlerTest.cs(49,0): at System.Net.Http.WinHttpHandlerFunctional.Tests.WinHttpHandlerTest.<>c__DisplayClass3_0.<<GetAsync_ConcurrentRead_Throws
  InvalidOperationException>b__0>d.MoveNext()

The test is in outerloop, so it won't run with each PR. But I can remove the test, I'm not insisting on having it.

Copy link
Member

Choose a reason for hiding this comment

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

That's what I meant about it being a stress test, though. It's trying to inflict certain interleavings by running a lot. If it's reasonably fast, though, OK.

Copy link
Member Author

Choose a reason for hiding this comment

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

~ 0.3-0.5 s in our CI, not the fastest, but we have much slower tests in outerloop running for 1-3s.

{
using var client = new HttpClient(new WinHttpHandler());
using var response = await client.GetAsync("https://httpbin.org/stream-bytes/4096", HttpCompletionOption.ResponseHeadersRead);
using var stream = await response.Content.ReadAsStreamAsync();
var tasks = new Task[1_000];
for (int i = 0; i < tasks.Length; ++i)
{
tasks[i] = Task.Run(async () =>
{
try
{
await stream.ReadAsync(new byte[5]);
}
catch (InvalidOperationException) // Expected exception for concurrent IO
{ }
});
}
await Task.WhenAll(tasks);
}

[OuterLoop]
[Theory]
[InlineData(CookieUsePolicy.UseInternalCookieStoreOnly, "cookieName1", "cookieValue1")]
Expand Down
Loading