Surface HttpRequestException from AndroidMessageHandler

[Copilot]

Xamarin.Android.Net.AndroidMessageHandler reported transport/protocol failures as the legacy System.Net.WebException, contradicting the documented HttpClient.SendAsync contract (which throws HttpRequestException).

Changes

• AndroidMessageHandler: all WebException throw sites in the request path (timeout, protocol, security, IO, connect, max‑redirections, invalid redirect URI, protocol‑method setup) now throw HttpRequestException.
• Back-compat: the original WebException — including its WebExceptionStatus — is kept as the InnerException, so code migrated from classic Xamarin.Android that inspects status (and existing test helpers that walk the inner chain) keeps working. Centralized in one helper:

static HttpRequestException CreateHttpRequestException (string message, Exception? innerException, WebExceptionStatus status)
    => new HttpRequestException (message, new WebException (message, innerException, status, null));

• Connection-failure path: ConnectAsync wraps every connection failure in HttpRequestException, so DoProcessRequest now catches that HttpRequestException when its InnerException is a Java.Net.ConnectException, unwraps it, and re-wraps it via CreateHttpRequestException with WebExceptionStatus.ConnectFailure. Previously the catch (Java.Net.ConnectException) block was unreachable, so connection failures did not get the intended inner WebException(ConnectFailure) back-compat wrapping.
• Tests: connecting to an unused port asserts HttpRequestException with a preserved inner WebException whose Status == ConnectFailure; exceeding MaxAutomaticRedirections asserts HttpRequestException with a preserved inner WebException whose Status == UnknownError.

Notes

• Public API surface is unchanged; only the thrown exception type differs.
• Callers catching WebException at the top level will no longer match — this is the intended behavioral change. The WebException remains reachable via ex.InnerException.

[Copilot]

Pull request overview

This PR updates Xamarin.Android.Net.AndroidMessageHandler to align with the HttpClient.SendAsync contract by surfacing request-path transport/protocol failures as HttpRequestException (while preserving the legacy WebException + WebExceptionStatus via InnerException for back-compat).

Changes:

• Replace WebException throw sites in the request path with HttpRequestException, preserving a WebException (with status) as the inner exception.
• Centralize exception creation in a CreateHttpRequestException(...) helper.
• Add tests asserting HttpRequestException is thrown for connection failures and exceeding MaxAutomaticRedirections.

Reviewed changes

Copilot reviewed 2 out of 2 changed files in this pull request and generated 3 comments.

File	Description
src/Mono.Android/Xamarin.Android.Net/AndroidMessageHandler.cs	Switches request-path failures to HttpRequestException and introduces a helper to preserve legacy WebException details.
tests/Mono.Android-Tests/Mono.Android-Tests/Xamarin.Android.Net/AndroidMessageHandlerTests.cs	Adds regression tests to validate the new exception surface and inner-exception preservation expectations.

[jonathanpeppers]

@copilot address the code review comments left above

[Copilot]

@copilot address the code review comments left above

Addressed all three review comments in 28f733d:

• DoProcessRequest now catches the HttpRequestException that ConnectAsync wraps around Java.Net.ConnectException, unwraps it, and re-wraps via CreateHttpRequestException so the inner WebException(ConnectFailure) back-compat path is actually exercised.
• ConnectionFailureThrowsHttpRequestException now asserts the inner WebException.Status == ConnectFailure.
• ExceedingMaxAutomaticRedirectionsThrowsHttpRequestException now asserts the inner WebException.Status == UnknownError.

[simonrozsival]

/review

[github-actions]

✅ Android PR Reviewer completed successfully!

[github-actions]

✅ LGTM

A focused, well-reasoned change. AndroidMessageHandler now surfaces transport/protocol failures as HttpRequestException (per the HttpClient.SendAsync contract) while preserving the legacy WebException — including its WebExceptionStatus — as the InnerException for code migrated from classic Xamarin.Android. I formed an independent assessment of the diff and then reconciled it with the PR description; the claims hold up.

Verified:

• All WebException throw sites converted. Every throw site in the request path now routes through the CreateHttpRequestException helper; no bare WebException throws remain. ✓
• The previously-dead catch (Java.Net.ConnectException) is now reachable. ConnectAsync wraps every connection failure in HttpRequestException, so the old catch could never match. The new catch (HttpRequestException ex) when (ex.InnerException is Java.Net.ConnectException) correctly unwraps and re-wraps it so the WebException(ConnectFailure) back-compat path actually runs. ✓
• Status preservation is correct. WebExceptionStatus.UnknownError for the max-redirections / invalid-redirect-URI paths matches what the old WebException(string) constructor produced, so status-inspecting code is unaffected. ✓
• No collateral breakage. No other product code or test asserts a bare WebException from this handler; AssertRejectsRemoteCertificate already catches both types and keeps passing. ✓
• Regression tests added for the connect-failure and max-redirections paths, asserting the inner WebException.Status (traced MaxAutomaticRedirections = 1 → throws on the first redirect, so the single-BeginGetContext listener is sufficient). ✓
• CI is green — 40/40 checks, including the macOS emulator device-test legs that exercise Mono.Android-Tests. ✓

Severity counts: ❌ 0 · ⚠️ 0 · 💡 2 (both optional/non-blocking, posted inline)

• Populate HttpRequestException.HttpRequestError for fuller modern-contract conformance.
• The inner-WebException shim only covers Java.Net.ConnectException; DNS/timeout connect failures aren't classified (pre-existing gap, not a regression).

Minor follow-up (pre-existing, not inlineable): the comment at AndroidMessageHandlerTests.cs:308-310 ("...we throw both of these...") and the now-unreachable catch (System.Net.WebException) {} on line 311 are stale after this change — the handler no longer throws a bare WebException. Worth tidying in a follow-up (stale-comment / dead-code cleanup).

Nice work — no blocking issues.

Generated by Android PR Reviewer for issue #11682 · 793.2 AIC · ⌖ 47.2 AIC · ⊞ 37.8K
_{Comment /review to run again}

[github-actions]

🤖 💡 API design — Optional/non-blocking: since Mono.Android targets modern .NET, consider also populating HttpRequestException.HttpRequestError (added in .NET 8) here, in addition to the inner WebException. The point of this change is to honor the HttpClient.SendAsync contract, and SocketsHttpHandler sets values like HttpRequestError.ConnectionError / SecureConnectionError so callers can branch on ex.HttpRequestError rather than unwrapping the legacy WebException. e.g. for the connect-failure path:

new HttpRequestException (HttpRequestError.ConnectionError, message, new WebException (message, innerException, status, null))

Mapping each WebExceptionStatus to an HttpRequestError is a bit more than a one-liner, so feel free to defer — just flagging it for fuller modern-contract conformance.

Rule: Conform to the modern HttpClient contract

[jonathanpeppers]

Deferring to a follow-up. Mapping each WebExceptionStatus to an HttpRequestError (e.g. ConnectFailure → ConnectionError, SecureChannelFailure → SecureConnectionError, Timeout → ?, ProtocolError → ?, UnknownError → ?) needs deliberation and its own test coverage, and would expand the scope of this otherwise-focused back-compat PR. Filing a follow-up issue.

[github-actions]

🤖 💡 Patterns — Optional/non-blocking: this back-compat shim restores an inner WebException only for Java.Net.ConnectException. Other failures that ConnectAsync wraps the same way — DNS (Java.Net.UnknownHostException → NameResolutionFailure) and connect timeouts (Java.Net.SocketTimeoutException → Timeout) — still surface as HttpRequestException whose InnerException is the raw Java exception, so WebException-walking code (including the IsConnectionFailure test helper) won't classify them. This is a pre-existing gap, not a regression — before this change the old catch (Java.Net.ConnectException) was dead, so no connect failure got the WebException wrapper; this PR strictly improves the most common case. A follow-up could map the DNS/timeout cases too for completeness.

Rule: Preserve back-compat consistently

[jonathanpeppers]

Agreed — this is a pre-existing gap (the old catch (Java.Net.ConnectException) was dead, so DNS/timeout never got the WebException wrapper either). Out of scope for this PR; tracking in a follow-up issue alongside the HttpRequestError work.

[jonathanpeppers]

Thanks for the review! Addressed the stale-comment / dead-code follow-up in 56138fd: removed the now-unreachable catch (System.Net.WebException) {} and the misleading "we throw both" comment in AssertRejectsRemoteCertificate. Confirmed by grepping throw new WebException in src/Mono.Android/Xamarin.Android.Net/ — zero matches; all 8 throw sites now route through CreateHttpRequestException.

Deferring the two inline 💡 suggestions (HttpRequestError population and DNS/timeout WebException shim) to a follow-up — replied inline.