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 3 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
2 changes: 1 addition & 1 deletion src/libraries/System.Net.Http.WinHttpHandler/src/System/Net/Http/WinHttpRequestState.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -150,7 +150,7 @@ public WinHttpTransportContext TransportContext

public RendezvousAwaitable<int> LifecycleAwaitable { get; set; } = new RendezvousAwaitable<int>();
public TaskCompletionSource<bool>? TcsInternalWriteDataToRequestStream { get; set; }
public bool AsyncReadInProgress { get; set; }
public volatile int AsyncReadInProgress;

// WinHttpResponseStream state.
public long? ExpectedBytesToRead { get; set; }
Expand Down
40 changes: 17 additions & 23 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,15 @@ 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;
// Check that there are no other pending read operations
if (Interlocked.CompareExchange(ref _state.AsyncReadInProgress, 1, 0) == 1)
{
throw new InvalidOperationException(SR.net_http_no_concurrent_io_allowed);
}
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
{
using var ctr = cancellationToken.Register(s => ((WinHttpResponseStream)s!).CancelPendingResponseStreamReadOperation(), this);
_state.PinReceiveBuffer(buffer);
// Loop until there's no more data to be read
while (true)
{
Expand Down Expand Up @@ -163,8 +161,7 @@ private async Task CopyToAsyncCore(Stream destination, byte[] buffer, Cancellati
}
finally
{
_state.AsyncReadInProgress = false;
ctr.Dispose();
_state.AsyncReadInProgress = 0;
ArrayPool<byte>.Shared.Return(buffer);
}

Expand Down Expand Up @@ -201,11 +198,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 +213,15 @@ 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);
_state.AsyncReadInProgress = true;
// Check that there are no other pending read operations
if (Interlocked.CompareExchange(ref _state.AsyncReadInProgress, 1, 0) == 1)
{
throw new InvalidOperationException(SR.net_http_no_concurrent_io_allowed);
}
try
{
using var ctr = token.Register(s => ((WinHttpResponseStream)s!).CancelPendingResponseStreamReadOperation(), this);
_state.PinReceiveBuffer(buffer);
lock (_state.Lock)
{
Debug.Assert(!_requestHandle.IsInvalid);
Expand Down Expand Up @@ -262,8 +257,7 @@ private async Task<int> ReadAsyncCore(byte[] buffer, int offset, int count, Canc
}
finally
{
_state.AsyncReadInProgress = false;
ctr.Dispose();
_state.AsyncReadInProgress = 0;
}
}

Expand Down Expand Up @@ -357,7 +351,7 @@ private void CancelPendingResponseStreamReadOperation()
{
lock (_state.Lock)
{
if (_state.AsyncReadInProgress)
if (_state.AsyncReadInProgress == 1)
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 confused by the comment on this function. What happens if we Dispose of the handle even if there's not a pending request? There are race conditions here, both with and before this change.

Copy link
Member Author

@ManickaP ManickaP Jan 24, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

Honestly, I don't understand it either (it's been there since 2016: 87cf76a).
If we close the handle, all subsequent calls on it will obviously blow up with ODE or some such exception from the invalid handle.
Before this change, we set the AsyncReadInProgress after the token registration. So the only thing that comes to mind is to prevent pre-cancelled token to kill the handle if the read itself hasn't started yet. But I'm grasping at straws here... I can remove that part of the comment.

One thing I noticed though is that for the sending side, we're not locking around the handle dispose (upon token cancellation) which we should. All calls to WinHTTP for one request handle should be serialized. So I'll fix that as well.

{
if (NetEventSource.Log.IsEnabled()) NetEventSource.Info("before dispose");
_requestHandle?.Dispose(); // null check necessary to handle race condition between stream disposal and cancellation
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