[Mono.Android] Data sharing and Close() overrides

[jonpryor]

Context: #9039
Context: #9039 (comment)
Context: 0315e89

In #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 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:

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().

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!

Notes to Inheritors

In derived classes, do not override the Close() method,
instead put all of the Stream cleanup logic in the
Dispose(Boolean) method. For more information, see
Implementing a Dispose Method.

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 #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().)