Skip to content

v9.8.1-beta.2

v9.8.1-beta.2: fix: timeout on ios uploads on http 1.1 connection (#3787)
@isekovanic isekovanic tagged this 26 Aug 09:03 
## 🎯 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
Assets 2
Loading