From 83626c9cf8a8af3b3e2f365de147662cbb38a2d6 Mon Sep 17 00:00:00 2001 From: Jonathan Pryor Date: Fri, 12 Jul 2024 07:32:09 -0400 Subject: [PATCH] [Mono.Android] Data sharing and Close() overrides (#9103) Context: https://github.com/dotnet/android/issues/9039 Context: https://github.com/dotnet/android/issues/9039#issuecomment-2222830427 Context: 0315e8955cb93190832dc227aafaa90dda57a91a In dotnet/android#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.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 0315e895, 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/android#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]: https://github.com/dotnet/android/compare/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) --- src/Mono.Android/Android.Runtime/InputStreamInvoker.cs | 10 ++++------ .../Android.Runtime/OutputStreamInvoker.cs | 10 ++++------ .../Android.Runtime/InputStreamInvokerTest.cs | 9 +++++++++ 3 files changed, 17 insertions(+), 12 deletions(-) diff --git a/src/Mono.Android/Android.Runtime/InputStreamInvoker.cs b/src/Mono.Android/Android.Runtime/InputStreamInvoker.cs index 232109d4416..fedd5ce2b2d 100644 --- a/src/Mono.Android/Android.Runtime/InputStreamInvoker.cs +++ b/src/Mono.Android/Android.Runtime/InputStreamInvoker.cs @@ -37,7 +37,9 @@ protected override void Dispose (bool disposing) if (disposing && BaseInputStream != null) { try { BaseFileChannel = null; - BaseInputStream.Close (); + if (BaseInputStream.PeerReference.IsValid) { + BaseInputStream.Close (); + } BaseInputStream.Dispose (); } catch (Java.IO.IOException ex) when (JNIEnv.ShouldWrapJavaException (ex)) { throw new IOException (ex.Message, ex); @@ -58,11 +60,7 @@ protected override void Dispose (bool disposing) // public override void Close () { - try { - BaseInputStream.Close (); - } catch (Java.IO.IOException ex) when (JNIEnv.ShouldWrapJavaException (ex)) { - throw new IOException (ex.Message, ex); - } + base.Close (); } public override void Flush () diff --git a/src/Mono.Android/Android.Runtime/OutputStreamInvoker.cs b/src/Mono.Android/Android.Runtime/OutputStreamInvoker.cs index c9985ee084c..4438b25804f 100644 --- a/src/Mono.Android/Android.Runtime/OutputStreamInvoker.cs +++ b/src/Mono.Android/Android.Runtime/OutputStreamInvoker.cs @@ -28,11 +28,7 @@ public OutputStreamInvoker (Java.IO.OutputStream stream) // public override void Close () { - try { - BaseOutputStream.Close (); - } catch (Java.IO.IOException ex) when (JNIEnv.ShouldWrapJavaException (ex)) { - throw new IOException (ex.Message, ex); - } + base.Close (); } // @@ -50,7 +46,9 @@ protected override void Dispose (bool disposing) { if (disposing && BaseOutputStream != null) { try { - BaseOutputStream.Close (); + if (BaseOutputStream.PeerReference.IsValid) { + BaseOutputStream.Close (); + } BaseOutputStream.Dispose (); } catch (Java.IO.IOException ex) when (JNIEnv.ShouldWrapJavaException (ex)) { throw new IOException (ex.Message, ex); diff --git a/tests/Mono.Android-Tests/Android.Runtime/InputStreamInvokerTest.cs b/tests/Mono.Android-Tests/Android.Runtime/InputStreamInvokerTest.cs index 6252b61ae41..cb91c704f12 100644 --- a/tests/Mono.Android-Tests/Android.Runtime/InputStreamInvokerTest.cs +++ b/tests/Mono.Android-Tests/Android.Runtime/InputStreamInvokerTest.cs @@ -12,6 +12,15 @@ namespace Android.RuntimeTests [TestFixture] public class InputStreamInvokerTest { + [Test] + public void Disposing_Shared_Data_Does_Not_Throw_IOE () + { + var javaInputStream = new Java.IO.ByteArrayInputStream (new byte[]{0x1, 0x2, 0x3, 0x4}); + var invoker = new InputStreamInvoker (javaInputStream); + javaInputStream.Dispose (); + invoker.Dispose (); + } + [Test] public void InputStreamTest () {