fix: say why a download failed, and what that means for the other archives - #184
Merged
Conversation
`%lu` formats an unsigned long, and the code handed to it is a signed one that is negative for every URL loading error: `NSURLErrorNetworkConnectionLost` reached JS as 18446744073709550611 rather than -1005. The value was deterministic, so it could be mapped back, but nothing about it read as an error code - and the codes are the only stable way JS tells a connection that dropped apart from one that timed out, since the message alongside them is localized.
`URLConnection` reads a timeout of 0 as no timeout at all, which is what both downloads were opened with: `getInputStream()` and every `read()` after it could wait for as long as the operating system kept the socket alive. iOS has always bounded its download - 60 seconds of silence and the request fails - so a stalled download was a failure there and an indefinite wait here. A caller that gives up on its own clock does not close the connection underneath this, so the download went on running long after anyone was waiting for it, and reported whatever it eventually hit as if it had just happened. The read timeout bounds silence rather than the download, so a slow connection that keeps delivering is still never cut off.
Nothing read the response status, so a 4xx or a 5xx reached the caller as whatever `getInputStream()` happened to throw for it - an IOException among all the other IOExceptions a download can end in, with no way to tell a release served from a CDN that answered 503 apart from a connection that dropped. iOS has always refused a status of 400 or above, and names it the same way, so one release answered with one status now reads the same in both platforms' reports. The status is read before the body, because asking `getInputStream()` first turns some statuses into a stream over the error page and others into an exception that no longer knows which status it was.
A promise rejected with a bare throwable is given the code `EUNSPECIFIED` by React Native, so every way an Android download can fail arrived in JS under one code with only the message telling them apart - and the message is an exception's own words, which is not a value a report can group by. iOS has always carried the `NSURLError` code, so the same failure was classifiable on one platform and not on the other. The four categories are the ones that ask for different things to happen next. A connection that dropped is worth waiting out; a server that answered 404 is not the network's doing; an update that failed its integrity check will fail it again however many times it is downloaded. A category no caller would act differently on would only make the reports wider, which is why there is no category per way a download can fail.
A release published with a binary patch offers up to three archives, and the client moves on to the next one whenever it cannot use the one it has. A connection that dropped, timed out, or never opened was read that way too: the client fell back, and downloaded the full archive - the largest of the three, from nothing - over the network that had just failed to carry the smallest. So a download that had already failed spent a second, longer download to fail again, and reported the second failure as if the first had not happened. It now ends with what the network did. A server that answered is untouched by this: a 404 on one archive says nothing about the next, so the client still moves on the way it always has.
The bytes received were compared against `getContentLength()` whatever it answered, and it answers -1 for a body a server sends with no `Content-Length` - a length no read total can match, so every one of those downloads was refused as if it had arrived short. Nothing had reported it, which fits: the request asks for `identity` encoding and a CDN answers that with a length. It took a server that chose otherwise. A length the server never declared is nothing to check against, so it is no longer checked against.
The bytes stopped arriving partway through a body the server had declared the length of, and the count was the only thing that noticed - the socket raised nothing, so this arrived as an unnamed I/O error and was classified as one. That put it in the same bucket as a full disk, and it cost the patch archives their point: an archive cut off mid-download was read as an archive that could not be used, so the client fell back and asked for the full one - the largest of the three - over the connection that had just stopped delivering. It is a type of its own now, and named a network failure everywhere one is acted on.
The catch listed `IOException` and this package's unchecked exception, which covered what a download usually fails with and not what it rarely does. Two went past it: a download URL that is not a URL, and an archive naming a path outside the folder it unpacks into. Both ran on the background executor, which has nowhere to report an exception to, so the promise was never settled either way. JS was left waiting on a download that had already stopped, with no timeout of its own to end the wait - the one failure shape worse than a rejection. The download now settles its promise whatever it hits, and what it hit is classified the same way as everything else.
A diff that failed before its bundle was restored was read as a failure of the bundle patch, which every archive of a release carries byte for byte - so the patch archive was passed over as one that could only fail the same way. That holds for an applier that refused the patch. It does not hold for a server that answered the diff's URL with a status: the patch archive is at a URL of its own, and diffs are published one per recent version, so they are the first thing a retention policy clears out while the patch archive stays. So a release whose diff had been cleaned up downloaded the largest archive it has, with a patch archive sitting untried behind a 404 that said nothing about it. The two platforms tell the cases apart the same way now, from whether the archive was answered with a status. The iOS suite serves its archives as files, which have no status to answer with, so the status case is covered by the Android suite and by the error utils tests.
The client now keeps the patch archive when the diff's URL is answered with a status, and iOS had nothing exercising it: the package tests serve their archives as files, which have no status to answer with, so the download handler's whole error-status path has never been covered there. The mock server of the E2E suite serves over HTTP and already 404s a path it holds nothing for, so the scenario is a release published whole and then left with its diff archive deleted - the shape of one whose diff a retention policy cleared out while the archives at the other URLs stayed. What it pins is the pair the other assertions cannot tell apart on their own: the archives the app asked for, in order, and the reasons its own callback reported for them.
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
1 participant
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.
Background
A failed download said almost nothing about why it failed.
On Android every one of them reached JS under
EUNSPECIFIED, because the promise wasrejected with a bare throwable, leaving only the exception's own message to tell a dropped
connection apart from an update that failed its integrity check. The download had no
timeout of any kind —
URLConnectionreads a timeout of 0 as none at all — so a stalledone waited for as long as the operating system kept the socket alive, long after whoever
asked for it had given up on their own clock. And the response status was never read, so a
CDN answering 404 arrived as whatever
getInputStream()happened to throw for it.On iOS the error code was carried but formatted with
%lu, and every URL loading error'scode is negative:
NSURLErrorNetworkConnectionLostreached JS as 18446744073709550611.This matters more now that a release published with a binary patch offers three archives of
the same update. The client moves on to the next archive whenever it cannot use the one it
has, and cannot use was decided without knowing what had gone wrong. A connection that
dropped mid-download was read as a verdict on the archive, so the client fell back and
asked for the full archive — the largest of the three, from nothing — over the network that
had just failed to carry the smallest.
Changes
What a failure is called
Android names the kind of failure a download ended in, in four categories chosen for asking
different things to happen next rather than one per way a download can fail.
CODE_PUSH_NETWORKCODE_PUSH_HTTPCODE_PUSH_INTEGRITYCODE_PUSH_UNKNOWNA status of 400 or above is now read before the body and refused, the way iOS has always
refused it and with a message that reads the same. iOS keeps carrying the
NSURLErrorcodeand now prints it as the signed number it is.
The two platforms still shape the code differently, because they have different things to
name: iOS has a stable per-error code and Android has none. Both are documented in
Telemetry callbacks.
What bounds a download
Android opens its downloads with a 10s connect timeout and a 30s read timeout. The read
timeout bounds silence rather than the download, so a slow connection that keeps delivering
is never cut off however long the whole archive takes.
The received byte count is compared against the declared length only when the server
declared one. It always was, and
getContentLength()answers -1 for a body sent without aContent-Length— a length no read total can match, so every one of those downloads wasrefused as if it had arrived short. When the comparison does fail it now raises a type of
its own and is named a network failure: the bytes stopped arriving partway through, the
socket raised nothing for it, and only the count noticed.
What a failure means for the other archives
The fallback from one archive to the next is a run of verdicts on the archives, and two
kinds of failure are not verdicts on anything.
one is the largest, so the download ends with what the network did rather than spending a
second, longer download to fail the same way.
archive at another, and diffs are published one per recent version — the first thing a
retention policy clears out, while the patch archive at its own URL stays. A release whose
diff had been cleaned up used to download the largest archive it has with a perfectly good
patch archive sitting untried behind a 404.
Everything else is unchanged: a failure in the bundle patch every archive carries byte for
byte still skips ahead to the full archive, because retrying it could only fail the same way.
Settling the promise
The download's catch listed
IOExceptionand this package's unchecked exception, whichcovered what a download usually fails with and not what it rarely does. A download URL that
is not a URL, and an archive naming a path outside the folder it unpacks into, both went
past it — on the background executor, which has nowhere to report an exception to, so the
promise was never settled either way and JS was left waiting on a download that had already
stopped. It settles whatever it hits now.
What existing users see
onSyncError/CodePush.sync()rejectionerror.codeon Android is now one of the four categories instead ofEUNSPECIFIED. On iOS it is the same code as before, printed signed rather than as an unsigned long. Messages are unchanged apart from Android's new one for an error status.Content-Lengthnow installs instead of being refused.Verification
CodePushErrorCodeTestand five added download scenarios.CodePushErrorUtilsTests.diff archive deleted, so the client meets an actual 404.
Two gaps worth naming. The E2E scenario was not run against the code before this branch, so
it is confirmed to pass rather than confirmed to discriminate. And iOS has no unit coverage
of the error-status path at all —
CodePushPackageTestsserves its archives as files, whichhave no status to answer with — which is why the scenario above was added to the E2E suite
rather than there.