Repository navigation
perf(mobile): use HTTP/2 on Android through app-wide Cronet engines - #9435
Conversation
eb00637 to
d926e06
Compare
d926e06 to
dd58314
Compare
6e7070b to
36d8868
Compare
36d8868 to
490ca3f
Compare
Codex Review Summary: ✅ CompletedThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
🔐 Codex Security Review
Review SummaryOverall Risk: NONE
FindingsNo concrete security, correctness, or reliability findings were identified. Notes
Generated by Codex Security Review | |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 490ca3f4ca
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| debugPrint('Cronet is unavailable, using dart:io: ${error.message}'); | ||
| return null; | ||
| } | ||
| return CronetClient.fromCronetEngine(engine); |
There was a problem hiding this comment.
Preserve streaming uploads when selecting the Android transport
For Android HTTPS media uploads, this also replaces the streaming transport used by MediaUploadService._sendUploadRequest. However, CronetClient.send in cronet_http 1.10.1 drains the entire request with await stream.toBytes() and copies it into a Java buffer before starting the network request. Consequently, the composer's upload progress reaches 100% before transmission begins, and supported 100 MB attachments require additional full-body allocations that can cause memory failures on constrained devices. Keep streamed uploads on dart:io or use a genuinely streaming Cronet upload implementation. Validate backpressure through the selected production transport: the existing test uses _AbortRecorder, so it cannot detect this regression.
AGENTS.md reference: AGENTS.md:L243-L247
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
🤖 Fixed in 96aff92. On Android, streamed requests with a body (media and voice note uploads) now keep the handle's dart:io client, so upload progress and memory behave as before; streamed GETs and buffered requests still use Cronet. A new test covers the routing.
Signed-off-by: Tom Brow <[email protected]>
CronetClient reads a request body into memory before sending it, so upload progress would reach 100% before transmission and large attachments would be copied in full. Streamed requests with a body keep the handle's dart:io client on Android. Signed-off-by: Tom Brow <[email protected]>
490ca3f to
96aff92
Compare
kazemek21
left a comment
There was a problem hiding this comment.
Kyle reviewed this PR and the Nexus review-tracker notes and approved with no comments.
Posted by Codex (gpt-6.1-sol) on behalf of kylez
Follows #8897 (iOS HTTP/2), now on main.
What
Android gets the same app-wide shared sessions as iOS, backed by Cronet (
cronet_http) instead ofNSURLSession:cronet_http's changelog lists DNS failures with QUIC on cellular.dart:ioon Android:CronetClientreads a request body into memory before sending it, which would break upload progress and copy large attachments in full.dart:io, as today.NSURLSessionones, so invite and pairing errors keep their friendly messages.Measurements
Android emulator (API 36, arm64), profile builds of this branch and main, signed in to the Feedback community. 8 cold starts per build, alternating builds in blocks of 4. Times are for relay
/queryrequests started in the first 8 s.dart:io)Unexplained stall: most of main's startup gap is a ~5 s stall before its first response on every cold start. Its cause is unconfirmed; my guess is
dart:ioloading its CA store on the emulator, which HTTP/2 itself wouldn't fix, so the first two rows likely overstate the win on real devices. The rows after the first connection exclude the stall and are the fairer comparison. I also tried emulated EDGE throttling, but the emulator's throttle did not slow its traffic, so there are no slow-link numbers yet.Regressions
None found. Devices without Play services keep
dart:io. Cleartexthttp://requests keep the handle's owndart:ioclient, as on iOS.Testing
flutter test, 4784 tests).