Commit
This commit does not belong to any branch on this repository, and may belong to a fork outside of the repository.
[Mono.Android] Data sharing and Close() overrides (dotnet#9103)
Context: dotnet#9039 Context: dotnet#9039 (comment) Context: 0315e89 In dotnet#9039, a customer reports that starting with the .NET for Android workload 34.0.113, their app would start crashing with an `ObjectDisposedException`: ObjectDisposed_ObjectName_Name, Java.IO.InputStreamInvoker at Java.Interop.JniPeerMembers.AssertSelf(IJavaPeerable ) at Java.Interop.JniPeerMembers.JniInstanceMethods.InvokeVirtualVoidMethod(String , IJavaPeerable , JniArgumentValue* ) at Java.IO.InputStream.Close() at Android.Runtime.InputStreamInvoker.Close() at System.IO.Stream.Dispose() at System.IO.BufferedStream.Dispose(Boolean ) at System.IO.Stream.Close() at System.IO.Stream.Dispose() at System.Net.Http.StreamContent.Dispose(Boolean ) at System.Net.Http.HttpContent.Dispose() at System.Net.Http.HttpResponseMessage.Dispose(Boolean ) at Xamarin.Android.Net.AndroidHttpResponseMessage.Dispose(Boolean ) at System.Net.Http.HttpResponseMessage.Dispose() at System.Net.Http.HttpClient.HandleFailure(Exception , Boolean , HttpResponseMessage , CancellationTokenSource , CancellationToken , CancellationTokenSource ) at System.Net.Http.HttpClient.<SendAsync>g__Core|83_0(HttpRequestMessage , HttpCompletionOption , CancellationTokenSource , Boolean , CancellationTokenSource , CancellationToken ) This was rather confusing, as between [34.0.95 and 34.0.113][0] the only change to `Mono.Android.dll` was 0315e89, which has nothing to do with networking. Additional consideration presented a hypothetical: [`IDisposable.Dispose()` should be idempotent][1]: > To help ensure that resources are always cleaned up appropriately, > a Dispose method should be idempotent, such that it's callable > multiple times without throwing an exception. *Is* `InputStreamInvoker.Dispose()` idempotent? An additional conundrum is that `InputStreamInvoker.Dispose()` doesn't exist; it's `Stream.Dispose()` that exists, and [`Stream.Dispose()` invokes `Stream.Close()`][2]. This in turn means that `Stream.Close()` must be idempotent, which in turn means that `InputStreamInvoker.Close()` must be idempotent. Additionally, [`Stream.Close()` docs say you shouldn't override it][3]! > ## Notes to Inheritors > In derived classes, do not override the [`Close()`][4] method, > instead put all of the Stream cleanup logic in the > [`Dispose(Boolean)`][5] method. For more information, see > [Implementing a Dispose Method][1]. So we have a theoretical concern that `InputStreamInvoker.Close()` might not be idempotent, and that *might* be responsible for an `ObjectDisposedException`. Maybe. (At least it's a start?) Create the obvious idempotent unit test, let's see if it fails: var javaInputStream = new Java.IO.ByteArrayInputStream (new byte[]{0x1, 0x2, 0x3, 0x4}); var invoker = new InputStreamInvoker (javaInputStream); invoker.Dispose (); invoker.Dispose (); Calling `invoker.Dispose()` twice does not fail. It *is* idempotent, at least for this test data. However, with a slight change to that logic, we're not only able to make things break, but the breakage looks rather similar to the original `ObjectDisposedException`! var javaInputStream = new Java.IO.ByteArrayInputStream (new byte[]{0x1, 0x2, 0x3, 0x4}); var invoker = new InputStreamInvoker (javaInputStream); javaInputStream.Dispose (); invoker.Dispose (); fails with: at Java.Interop.JniPeerMembers.AssertSelf(IJavaPeerable self) at Java.Interop.JniPeerMembers.JniInstanceMethods.InvokeVirtualVoidMethod(String encodedMember, IJavaPeerable self, JniArgumentValue* parameters) at Java.IO.InputStream.Close() at Android.Runtime.InputStreamInvoker.Close() at System.IO.Stream.Dispose() Thus, a new hypothesis for dotnet#9039: we *somehow* have a `Java.IO.InputStream` instance being shared, and that shared instance is being `.Dispose()`d of from multiple places. Neither `InputStreamInvoker.Close()` nor `InputStreamInvoker.Dispose()` anticipated this, both invoke `BaseInputStream.Close()` on a now disposed instance, and the `ObjectDisposedException` is the result. TODO: try to validate this hypothesis. Perhaps it's related to the use of `BufferedStream` in `AndroidMessageHandler.GetContent()`? For now, follow the advice of the `Stream.Close()` docs, and "remove" the `InputStreamInvoker.Close()` and `OutputStreamInvoker.Close()` method overrides. Furthermore, update their `Dispose(bool)` methods to verify that `BaseInputStream` and/or `BaseOutputStream` are not disposed before invoking `.Close()`. (Note: the `Close()` methods aren't actually removed, because doing so makes the public API analyzers complain. Instead, `Close()` is retained, but it just calls `base.Close()`.) [0]: dotnet/android@34.0.95...34.0.113 [1]: https://learn.microsoft.com/dotnet/standard/garbage-collection/implementing-dispose [2]: https://github.com/dotnet/runtime/blob/2ea6ae57874c452923af059cbcb57d109564353c/src/libraries/System.Private.CoreLib/src/System/IO/Stream.cs#L156 [3]: https://learn.microsoft.com/dotnet/api/system.io.stream.close?view=net-8.0#notes-to-inheritors [4]: https://learn.microsoft.com/dotnet/api/system.io.stream.close?view=net-8.0#system-io-stream-close [5]: https://learn.microsoft.com/dotnet/api/system.io.stream.dispose?view=net-8.0#system-io-stream-dispose(system-boolean)
- Loading branch information
