[WinHTTP] Make concurrent IO check thread safe #111750

ManickaP · 2025-01-23T16:51:43Z

Note that this change makes the InvalidOperationException for concurrent IO async from sync. So now it's stored in the returned Task instead of being thrown synchronously.

Also, I can change this to Interlocked.CompareExchange. I went with the straight forward solution, for compare exchange, I'd have to change AsyncReadInProgress to either a method or a public field.

dotnet-policy-service · 2025-01-23T16:52:19Z

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

ManickaP · 2025-01-23T16:52:55Z

/azp run runtime-libraries-coreclr outerloop-windows

azure-pipelines · 2025-01-23T16:53:05Z

Azure Pipelines successfully started running 1 pipeline(s).

src/libraries/System.Net.Http.WinHttpHandler/src/System/Net/Http/WinHttpResponseStream.cs

stephentoub · 2025-01-23T16:57:44Z

src/libraries/System.Net.Http.WinHttpHandler/src/System/Net/Http/WinHttpResponseStream.cs

@@ -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);


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.

ManickaP · 2025-01-23T17:25:24Z

/azp run runtime-libraries-coreclr outerloop-windows

azure-pipelines · 2025-01-23T17:25:34Z

Azure Pipelines successfully started running 1 pipeline(s).

stephentoub · 2025-01-23T18:40:16Z

src/libraries/System.Net.Http.WinHttpHandler/src/System/Net/Http/WinHttpResponseStream.cs

+            if (Interlocked.CompareExchange(ref _state.AsyncReadInProgress, 1, 0) == 1)
+            {
+                throw new InvalidOperationException(SR.net_http_no_concurrent_io_allowed);
+            }


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.

stephentoub · 2025-01-23T18:42:53Z

src/libraries/System.Net.Http.WinHttpHandler/src/System/Net/Http/WinHttpResponseStream.cs

@@ -357,7 +351,7 @@ private void CancelPendingResponseStreamReadOperation()
        {
            lock (_state.Lock)
            {
-                if (_state.AsyncReadInProgress)
+                if (_state.AsyncReadInProgress == 1)


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.

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.

stephentoub · 2025-01-23T18:44:56Z

src/libraries/System.Net.Http.WinHttpHandler/tests/FunctionalTests/WinHttpHandlerTest.cs

@@ -46,6 +46,29 @@ public void SendAsync_SimpleGet_Success()
            }
        }

+        [OuterLoop("Uses external server.")]
+        [Fact]
+        public async Task GetAsync_ConcurrentRead_ThrowsInvalidOperationException()


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?

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.

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.

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

ManickaP · 2025-01-24T13:06:04Z

/azp run runtime-libraries-coreclr outerloop-windows

azure-pipelines · 2025-01-24T13:06:14Z

Azure Pipelines successfully started running 1 pipeline(s).

ManickaP · 2025-01-27T09:54:56Z

/azp run runtime-libraries-coreclr outerloop-windows

azure-pipelines · 2025-01-27T09:55:11Z

Azure Pipelines successfully started running 1 pipeline(s).

ManickaP · 2025-01-27T12:06:15Z

Outerloop failures unrelated.

ManickaP added 2 commits January 23, 2025 17:04

Check for concurrent IO is now thread safe.

dce53ff

Add test

3c9b2a6

dotnet-issue-labeler bot added the area-System.Net label Jan 23, 2025

dotnet-policy-service bot assigned ManickaP Jan 23, 2025

stephentoub reviewed Jan 23, 2025

View reviewed changes

src/libraries/System.Net.Http.WinHttpHandler/src/System/Net/Http/WinHttpResponseStream.cs Show resolved Hide resolved

stephentoub reviewed Jan 23, 2025

View reviewed changes

Use compare exchange, fix cancellation token registration

508cb3c

stephentoub reviewed Jan 23, 2025

View reviewed changes

stephentoub approved these changes Jan 23, 2025

View reviewed changes

build-analysis bot mentioned this pull request Jan 23, 2025

[wasm] Out of memory exception in System.Xml.XmlWriterApiTests.TCFullEndElement tests #110090

Open

ManickaP added 4 commits January 24, 2025 12:00

Typo

b41362a

Fixed unit tests.

5e64c61

Test exception narrow down

85479e6

Narrow down the exception

e62290b

build-analysis bot mentioned this pull request Jan 24, 2025

restarted. Azure DevOps can't recover from restarts. dotnet/dnceng#3879

Open

3 tasks

Don't dispose handle outside of lock

26bd042

ManickaP merged commit c47a53d into dotnet:main Jan 27, 2025
82 of 88 checks passed

ManickaP deleted the winhttp-concurent-io branch January 27, 2025 12:06

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[WinHTTP] Make concurrent IO check thread safe #111750

[WinHTTP] Make concurrent IO check thread safe #111750

ManickaP commented Jan 23, 2025

dotnet-policy-service bot commented Jan 23, 2025

ManickaP commented Jan 23, 2025

azure-pipelines bot commented Jan 23, 2025

stephentoub Jan 23, 2025

ManickaP commented Jan 23, 2025

azure-pipelines bot commented Jan 23, 2025

stephentoub Jan 23, 2025

stephentoub Jan 23, 2025

ManickaP Jan 24, 2025 •

edited

Loading

stephentoub Jan 23, 2025

ManickaP Jan 24, 2025

stephentoub Jan 24, 2025

ManickaP Jan 24, 2025

ManickaP commented Jan 24, 2025

azure-pipelines bot commented Jan 24, 2025

ManickaP commented Jan 27, 2025

azure-pipelines bot commented Jan 27, 2025

ManickaP commented Jan 27, 2025

[WinHTTP] Make concurrent IO check thread safe #111750

[WinHTTP] Make concurrent IO check thread safe #111750

Conversation

ManickaP commented Jan 23, 2025

dotnet-policy-service bot commented Jan 23, 2025

ManickaP commented Jan 23, 2025

azure-pipelines bot commented Jan 23, 2025

stephentoub Jan 23, 2025

Choose a reason for hiding this comment

ManickaP commented Jan 23, 2025

azure-pipelines bot commented Jan 23, 2025

stephentoub Jan 23, 2025

Choose a reason for hiding this comment

stephentoub Jan 23, 2025

Choose a reason for hiding this comment

ManickaP Jan 24, 2025 • edited Loading

Choose a reason for hiding this comment

stephentoub Jan 23, 2025

Choose a reason for hiding this comment

ManickaP Jan 24, 2025

Choose a reason for hiding this comment

stephentoub Jan 24, 2025

Choose a reason for hiding this comment

ManickaP Jan 24, 2025

Choose a reason for hiding this comment

ManickaP commented Jan 24, 2025

azure-pipelines bot commented Jan 24, 2025

ManickaP commented Jan 27, 2025

azure-pipelines bot commented Jan 27, 2025

ManickaP commented Jan 27, 2025

ManickaP Jan 24, 2025 •

edited

Loading