fix: timeout on ios uploads on http 1.1 connection - #3787
Open
isekovanic wants to merge 1 commit into
Open
Conversation
Contributor
SDK Size
|
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.
🎯 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
useNativeMultipartUploadspecifically.The
CFNetworktrace of a failing upload looks something like this: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
NSInputStreamsubclass.CFNetworkdrives anHTTP/1.1request body through theCFReadStreaminterface, which a plainInputStreamsubclass cannot participate in, so it can never report end-of-stream.CFNetworkstops callingreadthe momentContent-Lengthis satisfied, so the subclass never returned 0, never reached.atEndand soCFNetworknever 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 advertiseh2, a TLS intercepting proxy or a local debugging proxy such asCharlesorProxymanwill all put us there. OverHTTP/2andHTTP/3the request body is framed and terminated byContent-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 handsURLSessionthe read end of aCFStreamCreateBoundPair, so a realCFReadStreamthat reports every event and a newStreamMultipartBodyProducerfeeds the write end from the same element list. Closing the write end is what tellsCFNetworkthe 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
StreamMultipartBodyErrorBoxand preferred over the transport error indidCompleteWithError. The box is per attempt so thenURLSessioncan 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 KiBof extra memory per request (which is nothing). In turn we handle a lot of odd edge cases, like:And naturally,
HTTP/1.1no longer fails uploads.🎨 UI Changes
🧪 Testing
☑️ Checklist
developbranch