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.

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

Non-catchable ObjectDisposedException when using NSUrlSessionHandler #20123

Open
tipa opened this issue Feb 15, 2024 · 9 comments
Open

Non-catchable ObjectDisposedException when using NSUrlSessionHandler #20123

tipa opened this issue Feb 15, 2024 · 9 comments
Labels
bug If an issue is a bug or a pull request a bug fix networking If an issue or pull request is related to networking
Milestone

Comments

@tipa
Copy link

tipa commented Feb 15, 2024

I am seeing a crash in one of my app when doing web requests and using an NSUrlSessionHandler.

var httpClient = new HttpClient(new NSUrlSessionHandler())

I have noticed this crash being reported for months already, but with the last update I have added a button to the apps UI that allows users to cancel web requests (using CancellationToken) and it appears like as if this change has increased the occurence of the exception even more. I have all my web requests in a try-catch clause, however this exception crashes my app nevertheless.

ObjectDisposed_StreamClosed (System.ObjectDisposedException)
   at System.ThrowHelper.ThrowObjectDisposedException_StreamClosed(String) + 0x3c
   at System.IO.MemoryStream.Read(Byte[], Int32, Int32) + 0x124
   at System.Net.Http.MultipartContent.ContentReadStream.Read(Byte[], Int32, Int32) + 0x78
   at System.Net.Http.NSUrlSessionHandler.WrappedNSInputStream.Read(IntPtr buffer, UIntPtr len) + 0x58
   at MyApp!<BaseAddress>+0x7082f8

According to Sentry, the crash happens in the com.apple.NSURLConnectionLoader thread (and that's also likely the reason I am unable to catch the exception)

Environment

.NET 8, NativeAot-compiled
iOS 17.3.1

@rolfbjarne
Copy link
Member

So there are two problems here:

  1. The exception itself, an ObjectDisposedException doesn't seem quite right, it's an indication something else is wrong. This might be related to App crash due to NullReferenceException in NSUrlSessionHandlerDelegate.SetResponse #9132 somehow.
  2. The fact that the exception crashes the process. This shouldn't happen.

I'll look into this to see what I can do to at the very least not make the process crash.

@rolfbjarne rolfbjarne added the bug If an issue is a bug or a pull request a bug fix label Feb 16, 2024
@rolfbjarne rolfbjarne added this to the Future milestone Feb 16, 2024
@rolfbjarne rolfbjarne self-assigned this Feb 16, 2024
rolfbjarne added a commit to rolfbjarne/xamarin-macios that referenced this issue Feb 16, 2024
We don't want to leak exceptions back to the calling native code in WrappedNSInputStream.Read, because that will likely crash the process.

Example stack trace:

    ObjectDisposed_StreamClosed (System.ObjectDisposedException)
       at System.ThrowHelper.ThrowObjectDisposedException_StreamClosed(String) + 0x3c
       at System.IO.MemoryStream.Read(Byte[], Int32, Int32) + 0x124
       at System.Net.Http.MultipartContent.ContentReadStream.Read(Byte[], Int32, Int32) + 0x78
       at System.Net.Http.NSUrlSessionHandler.WrappedNSInputStream.Read(IntPtr buffer, UIntPtr len) + 0x58
       at MyApp!<BaseAddress>+0x7082f8

Instead return -1 from the Read method, which is documented as an error
condition, and then also return a custom NSError from the Error property -
which is also documented to be where the error is supposed to be surfaced.

Ref: https://developer.apple.com/documentation/foundation/nsinputstream/1411544-read

Ref: xamarin#20123.
rolfbjarne added a commit that referenced this issue Feb 21, 2024
)

We don't want to leak exceptions back to the calling native code in WrappedNSInputStream.Read, because that will likely crash the process.

Example stack trace:

    ObjectDisposed_StreamClosed (System.ObjectDisposedException)
       at System.ThrowHelper.ThrowObjectDisposedException_StreamClosed(String) + 0x3c
       at System.IO.MemoryStream.Read(Byte[], Int32, Int32) + 0x124
       at System.Net.Http.MultipartContent.ContentReadStream.Read(Byte[], Int32, Int32) + 0x78
       at System.Net.Http.NSUrlSessionHandler.WrappedNSInputStream.Read(IntPtr buffer, UIntPtr len) + 0x58
       at MyApp!<BaseAddress>+0x7082f8

Instead return -1 from the Read method, which is documented as an error
condition, and then also return a custom NSError from the Error property -
which is also documented to be where the error is supposed to be surfaced.

Ref: https://developer.apple.com/documentation/foundation/nsinputstream/1411544-read

Ref: #20123.
@rolfbjarne
Copy link
Member

rolfbjarne commented Feb 21, 2024

I've merged an attempt at fixing the crash itself (we'll now return -1 from the Read method and set the Error property on the NSInputStream when Read throws an exception). I can't reproduce the problem, so I can't say what will happen if this scenario occurs, but it will probably not be worse than crashing the process. Hopefully the error will bubble up and show up in a place where it can be handled/reported properly.

The fix will be released with our release matching Xcode 15.3.

@rolfbjarne rolfbjarne removed their assignment Feb 21, 2024
@tipa
Copy link
Author

tipa commented Feb 21, 2024

Thank you!
Looking again at the stack trace, I might have an idea what was going wrong.
In my app, I am sending a MultipartContent in an Http request. If that request fails (e.g. because there's no internet connection), I am retrying the request. When doing that, I did not create a new MultipartContent, but instead used the previous one - which had already been disposed (normal HttpClient behavior). In the second request, the NSUrlSession then attempts to read from the disposed content stream and throws an ObjectDisoposedException.

However, I just tried to reproduced that crash in a Debug build and it produces a catchable exception (without any mention of WrappedNSInputStream) with the following stack trace:

System.ObjectDisposedException: Cannot access a disposed object.
System.Net.Http.MultipartContent'.
   at System.Net.Http.HttpContent.CheckDisposed()
   at System.Net.Http.HttpContent.ReadAsStreamAsync(CancellationToken cancellationToken)
   at System.Net.Http.HttpContent.ReadAsStreamAsync()
   at System.Net.Http.NSUrlSessionHandler.CreateRequest(HttpRequestMessage request) in /Users/builder/azdo/_work/1/s/xamarin-macios/src/Foundation/NSUrlSessionHandler.cs:line 481
   at System.Net.Http.NSUrlSessionHandler.SendAsync(HttpRequestMessage request, CancellationToken cancellationToken) in /Users/builder/azdo/_work/1/s/xamarin-macios/src/Foundation/NSUrlSessionHandler.cs:line 511
   at System.Net.Http.HttpClient.<SendAsync>g__Core|83_0(HttpRequestMessage request, HttpCompletionOption completionOption, CancellationTokenSource cts, Boolean disposeCts, CancellationTokenSource pendingRequestsCts, CancellationToken originalCancellationToken)

Maybe the http requests & threading behave differently in Debug and Release (with NativeAot)?
Or some timing difference does not have it crash here, but instead returns the MemoryStream that is handed over to WrappedNSInputStream here but then disposed shortly after. This might happen when sending two http requests with the same HttpContent concurrently

@rolfbjarne
Copy link
Member

When doing that, I did not create a new MultipartContent, but instead used the previous one - which had already been disposed (normal HttpClient behavior).

Looks like that changed in HttpClient:

dotnet/corefx#19082

https://github.com/dotnet/runtime/blob/8fb9f4b9fadb8060a7cf5cb991d526a669a2f03c/src/libraries/System.Net.Http/src/System/Net/Http/HttpClient.cs#L665-L680

So I'm not sure what's exactly disposing the MultipartContent.

It's still good that you were able to reproduce something similar: would you be able to provide a test project I can use to replicate this?

Maybe the http requests & threading behave differently in Debug and Release (with NativeAor)?

I think it's more likely just a timing issue, depending on exactly when the network goes down, and potentially also when the GC runs.

@tipa
Copy link
Author

tipa commented Feb 21, 2024

In this project you see how I retry my http requests and how to reproduce the exception: http_test.zip

The crash happens in this case because I manually dispose the HttpRequestMessage using the using keyword.
I edited my comment above after you've quoted it with a possible explanation of how the crash could happen in WrappedNSInputStream.Read instead of in HttpContent.ReadAsStreamAsync

Edit: I tried refactoring the logic that it uses a while-loop to retry requests using the same HttpRequestMessage without disposing it instead of calling SendRequest recusrively, but that doesn't work as that will throw:

System.InvalidOperationException: 'The request message was already sent. Cannot send the same request message multiple times.'

   at System.Net.Http.HttpClient.CheckRequestMessage(HttpRequestMessage request)
   at System.Net.Http.HttpClient.SendAsync(HttpRequestMessage request, HttpCompletionOption completionOption, CancellationToken cancellationToken)
   at System.Net.Http.HttpClient.SendAsync(HttpRequestMessage request, CancellationToken cancellationToken)

@rolfbjarne
Copy link
Member

In this project you see how I retry my http requests and how to reproduce the exception: http_test.zip

The crash happens in this case because I manually dispose the HttpRequestMessage using the using keyword. I edited my comment above after you've quoted it with a possible explanation of how the crash could happen in WrappedNSInputStream.Read instead of in HttpContent.ReadAsStreamAsync

Edit: I tried refactoring the logic that it uses a while-loop to retry requests using the same HttpRequestMessage without disposing it instead of calling SendRequest recusrively, but that doesn't work as that will throw:

System.InvalidOperationException: 'The request message was already sent. Cannot send the same request message multiple times.'

   at System.Net.Http.HttpClient.CheckRequestMessage(HttpRequestMessage request)
   at System.Net.Http.HttpClient.SendAsync(HttpRequestMessage request, HttpCompletionOption completionOption, CancellationToken cancellationToken)
   at System.Net.Http.HttpClient.SendAsync(HttpRequestMessage request, CancellationToken cancellationToken)

I tried to stress test this by:

  • Using the httpbin.org/delay end point to delay response for 10 seconds
  • Post a lot of data (100MB) to this end point
  • Cancel at a random moment during those 10 seconds (using a cancellation token)
  • Do this 100 times in parallel.

Source code: https://gist.github.com/rolfbjarne/e415f1bc4ca9b98964041c28a37ad311

And there were no exceptions on a background thread, only the expected TaskCanceledException.

So I'm not sure what to do here without a test project I can use to reproduce the ObjectDisposedException.

@rolfbjarne rolfbjarne added the need-repro Waiting for a test case before the bug can be investigated label Mar 7, 2024
@tipa
Copy link
Author

tipa commented Mar 7, 2024

Since there have been two fixes to the NSUrlSessionHandler in the meantime (#20131 & #16341), it's possible that the crash I observed would no longer occur. Once those PRs have been released (perhaps together with Xcode 15.3 support) as well as this one on the Android side, I will refactor my HTTP retry-logic to use the same HttpContent for multiple HttpRequestMessages - and if I continue to see some HTTP-related crash, I can open a new issue

@microsoft-github-policy-service microsoft-github-policy-service bot added need-attention An issue requires our attention/response and removed need-repro Waiting for a test case before the bug can be investigated labels Mar 7, 2024
@rolfbjarne rolfbjarne added need-info Waiting for more information before the bug can be investigated no-auto-reply For internal use and removed need-attention An issue requires our attention/response labels Mar 8, 2024
@microsoft-github-policy-service microsoft-github-policy-service bot removed the no-auto-reply For internal use label Mar 8, 2024
@tipa
Copy link
Author

tipa commented Mar 11, 2024

@rolfbjarne I was able to create a test project with which I was able to reproduce this possibly related crash 4 times within just a couple of minutes: #11799 (comment)
It's a long standing bug that was reported by dozens of users and it would be amazing if it could also be fixed

@microsoft-github-policy-service microsoft-github-policy-service bot added need-attention An issue requires our attention/response and removed need-info Waiting for more information before the bug can be investigated labels Mar 11, 2024
@rolfbjarne
Copy link
Member

I can reproduce a crash with your sample; I'll have a look.

@rolfbjarne rolfbjarne removed the need-attention An issue requires our attention/response label Mar 12, 2024
@rolfbjarne rolfbjarne added the networking If an issue or pull request is related to networking label Mar 19, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug If an issue is a bug or a pull request a bug fix networking If an issue or pull request is related to networking
Projects
None yet
Development

No branches or pull requests

2 participants