Skip to content

Commit 824818b

Browse files
authored
fix: timeout on ios uploads on http 1.1 connection (#3787)
## 🎯 Goal This PR fixes iOS attachment uploads hanging for 60s and then failing with `-1001` (timed out) after the server has already accepted the file. Affects apps using `useNativeMultipartUpload` specifically. The `CFNetwork` trace of a failing upload looks something like this: ``` resuming, timeouts(60.0, …) received response, status 201 ← server took the file …60 s of nothing… finished with error [-1001] summary for task failure { response_status=201, request_bytes=2810041, response_duration_ms=0, protocol="http/1.1" } ``` So the upload succeeds and the client throws it away a minute later. The user sees a failed attachment (and the message with the attachment is not sent consequently). ## 🛠 Implementation details The root cause is unfortunately not as simple. The multipart body was pretty much a hand rolled `NSInputStream` subclass. `CFNetwork` drives an `HTTP/1.1` request body through the `CFReadStream` interface, which a plain `InputStream` subclass cannot participate in, so it can never report end-of-stream. `CFNetwork` stops calling `read` the moment `Content-Length` is satisfied, so the subclass never returned 0, never reached `.atEnd` and so `CFNetwork` never learned the body had ended. The transaction stayed open until the timeout (which is 60 seconds later). This isn't something that would fire all the time, however. It happens whenever the connection ends up on `HTTP/1.1`. That's decided on a much lower level (during the TLS handshake), so it's server and network determined rather than something the app controls, so if an endpoint that doesn't advertise `h2`, a TLS intercepting proxy or a local debugging proxy such as `Charles` or `Proxyman` will all put us there. Over `HTTP/2` and `HTTP/3` the request body is framed and terminated by `Content-Length`, so the missing end-of-stream signal never mattered and the bug stayed dormant. Hence, why this has gone unnoticed for so long. Naturally, this is an edge case altogether but I've decided to rewrite chunks of the body stream class. There have always existed some certain parts of it that bothered me and we attempt to address them here as well. `makeStream()` now hands `URLSession` the read end of a `CFStreamCreateBoundPair`, so a real `CFReadStream` that reports every event and a new `StreamMultipartBodyProducer` feeds the write end from the same element list. Closing the write end is what tells `CFNetwork` the body is complete. The producer is driven by GCD (`CFWriteStreamSetClient` + `CFWriteStreamSetDispatchQueue`) rather than a run loop, so it owns no thread and can't outlive its work. Because a bound pair has no error channel, body production failures are recorded in a `StreamMultipartBodyErrorBox` and preferred over the transport error in `didCompleteWithError`. The box is **per attempt** so then `URLSession` can request a fresh body stream on retry and an abandoned attempt's failure must not fail a later one that succeeds. Aside from a bit of code complexity, this also costs about `128 KiB` of extra memory per request (which is nothing). In turn we handle a lot of odd edge cases, like: - no longer relying on kind of undocumented behaviour - failures actually get reported every time now - backpressure is now being handled instead of just falsely quitting the upload - retry scoping is now strictly And naturally, `HTTP/1.1` no longer fails uploads. ## 🎨 UI Changes ## 🧪 Testing ## ☑️ Checklist - [x] I have signed the [Stream CLA](https://docs.google.com/forms/d/e/1FAIpQLScFKsKkAJI7mhCr7K9rEIOpqIDThrWxuvxnwUq2XkHyG154vQ/viewform) (required) - [x] PR targets the `develop` branch - [ ] Documentation is updated - [x] New code is tested in main example apps, including all possible scenarios - [x] SampleApp iOS and Android - [x] Expo iOS and Android
1 parent a72b6b3 commit 824818b

2 files changed

Lines changed: 296 additions & 103 deletions

File tree

0 commit comments

Comments
 (0)