Repository navigation
Report Android and Apple response-body failures as transport errors - #38
Open
bkaradzic-microsoft wants to merge 2 commits into
Open
bkaradzic-microsoft wants to merge 2 commits into
bkaradzic-microsoft wants to merge 2 commits into
Conversation
Check pending JNI exceptions immediately after reading or copying a response body, clear partial responses on failure, and publish status only after a complete read. Preserve completed HTTP error responses via getErrorStream. Document the Android transport-error diagnostics. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
bkaradzic-microsoft
pushed a commit
to bkaradzic-microsoft/JsRuntimeHost
that referenced
this pull request
Oct 5, 2026
Prepare replacement requests before invalidating active sends and preserve responseType. Detach canceled transport state before synchronous callbacks and guard response access while the transport is active. Keep callbacks in a JavaScript WeakMap keyed by their XHR, with stable native listener records and short-lived persistent invocation references. Retain non-callable handler objects by identity without invoking them. Pin the Android response-read fix from BabylonJS/UrlLib#38. Add reuse, failed-open, object-identity, self-capturing GC, streaming-abort, and truncated HTTP response regressions, and update the polyfill documentation. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
There was a problem hiding this comment.
Copilot review overview
🟢 Approval recommended
The implementation matches the stated behavior and safely clears partial response state on failures.
Review effort: Balanced
Findings: None
What changed in this PR
Defers Android HTTP success reporting until response bodies are fully read and surfaces read failures as transport diagnostics.
Changes:
- Handles JNI exceptions and truncated known-length responses.
- Uses error streams for completed HTTP error responses.
- Documents Android transport-error behavior.
| File | Description |
|---|---|
Source/UrlRequest_Android.cpp |
Adds robust response reading and error reporting. |
README.md |
Documents Android response and diagnostic semantics. |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
NSURLSession can complete a truncated 404 response without an NSError. Validate unencoded fixed-length bodies before publishing status, headers, or data and report ResponseReadFailed on premature EOF. Keep compressed, chunked, and bodyless responses out of that comparison. Add loopback regressions covering string/buffer 200 and 404 responses, gzip, chunked transfer, empty bodies, and 204/304 status semantics. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: 2b2272ba-79f8-41d0-aae1-f514a0f712ad
bkaradzic-microsoft
changed the title
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Problem
Companion to BabylonJS/JsRuntimeHost#221, addressing the Android response-read review.
Android published the HTTP status before reading the body. A failed read could therefore retain status 200 and make XHR dispatch
load. The pinnedInputStream::Readwrapper also leaves Java exceptions pending: the nextByteArrayOutputStream::WriteJNI call aborts under CheckJNI when a response is truncated.Changes
JavaException/ResponseReadFailedtransport diagnostics.getErrorStream(), so a completed 404 remains a successful transfer with HTTP status 404.Validation
Exercised through the native XHR regression tests added in BabylonJS/JsRuntimeHost#221 on an Android API 36.1 x86_64 emulator, QuickJS, NDK 29:
CompletedAndTruncatedHttpBodies: complete and truncated 200/404 responses from a loopback server.AbortResponseReadsWhileBodyIsStreaming: text and arraybuffer getters during synchronous abort.The truncated-response test crashes with the original pinned UrlLib under CheckJNI and passes with this change. UrlLib's existing Windows suite also passes (9 passed, 6 platform-specific skips).
Apple follow-up
PR #221's macOS jobs exposed a second backend issue:
NSURLSessioncompletes a truncated 404 body without anNSError. Commit 85e8f02 validates fixed-length, identity-encoded HTTP bodies before publishing status or data, returningurllib:ResponseReadFailed(0)on premature EOF.The check deliberately excludes gzip/other content encodings, chunked transfer encoding, and bodyless 204/304 responses: decoded
NSDatalength is not generally the wireContent-Length.Native UrlLib loopback regressions now cover complete/truncated 200 and 404 bodies in both response modes, plus Apple gzip, chunked, empty, and bodyless response cases. The existing XHR regression remains unchanged. Linux transport regressions and the downstream XHR ThreadSanitizer run pass locally; Companion CI is green on this commit: 18 macOS tests passed, including both new regressions; Linux and Windows jobs also passed.