Skip to content

Try to avoid ObjectDisposedException when writing output #8632

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

Closed
wants to merge 5 commits into from
Closed

Try to avoid ObjectDisposedException when writing output #8632

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
3b895d4
Try to avoid `ObjectDisposedException` when writing output
grendello Jan 12, 2024
b42a248
Add an extra try/catch and update messages
grendello Jan 12, 2024
34c89a8
Merge branch 'main' into dev/grendel/http-client-check-stream-open
grendello Jan 12, 2024
7bfec48
More cancellation checks
grendello Jan 12, 2024
bb425e8
Merge branch 'main' into dev/grendel/http-client-check-stream-open
grendello Mar 20, 2024
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
19 changes: 16 additions & 3 deletions src/Mono.Android/Xamarin.Android.Net/AndroidClientHandler.Legacy.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -361,7 +361,13 @@ Task ConnectAsync (HttpURLConnection httpConnection, CancellationToken ct)
protected virtual async Task WriteRequestContentToOutput (HttpRequestMessage request, HttpURLConnection httpConnection, CancellationToken cancellationToken)
{
using (var stream = await request.Content.ReadAsStreamAsync ().ConfigureAwait (false)) {
await stream.CopyToAsync(httpConnection.OutputStream!, 4096, cancellationToken).ConfigureAwait(false);
cancellationToken.ThrowIfCancellationRequested ();
try {
await stream.CopyToAsync(httpConnection.OutputStream!, 4096, cancellationToken).ConfigureAwait(false);
Copy link
Contributor

Choose a reason for hiding this comment

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

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?

Copy link
Contributor Author

@grendello grendello Feb 16, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

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".

Copy link
Contributor

Choose a reason for hiding this comment

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

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!)

} catch (ObjectDisposedException ex) {
Logger.Log (LogLevel.Error, LOG_APP, $"Stream disposed while copying the content stream to output: {ex}");
cancellationToken.ThrowIfCancellationRequested ();
Copy link
Contributor

Choose a reason for hiding this comment

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

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?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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.

}

//
// Rewind the stream to beginning in case the HttpContent implementation
Expand All @@ -381,8 +387,15 @@ protected virtual async Task WriteRequestContentToOutput (HttpRequestMessage req
//
// See https://bugzilla.xamarin.com/show_bug.cgi?id=55477
//
if (stream.CanSeek)
stream.Seek (0, SeekOrigin.Begin);
if (stream.CanSeek) {
cancellationToken.ThrowIfCancellationRequested ();
try {
stream.Seek (0, SeekOrigin.Begin);
} catch (ObjectDisposedException ex) {
Logger.Log (LogLevel.Error, LOG_APP, $"Stream disposed while seeking to the beginning of the content stream: {ex}");
cancellationToken.ThrowIfCancellationRequested ();
}
}
}
}

Expand Down
60 changes: 37 additions & 23 deletions src/Mono.Android/Xamarin.Android.Net/AndroidMessageHandler.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -569,29 +569,43 @@ protected virtual async Task WriteRequestContentToOutput (HttpRequestMessage req
if (request.Content is null)
return;

var stream = await request.Content.ReadAsStreamAsync ().ConfigureAwait (false);
await stream.CopyToAsync(httpConnection.OutputStream!, 4096, cancellationToken).ConfigureAwait(false);

//
// Rewind the stream to beginning in case the HttpContent implementation
// will be accessed again (e.g. after redirect) and it keeps its stream
// open behind the scenes instead of recreating it on the next call to
// ReadAsStreamAsync. If we don't rewind it, the ReadAsStreamAsync
// call above will throw an exception as we'd be attempting to read an
// already "closed" stream (that is one whose Position is set to its
// end).
//
// This is not a perfect solution since the HttpContent may do weird
// things in its implementation, but it's better than copying the
// content into a buffer since we have no way of knowing how the data is
// read or generated and also we don't want to keep potentially large
// amounts of data in memory (which would happen if we read the content
// into a byte[] buffer and kept it cached for re-use on redirect).
//
// See https://bugzilla.xamarin.com/show_bug.cgi?id=55477
//
if (stream.CanSeek)
stream.Seek (0, SeekOrigin.Begin);
using (var stream = await request.Content.ReadAsStreamAsync ().ConfigureAwait (false)) {
cancellationToken.ThrowIfCancellationRequested ();
try {
await stream.CopyToAsync(httpConnection.OutputStream!, 4096, cancellationToken).ConfigureAwait(false);
} catch (ObjectDisposedException ex) {
Logger.Log (LogLevel.Error, LOG_APP, $"Stream disposed while copying the content stream to output: {ex}");
cancellationToken.ThrowIfCancellationRequested ();
}

//
// Rewind the stream to beginning in case the HttpContent implementation
// will be accessed again (e.g. after redirect) and it keeps its stream
// open behind the scenes instead of recreating it on the next call to
// ReadAsStreamAsync. If we don't rewind it, the ReadAsStreamAsync
// call above will throw an exception as we'd be attempting to read an
// already "closed" stream (that is one whose Position is set to its
// end).
//
// This is not a perfect solution since the HttpContent may do weird
// things in its implementation, but it's better than copying the
// content into a buffer since we have no way of knowing how the data is
// read or generated and also we don't want to keep potentially large
// amounts of data in memory (which would happen if we read the content
// into a byte[] buffer and kept it cached for re-use on redirect).
//
// See https://bugzilla.xamarin.com/show_bug.cgi?id=55477
//
if (stream.CanSeek) {
cancellationToken.ThrowIfCancellationRequested ();
try {
stream.Seek (0, SeekOrigin.Begin);
} catch (ObjectDisposedException ex) {
Logger.Log (LogLevel.Error, LOG_APP, $"Stream disposed while seeking to the beginning of the content stream: {ex}");
cancellationToken.ThrowIfCancellationRequested ();
}
}
}
}

internal Task WriteRequestContentToOutputInternal (HttpRequestMessage request, HttpURLConnection httpConnection, CancellationToken cancellationToken)
Expand Down