[Mono.Android] Prevent ObjectDisposedException while reading HTTP response from InputStreamInvoker

[simonrozsival]

Fixes #9039

TODO:

• Add tests
• Find the root cause of the issue - why has this issue surfaced recently?
• Find out what else could be affected by the same problem?

The issue manifests in the following code snippet, where we're downloading a large file from the server:

var request = new HttpRequestMessage(HttpMethod.Get, $"/images/{imageId}/download");
var response = await _client.SendAsync(request, HttpCompletionOption.ResponseHeadersRead, cancellationToken);

response.EnsureSuccessStatusCode();

await using var stream = await response.Content.ReadAsStreamAsync(cancellationToken);
await using var fileStream = File.Create(path);

int bytesRead = -1;
var buffer = new byte[512].AsMemory(); // Small buffer on purpose.
while (bytesRead != 0)
{
    bytesRead = await stream.ReadAsync(buffer, cancellationToken);
    await fileStream.WriteAsync(buffer[..bytesRead], cancellationToken);

    Console.WriteLine($"Downloaded {(int)(100 * fileStream.Length / size)}%");
}

My best attempt at explaining the bug is the following:

1. the AndroidMessageHandler returns an instance of AndroidHttpResponseMessage which keeps a reference to the HttpURLConnection MCW which internally keeps a reference to the InputStream (wrapped by Android.Runtime.InputStreamInvoker)

2. after line stream = await response.Content.ReadAsStreamAsync(cancellationToken)...

   1. stream is referencing the response's InputStreamInvoker which keeps a reference to the InputStream
   2. response can be collected by GC

3. when the download is long enough for several .NET and Java GCs to occur, this happens:

   1. response is collected on the .NET side

   2. HttpURLConnection is collected on the Java side

   3. when the next .NET GC runs, our Java GC bridge will change the strong gref Handle in InputStream to a weak ref and initiate Java GC

   4. Java GC will collect the InputStream since there are no more references to it from the Java side

   5. the GC bridge will test if the weak ref is still valid after the Java GC and it notices it isn't, so it replaces the handle with IntPtr.Zero

   6. the .NET InputStream object won't be collected though, because InputStreamInvoker is still holding a reference to it

   7. the next time we try to read from the InputStreamInvoker, it's internal InputStream is disposed and reading from it will throw the ObjectDisposedException we are observing

The problem can be prevented by keeping a gref to the underlying InputStream in InputStreamInvoker. This way, the gc bridge cannot dispose the InputStream while the only reference to it is on the .NET side in an object which isn't a Java class wrapper. Without this patch, the app will fail every single time when using a local debug build. With the patch applied, the exception is not thrown anymore.

/cc @grendello @jonathanpeppers @jonpryor @AaronRobinsonMSFT

[jonathanpeppers]

@simonrozsival FYI CI is complaining about branch name being too long:

error NU5123: Warning As Error: The file 'package/services/metadata/core-properties/37a2a0f2bbc1473e941d1c1f18bc947a.psmdcp' path, name, or both are too long. Your package might not work without long file path support. Please shorten the file path or file name.

We have seen it on other PRs and just shortened the name...

[AaronRobinsonMSFT]

i. response is collected on the .NET side

@simonrozsival Can you try placing a GC.KeepAlive(response); on the other side of the loop and see if that also addressed the issue? I'm trying to see if we can confirm where the missing reference in the graph occurs.

[simonrozsival]

@AaronRobinsonMSFT adding GC.KeepAlive(response); doesn't make any difference. I can see the handle of the InputStream being collected between two Read(byte[], int, int) calls:

• when I added logging to Android.Runtime.InputStreamInvoker.Read to print the value of the base input stream Handle, I get a valid value in one Read (0x6092), then the following monodroid-gref from the gc bridge print those two logs mentioning that handle, and in the next call to Read the Handle is 0:

02-13 12:58:24.268 16901 16901 I monodroid-gref: +w+ grefc 312 gwrefc 12 obj-handle 0x6092/G -> new-handle 0x817/W from thread 'finalizer'(16901)
02-13 12:58:24.268 16901 16901 I monodroid-gref: -g- grefc 311 gwrefc 12 handle 0x6092/G from thread 'finalizer'(16901)

• the Android.Runtime.InputStreamInvoker object isn't collected on the .NET side - there is a call to Read after the Java object is collected
• the Android.Runtime.InputStreamInvoker has a direct reference to the itnernal Java.IO.InputStreamInvoker object which has been passed to the GC bridge anyway and it converted the gref to a weak ref as seen in the logs so that the Java side could collect it and the GC bridge sees that and replaces the Handle with 0
• I added logging to the Dispose and ~InputStream methods on the base class wrapper Java.IO.InputStream just to make sure we're not disposing the stream from the .NET side somewhere and I confirmed we aren't doing that. The logs from the finalizer appear much later, after the Read throws the ObjectDisposedException and the HTTP response is being disposed