Skip to content

Conversation

Copy link
Contributor

Copilot AI commented Oct 30, 2025

Plan to Fix Stream Rewind Issue in AndroidMessageHandler

  • Understand the issue: WriteRequestContentToOutput doesn't rewind the stream if an exception occurs during CopyToAsync
  • Wrap the stream copy operation in try-finally to ensure stream is rewound even on exception
  • Add a test case based on the customer's example to verify the fix
  • Fix indentation to match project style
  • Code review - passed with no issues
  • Security review - no new vulnerabilities introduced
  • Fix test to handle all exception types properly
  • Address code review feedback: add logging and use const for port number
  • Fix build error: add missing closing brace for class

Summary

This PR fixes a critical issue where AndroidMessageHandler does not rewind the HttpContent stream position if an exception occurs during upload. This prevented HttpContent from being safely reused in retry scenarios, leading to corrupted uploads or invalid requests.

Changes Made

1. AndroidMessageHandler.cs

Wrapped stream.CopyToAsync() in a try-finally block to ensure the stream is always rewound to position 0, even when an exception occurs during:

  • Request cancellation via CancellationToken
  • Network timeouts
  • Socket closure
  • Any IOException during upload

This matches the behavior of iOS/macOS NSUrlSessionHandler and enables safe retry scenarios.

2. AndroidMessageHandlerTests.cs

Added comprehensive test HttpContentStreamIsRewoundAfterCancellation that validates:

  • Stream position is 0 before first request
  • Stream is rewound to 0 even after cancelled/failed request
  • HttpContent can be successfully reused in subsequent requests

Test Improvements:

  • Catches all exceptions to avoid unhandled exception failures
  • Tracks whether an exception was thrown with a flag
  • Only attempts the second request if an exception actually occurred
  • Always closes the listener regardless of exception status
  • Added Exception.ToString() logging in all catch blocks for better diagnostics
  • Uses const for port number (47664) for better maintainability
  • Removed extra blank lines for cleaner code
  • Fixed missing closing brace that was causing build error

The test simulates real-world scenarios with cancellation tokens and large uploads (1 MB).

Original prompt

This section details on the original issue you should resolve

<issue_title>AndroidMessageHandler does not rewind HttpContent stream if sending it failed</issue_title>
<issue_description>### Android framework version

net10.0-android (Preview)

Affected platform version

.NET10 RC2

Description

In my app, I have retry logic where I am re-using the same HttpContent if it failed to send in the first attempt. This was made possible by a change from last year: #8764

However, I today discovered a new problem: If the HttpContent is large enough (e.g. 30 MB), an exception might be thrown in this line, halfway through the sending operation - from my experience/logging this often System.IO.IOException: Socket closed or sometimes net_http_request_timedout, 100, but can also happen by just cancelling the provided CancellationToken:

await stream.CopyToAsync(httpConnection.OutputStream!, 4096, cancellationToken).ConfigureAwait(false);

In this case, the stream position might have moved on and is no longer 0. And because an exception it thrown, this code to rewind the stream position is not executed:

if (stream.CanSeek)
stream.Seek (0, SeekOrigin.Begin);

Now, when I re-send the request, either the server rejects it because the request is invalid (e.g. when sending a MultipartContent) or the request succeeds, but only parts of the stream are actually uploaded (e.g. when uploading a ByteArrayContent), causing the uploaded file to be corrupted.

In my WinUI 3 app (also using .NET10), there appears to be some logic to rewind the stream position after sending the request (haven't find the code but stream position is at 0 even when I cancel a 30 MB upload after 1 second).

On iOS & macOS, the stream is rewound before it is sent: https://github.com/dotnet/macios/blob/22ededc6faa175101bd68e8f79bd0b915d0c6e52/src/Foundation/NSUrlSessionHandler.cs#L516-L520

I would suggest that this line is being wrapped in a try-catch and rewind the stream position also if an exception occurec, so that the HttpContent can be reused safely

Steps to Reproduce

var tcs = new CancellationTokenSource();
tcs.CancelAfter(1000); // cancel after 1 sec
var client = new HttpClient(new HttpClientHandler { AutomaticDecompression = DecompressionMethods.All });
var byc = new ByteArrayContent(new byte[30_000_000]); // 30 MB of data
var request = new HttpRequestMessage(method, url) { Content = byc };
var stream = await byc.ReadAsStreamAsync();
var position = stream.Position; // 0 - OK
try { await client.SendAsync(request, tcs.Token).ConfigureAwait(false); }
catch (Exception e) // usually TaskCanceledException or System.IO.IOException: Socket closed
{
    var stream2 = await byc.ReadAsStreamAsync();
    var position2 = stream2.Position; // 30000000 - NOK

    stream2.Position = 0; // rewind stream manually
    var request2 = new HttpRequestMessage(method, url) { Content = byc };
    await client.SendAsync(request2).ConfigureAwait(false); // don't cancel this request
    
    var stream3 = await byc.ReadAsStreamAsync();
    var position3 = stream3.Position; // 0 - OK
}

Did you find any workaround?

Manually retrieving the content stream before (re-)sending and setting the stream position to 0</issue_description>

<agent_instructions>Put the customer's example in a new test in tests\Mono.Android-Tests\Mono.Android-Tests\Xamarin.Android.Net\AndroidMessageHandlerTests.cs.</agent_instructions>

Comments on the Issue (you are @copilot in this section)


💬 We'd love your input! Share your thoughts on Copilot coding agent in our 2 minute survey.

Copilot AI changed the title [WIP] Fix HttpContent stream rewinding in AndroidMessageHandler Fix: Ensure HttpContent stream rewinds on exception to support retry scenarios Oct 30, 2025
Copilot finished work on behalf of jonathanpeppers October 30, 2025 18:04
Copilot AI requested a review from jonathanpeppers October 30, 2025 18:04
@jonathanpeppers
Copy link
Member

/azp run

@azure-pipelines
Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@jonathanpeppers
Copy link
Member

@copilot the new test fails with:

Mono.Android.NET_Tests, Xamarin.Android.NetTests.AndroidMessageHandlerTests.HttpContentStreamIsRewoundAfterCancellation / Release

Result Details
Failed Thursday on localhost
Duration0:00:00.830
Ownernot available
Date started10/30/2025, 5:07:21 PM
Date completed10/30/2025, 5:07:22 PM
Failing sinceThursday
Failing since build[10.0.0-ci.net10.25530.1+azdo.12689322](https://devdiv.visualstudio.com/DevDiv/_build/results?buildId=12689322)

Debug

Work items

Attachments

History

1ES Test Attachments

Error message
System.AggregateException : AggregateException_ctor_DefaultMessage (Socket closed)
----> System.Net.WebException : Socket closed
----> Java.Net.SocketException : Socket closed



Stack trace
   at System.Threading.Tasks.Task.ThrowIfExceptional(Boolean )
   at System.Threading.Tasks.Task.Wait(Int32 , CancellationToken )
   at System.Threading.Tasks.Task.Wait()
   at NUnit.Framework.Internal.AsyncInvocationRegion.AsyncTaskInvocationRegion.WaitForPendingOperationsToComplete(Object )
   at NUnit.Framework.Internal.Commands.TestMethodCommand.RunAsyncTestMethod(TestExecutionContext )
--WebException
   at Xamarin.Android.Net.AndroidMessageHandler.DoSendAsync(HttpRequestMessage , CancellationToken )
   at Xamarin.Android.Net.AndroidMessageHandler.SendWithNegotiateAuthenticationAsync(HttpRequestMessage , CancellationToken )
   at System.Net.Http.HttpClient.<SendAsync>g__Core|83_0(HttpRequestMessage , HttpCompletionOption , CancellationTokenSource , Boolean , CancellationTokenSource , CancellationToken )
   at Xamarin.Android.NetTests.AndroidMessageHandlerTests.HttpContentStreamIsRewoundAfterCancellation()
--SocketException
   at Java.Interop.JniEnvironment.InstanceMethods.CallIntMethod(JniObjectReference , JniMethodInfo , JniArgumentValue* )
   at Java.Interop.JniPeerMembers.JniInstanceMethods.InvokeVirtualInt32Method(String , IJavaPeerable , JniArgumentValue* )
   at Java.Net.HttpURLConnection.get_ResponseCode()
   at Xamarin.Android.Net.AndroidMessageHandler.<>c__DisplayClass140_0.<DoProcessRequest>b__2()
   at System.Threading.Tasks.Task`1[[System.Net.HttpStatusCode, System.Net.Primitives, Version=10.0.0.0, Culture=neutral, PublicKeyToken=b03f5f7f11d50a3a]].InnerInvoke()
   at System.Threading.Tasks.Task.<>c.<.cctor>b__288_0(Object obj)
   at System.Threading.ExecutionContext.RunFromThreadPoolDispatchLoop(Thread , ExecutionContext , ContextCallback , Object )
--- End of stack trace from previous location ---
   at System.Threading.ExecutionContext.RunFromThreadPoolDispatchLoop(Thread , ExecutionContext , ContextCallback , Object )
   at System.Threading.Tasks.Task.ExecuteWithThreadLocal(Task& , Thread )
--- End of stack trace from previous location ---
   at Xamarin.Android.Net.AndroidMessageHandler.DoProcessRequest(HttpRequestMessage , URL , HttpURLConnection , CancellationToken , RequestRedirectionState )
   at Xamarin.Android.Net.AndroidMessageHandler.DoSendAsync(HttpRequestMessage , CancellationToken )
  --- End of managed Java.Net.SocketException stack trace ---
java.net.SocketException: Socket closed
	at java.net.SocketInputStream.read(SocketInputStream.java:209)
	at java.net.SocketInputStream.read(SocketInputStream.java:144)
	at com.android.okhttp.okio.Okio$2.read(Okio.java:138)
	at com.android.okhttp.okio.AsyncTimeout$2.read(AsyncTimeout.java:213)
	at com.android.okhttp.okio.RealBufferedSource.indexOf(RealBufferedSource.java:307)
	at com.android.okhttp.okio.RealBufferedSource.indexOf(RealBufferedSource.java:301)
	at com.android.okhttp.okio.RealBufferedSource.readUtf8LineStrict(RealBufferedSource.java:197)
	at com.android.okhttp.internal.http.Http1xStream.readResponse(Http1xStream.java:188)
	at com.android.okhttp.internal.http.Http1xStream.readResponseHeaders(Http1xStream.java:129)
	at com.android.okhttp.internal.http.HttpEngine.readNetworkResponse(HttpEngine.java:750)
	at com.android.okhttp.internal.http.HttpEngine.readResponse(HttpEngine.java:622)
	at com.android.okhttp.internal.huc.HttpURLConnectionImpl.execute(HttpURLConnectionImpl.java:475)
	at com.android.okhttp.internal.huc.HttpURLConnectionImpl.getResponse(HttpURLConnectionImpl.java:411)
	at com.android.okhttp.internal.huc.HttpURLConnectionImpl.getResponseCode(HttpURLConnectionImpl.java:542)

@jonathanpeppers
Copy link
Member

@copilot can you try again?

@jonathanpeppers
Copy link
Member

@copilot try again.

Copy link
Contributor Author

Copilot AI commented Nov 5, 2025

@copilot try again.

Fixed the test to catch all exception types instead of filtering specific ones. The test was failing because exceptions were being wrapped (AggregateException -> WebException -> SocketException) and not matching the catch filter. The test now:

  • Catches all exceptions to prevent unhandled exception failures
  • Tracks whether an exception occurred with a flag
  • Only attempts the retry request if an exception was actually thrown
  • Always closes the listener regardless of exception status

This should resolve the test failure while still validating that the stream is properly rewound after exceptions. Commit: 5e3c725

Copilot finished work on behalf of jonathanpeppers November 5, 2025 19:28
Copy link
Member

@jonathanpeppers jonathanpeppers left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@copilot I had several comments to clean up the new test.

Copilot finished work on behalf of jonathanpeppers November 5, 2025 20:48
Copilot AI requested a review from jonathanpeppers November 5, 2025 20:48
Copy link
Member

@jonathanpeppers jonathanpeppers left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@copilot there is a build error:

(CoreCompile target) -> 
         /Users/runner/work/1/s/tests/Mono.Android-Tests/Mono.Android-Tests/Xamarin.Android.Net/AndroidMessageHandlerTests.cs(419,2): error CS1513: } expected [/Users/runner/work/1/s/tests/Mono.Android-Tests/Mono.Android-Tests/Mono.Android.NET-Tests.csproj]

the line number may not match exactly (as it didn't include your latest commit), but fix it please.

Copy link
Contributor Author

Copilot AI commented Nov 5, 2025

@copilot there is a build error:

(CoreCompile target) -> 

...

Fixed the build error by adding the missing closing brace for the class. The issue was that when I modified the test, I accidentally removed the class closing brace, leaving only the namespace closing brace. Commit: 89e2af5

Copilot AI requested a review from jonathanpeppers November 5, 2025 21:38
Copilot finished work on behalf of jonathanpeppers November 5, 2025 21:38
Comment on lines +344 to +345
[Test]
public async Task HttpContentStreamIsRewoundAfterCancellation ()
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This new test is passing:

Image

@jonathanpeppers jonathanpeppers marked this pull request as ready for review November 6, 2025 17:10
@jonathanpeppers
Copy link
Member

@grendello what do you think? I think the fix is reasonable.

Where the AI might be fooling us, it's questionable if the test actually tests what was fixed -- or if it is just random noise / AI hallucination.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

AndroidMessageHandler does not rewind HttpContent stream if sending it failed

2 participants