Try to avoid ObjectDisposedException when writing output

[grendello]

It may happen that the contents stream in the
WriteRequestContentToOutput method is disposed while asynchronously
copying content to the connection output stream. This will then cause
the ObjectDisposedException exception to be thrown when seeking back
to the beginning of the stream.

Try to avoid the problem by checking whether cancellation was requested
during the call to CopyToAsync and, if we get past that point, catch
the ObjectDisposedException to report it but not to rethrow it.

[jonpryor]

Context: https://discord.com/channels/732297728826277939/732297837953679412/1195284521378123836

…i keep getting.

System.IO.MemoryStream.CopyToAsync(Stream , Int32 , CancellationToken )
System.ObjectDisposedException: ObjectDisposed_StreamClosed

[the following call stack is sent to] my appcenter from users
with stacktrace.

System.IO.MemoryStream.CopyToAsync(Stream , Int32 , CancellationToken )
Xamarin.Android.Net.AndroidMessageHandler.WriteRequestContentToOutput(HttpRequestMessage , HttpURLConnection , CancellationToken )
Xamarin.Android.Net.AndroidMessageHandler.DoProcessRequest(HttpRequestMessage , URL , HttpURLConnection , CancellationToken , RequestRedirectionState )
Xamarin.Android.Net.AndroidMessageHandler.DoSendAsync(HttpRequestMessage , CancellationToken )
System.Net.Http.HttpClient.<SendAsync>g__Core|83_0(HttpRequestMessage , HttpCompletionOption , CancellationTokenSource , Boolean , CancellationTokenSource , CancellationToken )
MayApp.RestManager.ExecuteRest(String RequestType, StringContent Jsonmodel, Nullable`1 retry)

[jonpryor]

Context: https://github.com/dotnet/runtime/blob/release/8.0/src/libraries/System.Private.CoreLib/src/System/IO/MemoryStream.cs#L471

        public override Task CopyToAsync(Stream destination, int bufferSize, CancellationToken cancellationToken)
        {
            // This implementation offers better performance compared to the base class version.

            ValidateCopyToArguments(destination, bufferSize);
            EnsureNotClosed();

EnsureNotClosed() throws ObjectDissposedException() if the stream isn't open.

[jonpryor]

Context: https://github.com/dotnet/runtime/blob/release/8.0/src/libraries/System.Private.CoreLib/src/System/IO/Stream.cs#L996-L1011

Stream.ValidateCopyToArguments(*stream, int) throws ObjectDisposedException with ObjectDisposed_StreamClosed if destination is closed, i.e. if httpConnection.OutputStream! has been disposed, ObjectDisposedException can also be thrown from MemoryStream.CopyToAsync()!

[jonpryor]

This feels a tad odd: we're in a catch block, therefore an exception was thrown and caught, but if (for some reason) cancellationToken.ThrowIfCancellationRequested() doesn't throw, then…we continue execution? That doesn't feel right. Is potentially swallowing ObjectDisposedException and doing nothing more really appropriate?

[grendello]

I think it's fine, since the method returns a Task and, thus, all the exceptions are observable - but it might not be the case if the thread is being cancelled.

[jonpryor]

Given this observation, perhaps we should/also check httpConnection.OutputStream.CanWrite.

If httpConnection.OutputStream.CanWrite is false -- possibly because it was disposed, possibly because The Gods Must Be Crazy -- then What Should We Do? Throw an exception? Return a failed Task?

[grendello]

Checking CanWrite only tells us whether the stream is capable of writing, not whether it's able to do so. IOW, in many cases in the BCL a true returned from the property is not influenced by the stream state:

dotnet/runtime/src/libraries/System.Net.Http/src/System/Net/Http $ git grep -n 'bool CanWrite'
BrowserHttpHandler/BrowserHttpHandler.cs:391:        public override bool CanWrite => true;
BrowserHttpHandler/BrowserHttpHandler.cs:542:        public override bool CanWrite => false;
EmptyReadStream.cs:17:        public override bool CanWrite => false;
HttpContent.cs:1053:            public override bool CanWrite => true;
MultipartContent.cs:451:            public override bool CanWrite => false;
SocketsHttpHandler/DecompressionHandler.cs:253:                public override bool CanWrite => false;
SocketsHttpHandler/DecompressionHandler.cs:328:                    public override bool CanWrite => false;
SocketsHttpHandler/Http2Stream.cs:1438:                public override bool CanWrite => false;
SocketsHttpHandler/Http2Stream.cs:1530:                public override bool CanWrite => _http2Stream != null;
SocketsHttpHandler/Http3RequestStream.cs:1354:            public override bool CanWrite => false;
SocketsHttpHandler/Http3RequestStream.cs:1446:            public override bool CanWrite => _stream != null;
SocketsHttpHandler/HttpContentReadStream.cs:21:            public sealed override bool CanWrite => false;
SocketsHttpHandler/HttpContentWriteStream.cs:21:            public sealed override bool CanWrite => _connection != null;
SocketsHttpHandler/RawConnectionStream.cs:22:            public sealed override bool CanWrite => _connection != null;
StreamContent.cs:153:            public override bool CanWrite => false;

While the content streams do change the value of the property based on (portions) of their state, I don't think we should (and can) rely on that value, as per docs:

When overridden in a derived class, gets a value indicating whether the current stream supports writing.

I read the "supports" as "is capable of" and not "is able to right now".

[jonpryor]

I think "the current stream" is the important clause. Consider code such as:

if (stream.CanWrite) {
    stream.Write(…);
}

if that stream.Write() will throw, then what's the point of the stream.CanWrite check? (Sure, it can't check everything, e.g. the network might suddenly disappear or your disk runs out of space, so stream.Write(…) may throw. However, if you know that all writes will fail, then stream.CanWrite should return false!)

[jonpryor]

I think this PR existed in order to attempt to address dotnet/runtime#115611.

dotnet/runtime#115611 increasingly looks like a GC bug; see also:

• Fix an edge case in the Tarjan GC bridge that leads to losing xref information runtime#112825

A workaround for the issue has also been posted:

• HttpClient ObjectDisposed after SDK upgrade from 34.0.95 -> 34.0.113 runtime#115611

Given the above, do we still want this PR?