Runtime: Race condition in WebClient.IsBusy

[kevingosse]

Consider the following code:

webClient.DownloadStringAsync(new Uri(url));
while (webClient.IsBusy) ;
Console.WriteLine("Received response for client.DownloadStringAsync(Uri)");

webClient.DownloadStringAsync(new Uri(url), null);
while (webClient.IsBusy) ;
Console.WriteLine("Received response for client.DownloadStringAsync(Uri, Object)");

(note: I'm aware that this code is bad and dangerous, and I'm going to fix it. Still, the race condition could happen with more subtle workflows)

We have this code in a unit tests and saw some occasional failures:

System.NotSupportedException: WebClient does not support concurrent I/O operations.
    at System.Net.WebClient.StartOperation()
    at System.Net.WebClient.StartAsyncOperation(Object userToken)
    at System.Net.WebClient.DownloadStringAsync(Uri address, Object userToken)
    at Samples.WebRequest.RequestHelpers.SendWebClientRequests(Boolean tracingDisabled, String url, String requestContent) in D:\a\1\s\samples\Samples.WebRequest\RequestHelpers.cs:line 103

I believe this is caused by a race-condition in WebClient.

The IsBusy property is implemented as:

public bool IsBusy => _asyncOp != null;

Then the code executed at the end of the async operation is:

        private void InvokeOperationCompleted(AsyncOperation asyncOp, SendOrPostCallback callback, AsyncCompletedEventArgs eventArgs)
        {
            if (Interlocked.CompareExchange(ref _asyncOp, null, asyncOp) == asyncOp)
            {
                EndOperation();
                asyncOp.PostOperationCompleted(callback, eventArgs);
            }
        }

EndOperation decreases the counter that is used to detect nested operations. Therefore, there is a window of time during which IsBusy will return false (because _asyncOp is null) but the concurrent I/O check will fail (because EndOperation hasn't been executed yet).

I believe this can be fixed by updating the IsBusy property to also check the number of outstanding operations:

public bool IsBusy => _asyncOp != null || _callNesting > 0;

This would change the behavior slightly as the property currently only tracks asynchronous operations (with the change, it would track all operations). But this is consistent with the documentation, which doesn't mention the async/sync distinction:

true if the Web request is still in progress; otherwise false.

[msftbot[bot]]

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

[teo-tsirpanis]

Shouldn't we also make _asyncOp and _callNesting volatile?

[huoyaoyuan]

WebClient is going to be deprecated, so this should probably be a won't fix.

[benaadams]

Shouldn't we also make _asyncOp and _callNesting volatile?

Probably looks like it works on Framework but fails in Core; as IsBusy doesn't inline on Framework but does inline on Core so gets hoisted to register and never checked again for the the lifetime of loop.

https://twitter.com/KooKiz/status/1303714861617217536

[kevingosse]

Shouldn't we also make _asyncOp and _callNesting volatile?

Probably looks like it works on Framework but fails in Core; as IsBusy doesn't inline on Framework but does inline on Core so gets hoisted to register and never checked again for the the lifetime of loop.

https://twitter.com/KooKiz/status/1303714861617217536

Yeah. I think we didn't run into this additional issue in our original code because we use this loop in a very long method (300+ lines of code), so I'm guessing the JIT doesn't want to increase the size even further by inlining stuff. But for the general case, the field should definitely be volatile.

[teo-tsirpanis]

I've thought of something else: can we define IsBusy as _only_ _callNesting > 0 without checking _asyncOp (and of course make _callNesting volatile)?

[karelz]

Triage: Looks like weird use case for IsBusy. We would take the change - it seems fairly simple.
@kevingosse do you want to put up a PR? (for 6.0 aka master)
Thanks!

[kevingosse]

@teo-tsirpanis was faster than me 😛