diff --git a/android/app/src/main/java/com/microsoft/codepush/react/ArchiveAttemptLog.java b/android/app/src/main/java/com/microsoft/codepush/react/ArchiveAttemptLog.java index 56d0569a..48a4843f 100644 --- a/android/app/src/main/java/com/microsoft/codepush/react/ArchiveAttemptLog.java +++ b/android/app/src/main/java/com/microsoft/codepush/react/ArchiveAttemptLog.java @@ -43,6 +43,9 @@ private static class Attempt { String fallbackReason; long durationMs = -1; + /** Whether the server answered this archive's URL with a status instead of the archive. */ + boolean wasNotServed; + Attempt(String archive) { this.archive = archive; } @@ -72,14 +75,25 @@ void recordBundleRestored(long applyDurationMs) { /** * Whether the current archive's attempt got as far as restoring the bundle. A failure - * after that point is on the asset side of the archive - and every archive carries the - * same bundle patch, so this is what decides whether the patch archive is worth trying - * after a failed diff. + * after that point is on the asset side of the archive, which the archives do not share - + * unlike the bundle patch, which they carry byte for byte. */ boolean currentAttemptRestoredBundle() { return current().applyDurationMs >= 0; } + /** + * Whether the current archive never arrived, because the server answered its URL with a + * status rather than with the archive. + * + * A verdict on one URL, and the archives are at URLs of their own: a release whose diff + * has been cleaned up still has its patch archive. This is the one failure before the + * bundle is restored that says nothing about the archives left to try. + */ + boolean currentAttemptWasNotServed() { + return current().wasNotServed; + } + /** * The attempt at the current archive ended in one of the reasons the appliers report. * @@ -104,7 +118,8 @@ void recordFallback(String failureReason) { * here would put a value on the wire that no platform reports, so the fallback is * reported without a reason. */ - void recordFallbackAfterError() { + void recordFallbackAfterError(Throwable error) { + current().wasNotServed = CodePushErrorCode.HTTP.equals(CodePushErrorCode.of(error)); recordFallback(currentAttemptRestoredBundle() ? ArchiveRestoreResult.REASON_PACKAGE_VERIFICATION_FAILED : null); } diff --git a/android/app/src/main/java/com/microsoft/codepush/react/CodePushConstants.java b/android/app/src/main/java/com/microsoft/codepush/react/CodePushConstants.java index e70fdfcf..127bdab2 100644 --- a/android/app/src/main/java/com/microsoft/codepush/react/CodePushConstants.java +++ b/android/app/src/main/java/com/microsoft/codepush/react/CodePushConstants.java @@ -36,6 +36,24 @@ public class CodePushConstants { public static final String DEFAULT_JS_BUNDLE_NAME = "index.android.bundle"; public static final String DIFF_MANIFEST_FILE_NAME = "hotcodepush.json"; public static final int DOWNLOAD_BUFFER_SIZE = 1024 * 256; + + /** + * How long a download waits for the server to answer at all. + * + * Kept well above any real handshake, because the only thing this has to catch is a + * host that will never answer - a captive portal, a dead route, a network that went + * away between the update check and the download. + */ + public static final int DOWNLOAD_CONNECT_TIMEOUT_IN_MS = 10 * 1000; + + /** + * How long a download waits for the next bytes of a response already flowing. + * + * This bounds silence, not the download: a slow connection that keeps delivering is + * never cut off, however long the whole archive takes. A connection that delivers + * nothing for this long is one the operating system has stopped reporting as dead. + */ + public static final int DOWNLOAD_READ_TIMEOUT_IN_MS = 30 * 1000; public static final String DOWNLOAD_FILE_NAME = "download.zip"; public static final String DOWNLOAD_PROGRESS_EVENT_NAME = "CodePushDownloadProgress"; public static final String DOWNLOAD_URL_KEY = "downloadUrl"; diff --git a/android/app/src/main/java/com/microsoft/codepush/react/CodePushErrorCode.java b/android/app/src/main/java/com/microsoft/codepush/react/CodePushErrorCode.java new file mode 100644 index 00000000..151af2af --- /dev/null +++ b/android/app/src/main/java/com/microsoft/codepush/react/CodePushErrorCode.java @@ -0,0 +1,86 @@ +package com.microsoft.codepush.react; + +import java.net.SocketException; +import java.net.SocketTimeoutException; +import java.net.UnknownHostException; + +import javax.net.ssl.SSLException; + +/** + * What kind of failure a download ended in, in the words JS reads it by. + * + * React Native fills the code of a promise rejected with a bare throwable with + * `EUNSPECIFIED`, which left the message the only thing telling one failure apart from + * another - and a message is written for a person, not for a report to group by. + * + * The categories are the ones that ask for different things to happen next, which is why + * there are four of them rather than one per way a download can fail. A category no + * caller would act differently on is a category that only makes the reports wider. + */ +public class CodePushErrorCode { + + /** The network did not carry the download: the socket dropped, timed out, or never opened. */ + public static final String NETWORK = "CODE_PUSH_NETWORK"; + + /** The server answered, with a status that is not a body to install. */ + public static final String HTTP = "CODE_PUSH_HTTP"; + + /** + * The downloaded contents do not hash to the release's package hash, or hold no JS + * bundle by the name the app looks for. Downloading them again cannot help. + */ + public static final String INTEGRITY = "CODE_PUSH_INTEGRITY"; + + /** Nothing here has a word for it, and inventing one would only be a guess. */ + public static final String UNKNOWN = "CODE_PUSH_UNKNOWN"; + + private CodePushErrorCode() { + } + + /** + * The category of a failure, read through its causes: the download wraps some of what it + * catches, and the wrapper is never the part that says what went wrong. + */ + public static String of(Throwable error) { + for (Throwable cause = error; cause != null; cause = cause.getCause()) { + if (cause instanceof CodePushHttpException) { + return HTTP; + } + + if (cause instanceof CodePushInvalidUpdateException) { + return INTEGRITY; + } + + if (isTransportFailure(cause)) { + return NETWORK; + } + } + + return UNKNOWN; + } + + /** + * Whether the download failed because the network did not carry it. + * + * A server that answered is not this, however it answered: the connection worked, and + * asking a different URL over it is worth doing. A connection that never opened or that + * dropped is, and every URL behind it is equally out of reach. + */ + public static boolean isNetworkFailure(Throwable error) { + return NETWORK.equals(of(error)); + } + + private static boolean isTransportFailure(Throwable error) { + // `SocketException` is the one that covers the reported majority - the connection + // reset and the connection aborted an app being backgrounded mid-download leaves + // behind - along with the connection that was refused or had no route. + // + // A download that stopped short belongs here too. The socket raised nothing for it, + // so only the byte count says the network dropped the rest of the body. + return error instanceof SocketTimeoutException + || error instanceof SocketException + || error instanceof UnknownHostException + || error instanceof SSLException + || error instanceof CodePushIncompleteDownloadException; + } +} diff --git a/android/app/src/main/java/com/microsoft/codepush/react/CodePushHttpException.java b/android/app/src/main/java/com/microsoft/codepush/react/CodePushHttpException.java new file mode 100644 index 00000000..fd419b9c --- /dev/null +++ b/android/app/src/main/java/com/microsoft/codepush/react/CodePushHttpException.java @@ -0,0 +1,29 @@ +package com.microsoft.codepush.react; + +import java.io.IOException; + +/** + * The server answered a download with a status that is not a body to install. + * + * An I/O exception rather than one of this package's unchecked ones, because every caller + * of a download already handles `IOException` and this is one more way a download does not + * arrive - and because the alternative, an unchecked exception, would escape the download + * path uncaught. + * + * The message reads the way the other platform's does, so one release answered with the + * same status reads the same in both platforms' reports. + */ +public class CodePushHttpException extends IOException { + + private final int mStatusCode; + + public CodePushHttpException(String url, int statusCode) { + super("Received " + statusCode + " response from " + url); + mStatusCode = statusCode; + } + + /** The status the server answered with. */ + public int getStatusCode() { + return mStatusCode; + } +} diff --git a/android/app/src/main/java/com/microsoft/codepush/react/CodePushIncompleteDownloadException.java b/android/app/src/main/java/com/microsoft/codepush/react/CodePushIncompleteDownloadException.java new file mode 100644 index 00000000..37f95c16 --- /dev/null +++ b/android/app/src/main/java/com/microsoft/codepush/react/CodePushIncompleteDownloadException.java @@ -0,0 +1,17 @@ +package com.microsoft.codepush.react; + +import java.io.IOException; + +/** + * The response stopped before it had delivered the length it declared. + * + * A type of its own rather than one more unnamed I/O error, because this is a network + * failure the socket never raised one for: the bytes simply stopped arriving, and only the + * count says so. Everywhere a network failure is acted on has to act on this one too. + */ +public class CodePushIncompleteDownloadException extends IOException { + + public CodePushIncompleteDownloadException(long receivedBytes, long declaredBytes) { + super("Received " + receivedBytes + " bytes, expected " + declaredBytes); + } +} diff --git a/android/app/src/main/java/com/microsoft/codepush/react/CodePushNativeModule.java b/android/app/src/main/java/com/microsoft/codepush/react/CodePushNativeModule.java index 4a9f1963..79c4ec20 100644 --- a/android/app/src/main/java/com/microsoft/codepush/react/CodePushNativeModule.java +++ b/android/app/src/main/java/com/microsoft/codepush/react/CodePushNativeModule.java @@ -402,10 +402,15 @@ public void doFrame(long frameTimeNanos) { } catch (CodePushInvalidUpdateException e) { CodePushUtils.log(e); mSettingsManager.saveFailedUpdate(CodePushUtils.convertReadableToJsonObject(updatePackage)); - promise.reject(e); - } catch (IOException | CodePushUnknownException e) { + promise.reject(CodePushErrorCode.of(e), e.getMessage(), e); + } catch (Throwable e) { + // Anything at all, because a promise is waiting on this. A failure that + // leaves this method without settling it leaves JS waiting on a download + // that is no longer running, with nothing to time it out. The narrower + // catch this replaces let two through: a download URL that is not a URL, + // and an archive naming a path outside the folder it unpacks into. CodePushUtils.log(e); - promise.reject(e); + promise.reject(CodePushErrorCode.of(e), e.getMessage(), e); } } }); diff --git a/android/app/src/main/java/com/microsoft/codepush/react/CodePushUpdateManager.java b/android/app/src/main/java/com/microsoft/codepush/react/CodePushUpdateManager.java index f8e0d887..8835d8e8 100644 --- a/android/app/src/main/java/com/microsoft/codepush/react/CodePushUpdateManager.java +++ b/android/app/src/main/java/com/microsoft/codepush/react/CodePushUpdateManager.java @@ -180,12 +180,15 @@ public JSONObject downloadPackage(JSONObject updatePackage, String expectedBundl return patchAttempt.result(); } - // A diff that failed before its bundle was restored failed in the bundle patch, - // which the patch archive carries byte for byte - it would fail the same way, - // and trying it would only put a second doomed download in front of the full - // one. A failure after the restore is on the asset side, which the patch - // archive does not share. - patchArchiveWorthTrying = patchArchiveWorthTrying && patchAttempt.currentAttemptRestoredBundle(); + // The patch archive is worth trying when nothing about how the diff failed + // implicates it. A diff that failed after restoring its bundle failed on its + // asset side, which the patch archive does not share; a diff the server never + // served is a verdict on one URL, and the patch archive is at another. Anything + // else failed in the bundle patch both archives carry byte for byte, so the + // patch archive would fail the same way and trying it would only put a second + // doomed download in front of the full one. + patchArchiveWorthTrying = patchArchiveWorthTrying + && (patchAttempt.currentAttemptRestoredBundle() || patchAttempt.currentAttemptWasNotServed()); } if (patchArchiveWorthTrying) { @@ -218,8 +221,9 @@ private static String optArchiveDownloadUrl(JSONObject updatePackage, String dow /** * Installs the update from one of its patch archives. * - * Every way this can fail ends the same way, with the caller moving on to the next - * archive, so none of it is reported to the caller as an error. The ladder cannot loop: + * Every verdict on the archive ends the same way, with the caller moving on to the next + * one, so none of it is reported to the caller as an error. A network that did not carry + * the archive is not a verdict on it and is raised instead. The ladder cannot loop: * which archive comes next is the caller's decision alone, and the full archive at its * end is downloaded by a call that is not allowed to take the patch path, so it has no * failure of its own to fall back from. @@ -228,10 +232,13 @@ private static String optArchiveDownloadUrl(JSONObject updatePackage, String dow * whoever asked for the download * @return true when the update was installed, false when the caller has to move on to * the next archive + * @throws IOException when the network did not carry the archive, which is not a verdict + * on the archive and so is not a reason to try another one */ private boolean tryDownloadArchivePackage(JSONObject updatePackage, String expectedBundleFileName, DownloadProgressCallback progressCallback, - String archiveDownloadUrl, ArchiveAttemptLog patchAttempt) { + String archiveDownloadUrl, ArchiveAttemptLog patchAttempt) + throws IOException { try { ArchiveRestoreResult patchResult = downloadAndInstallPackage(updatePackage, expectedBundleFileName, progressCallback, archiveDownloadUrl, true, patchAttempt); @@ -243,11 +250,27 @@ private boolean tryDownloadArchivePackage(JSONObject updatePackage, String expec CodePushUtils.log("The " + patchAttempt.currentArchive() + " archive failed (" + patchResult.getFailureReason() + "). Falling back."); } catch (Exception | OutOfMemoryError e) { + if (CodePushErrorCode.isNetworkFailure(e)) { + // The network is what failed, not the archive, and the full archive is behind + // the same network - only larger, and started over from nothing. Falling back + // here would spend a second download to reach the failure already in hand. + CodePushUtils.log("The " + patchAttempt.currentArchive() + + " archive could not be downloaded. Giving up on the download."); + if (e instanceof IOException) { + throw (IOException) e; + } + + // A network failure read out of a wrapper this package raised, which the + // caller catches by its own type rather than by `IOException`. + throw new CodePushUnknownException( + "The " + patchAttempt.currentArchive() + " archive could not be downloaded.", e); + } + // Applying a patch is the one path that holds a whole bundle in memory, so // running out of it is a failure this has to absorb like any other: by the time // it lands here the arrays are unreachable, and the full archive is downloaded // to disk in chunks rather than held. - patchAttempt.recordFallbackAfterError(); + patchAttempt.recordFallbackAfterError(e); CodePushUtils.log(e); CodePushUtils.log("The " + patchAttempt.currentArchive() + " archive could not be applied. Falling back."); @@ -296,6 +319,8 @@ ArchiveRestoreResult downloadAndInstallPackage(JSONObject updatePackage, String try { URL downloadUrl = new URL(downloadUrlString); connection = (HttpURLConnection) (downloadUrl.openConnection()); + connection.setConnectTimeout(CodePushConstants.DOWNLOAD_CONNECT_TIMEOUT_IN_MS); + connection.setReadTimeout(CodePushConstants.DOWNLOAD_READ_TIMEOUT_IN_MS); if (android.os.Build.VERSION.SDK_INT < Build.VERSION_CODES.LOLLIPOP && downloadUrl.toString().startsWith("https")) { @@ -307,6 +332,15 @@ ArchiveRestoreResult downloadAndInstallPackage(JSONObject updatePackage, String } connection.setRequestProperty("Accept-Encoding", "identity"); + + // Read before the body is: an error status carries a body of its own, and + // asking `getInputStream()` for it first turns some statuses into a stream and + // others into an exception that says nothing about which status it was. + int responseCode = connection.getResponseCode(); + if (responseCode >= 400) { + throw new CodePushHttpException(downloadUrlString, responseCode); + } + bin = new BufferedInputStream(connection.getInputStream()); // Announced only once the response is flowing, so a connection that fails to @@ -347,8 +381,11 @@ ArchiveRestoreResult downloadAndInstallPackage(JSONObject updatePackage, String progressCallback.call(new DownloadProgress(totalBytes, receivedBytes)); } - if (totalBytes != receivedBytes) { - throw new CodePushUnknownException("Received " + receivedBytes + " bytes, expected " + totalBytes); + // Only against a length the server declared. `getContentLength()` answers -1 + // for a body sent without one, which no read total matches - so comparing anyway + // would fail every download a server chooses to send that way. + if (totalBytes >= 0 && totalBytes != receivedBytes) { + throw new CodePushIncompleteDownloadException(receivedBytes, totalBytes); } isZip = ByteBuffer.wrap(header).getInt() == 0x504b0304; @@ -507,6 +544,14 @@ public void downloadAndReplaceCurrentBundle(String remoteBundleUrl, String bundl try { downloadUrl = new URL(remoteBundleUrl); connection = (HttpURLConnection) (downloadUrl.openConnection()); + connection.setConnectTimeout(CodePushConstants.DOWNLOAD_CONNECT_TIMEOUT_IN_MS); + connection.setReadTimeout(CodePushConstants.DOWNLOAD_READ_TIMEOUT_IN_MS); + + int responseCode = connection.getResponseCode(); + if (responseCode >= 400) { + throw new CodePushHttpException(remoteBundleUrl, responseCode); + } + bin = new BufferedInputStream(connection.getInputStream()); File downloadFile = new File(getCurrentPackageBundlePath(bundleFileName)); downloadFile.delete(); diff --git a/android/app/src/test/java/com/microsoft/codepush/react/CodePushErrorCodeTest.java b/android/app/src/test/java/com/microsoft/codepush/react/CodePushErrorCodeTest.java new file mode 100644 index 00000000..d5cced6f --- /dev/null +++ b/android/app/src/test/java/com/microsoft/codepush/react/CodePushErrorCodeTest.java @@ -0,0 +1,90 @@ +package com.microsoft.codepush.react; + +import static org.junit.Assert.assertEquals; +import static org.junit.Assert.assertFalse; +import static org.junit.Assert.assertTrue; + +import org.junit.Test; + +import java.io.IOException; +import java.net.ConnectException; +import java.net.MalformedURLException; +import java.net.SocketException; +import java.net.SocketTimeoutException; +import java.net.UnknownHostException; + +import javax.net.ssl.SSLHandshakeException; + +public class CodePushErrorCodeTest { + + @Test + public void namesAConnectionThatDroppedAsANetworkFailure() { + // The message the reported majority of Android failures arrive with. + assertEquals(CodePushErrorCode.NETWORK, + CodePushErrorCode.of(new SocketException("Software caused connection abort"))); + } + + @Test + public void namesAConnectionThatTimedOutAsANetworkFailure() { + assertEquals(CodePushErrorCode.NETWORK, CodePushErrorCode.of(new SocketTimeoutException("timeout"))); + } + + @Test + public void namesAConnectionThatNeverOpenedAsANetworkFailure() { + assertEquals(CodePushErrorCode.NETWORK, CodePushErrorCode.of(new ConnectException("Connection refused"))); + assertEquals(CodePushErrorCode.NETWORK, CodePushErrorCode.of(new UnknownHostException("cdn.example.test"))); + assertEquals(CodePushErrorCode.NETWORK, CodePushErrorCode.of(new SSLHandshakeException("handshake failed"))); + } + + @Test + public void namesADownloadThatStoppedShortAsANetworkFailure() { + // The socket raised nothing for it, so only the byte count says the body was cut off. + Throwable error = new CodePushIncompleteDownloadException(1024, 4096); + + assertEquals(CodePushErrorCode.NETWORK, CodePushErrorCode.of(error)); + assertTrue(CodePushErrorCode.isNetworkFailure(error)); + } + + @Test + public void namesAnErrorStatusAsAnHttpFailureRatherThanANetworkOne() { + Throwable error = new CodePushHttpException("https://cdn.example.test/full.zip", 503); + + assertEquals(CodePushErrorCode.HTTP, CodePushErrorCode.of(error)); + assertFalse("a server that answered is not a network that failed", + CodePushErrorCode.isNetworkFailure(error)); + } + + @Test + public void namesAnUpdateThatIsNotWhatItClaimedAsAnIntegrityFailure() { + assertEquals(CodePushErrorCode.INTEGRITY, + CodePushErrorCode.of(new CodePushInvalidUpdateException("The update contents failed the data integrity check."))); + } + + @Test + public void readsThroughTheWrapperADownloadCatchesItsFailuresIn() { + Throwable wrapped = new CodePushUnknownException("Error closing IO resources.", + new SocketException("Connection reset")); + + assertEquals(CodePushErrorCode.NETWORK, CodePushErrorCode.of(wrapped)); + assertTrue(CodePushErrorCode.isNetworkFailure(wrapped)); + } + + @Test + public void namesTheFailuresThatUsedToEscapeTheDownloadUncaught() { + // Neither is an `IOException`, so the download's old catch let them past and left + // the promise waiting on it unsettled. They are classified like anything else now. + assertEquals(CodePushErrorCode.UNKNOWN, + CodePushErrorCode.of(new CodePushMalformedDataException("not a url", new MalformedURLException()))); + assertEquals(CodePushErrorCode.UNKNOWN, + CodePushErrorCode.of(new IllegalStateException("File is outside extraction target directory."))); + } + + @Test + public void leavesAFailureNothingHasAWordForUnnamed() { + Throwable error = new IOException("the disk is full"); + + assertEquals(CodePushErrorCode.UNKNOWN, CodePushErrorCode.of(error)); + assertFalse("an unnamed failure must not be retried as if the network caused it", + CodePushErrorCode.isNetworkFailure(error)); + } +} diff --git a/android/app/src/test/java/com/microsoft/codepush/react/CodePushUpdateManagerDownloadTest.java b/android/app/src/test/java/com/microsoft/codepush/react/CodePushUpdateManagerDownloadTest.java index 8bc23645..54b3ca6b 100644 --- a/android/app/src/test/java/com/microsoft/codepush/react/CodePushUpdateManagerDownloadTest.java +++ b/android/app/src/test/java/com/microsoft/codepush/react/CodePushUpdateManagerDownloadTest.java @@ -2,6 +2,7 @@ import static org.junit.Assert.assertArrayEquals; import static org.junit.Assert.assertEquals; +import static org.junit.Assert.fail; import static org.junit.Assert.assertFalse; import static org.junit.Assert.assertTrue; @@ -138,6 +139,57 @@ public void reportsAnInvalidManifestWhenThePatchUrlDoesNotServeAnArchive() throw assertFalse("bytes that are not an update must not reach the package folder", mPackageFolder.exists()); } + @Test + public void failsTheDownloadWhenTheServerAnswersTheArchiveWithAnErrorStatus() throws IOException { + // Nothing is served at this path, so the server answers it with a 404. + String fullUrl = mServer.urlOf("/missing-full.zip"); + + try { + updateManager(applierWriting(TARGET_BUNDLE)).downloadPackage( + fullUpdatePackage(mPackageHash, fullUrl), BUNDLE_FILE_NAME, ignoreProgress()); + fail("a download the server refused must not be reported as installed"); + } catch (CodePushHttpException e) { + assertEquals(404, e.getStatusCode()); + assertTrue("the message names the status the server answered with", + e.getMessage().contains("404")); + } + + assertFalse("nothing the server refused reaches the package folder", mPackageFolder.exists()); + } + + @Test + public void givesUpTheDownloadWhenThePatchArchiveArrivesShort() throws IOException { + // The connection carried part of the body and stopped. The full archive is behind + // the same connection and is larger, so there is nothing to fall back to. + byte[] patchArchive = zipOf(patchArchiveContents()); + String patchUrl = mServer.serveClaimingLength("/patch.zip", patchArchive, patchArchive.length + 1024); + String fullUrl = serve("/full.zip", zipOf(fullArchiveContents())); + + try { + updateManager(applierWriting(TARGET_BUNDLE)) + .downloadPackage(updatePackage(fullUrl, patchUrl), BUNDLE_FILE_NAME, ignoreProgress()); + fail("a download that stopped short must not be reported as installed"); + } catch (IOException e) { + assertEquals(CodePushErrorCode.NETWORK, CodePushErrorCode.of(e)); + } + + assertEquals("the full archive is behind the connection that just stopped", + Arrays.asList("/patch.zip"), mServer.requestedPaths()); + assertFalse("nothing that arrived short reaches the package folder", mPackageFolder.exists()); + } + + @Test + public void installsAnUpdateTheServerSentWithoutDeclaringItsLength() throws IOException { + // `getContentLength()` answers -1 for a body sent with no `Content-Length`, which no + // read total matches - so a download checked against it anyway could never arrive. + String fullUrl = mServer.serveWithoutContentLength("/full.zip", zipOf(fullArchiveContents())); + + updateManager(applierWriting(TARGET_BUNDLE)).downloadPackage( + fullUpdatePackage(mPackageHash, fullUrl), BUNDLE_FILE_NAME, ignoreProgress()); + + assertInstalledContents(); + } + @Test public void fallsBackToTheFullArchiveWhenApplyingThePatchRunsOutOfMemory() throws IOException { String patchUrl = serve("/patch.zip", zipOf(patchArchiveContents())); @@ -307,10 +359,11 @@ public void fallsBackToThePatchArchiveWhenTheAssetDiffManifestDoesNotNameTheFile } @Test - public void skipsThePatchArchiveWhenTheAssetDiffCannotBeDownloaded() throws IOException { - // A diff that never arrived left no verdict at all: nothing says the patch archive - // is any better off, and the full download is the one that cannot fail - so a - // client is never walked through two doomed downloads on its way there. + public void triesThePatchArchiveWhenTheServerDoesNotServeTheAssetDiff() throws IOException { + // A 404 is a verdict on the URL it was asked of. Diffs are published one per recent + // version and are the first thing a retention policy clears out, while the patch + // archive at its own URL stays - so nothing about a diff that is gone says the patch + // archive is. Map updateContents = assetDiffTargetContents(); String updateHash = packageHashOf(updateContents); String diffUrl = mServer.urlOf("/missing-diff.zip"); @@ -320,13 +373,64 @@ public void skipsThePatchArchiveWhenTheAssetDiffCannotBeDownloaded() throws IOEx JSONObject patchResult = updateManager(applierWriting(TARGET_BUNDLE)).downloadPackage( updatePackageWithAssetDiff(updateHash, fullUrl, patchUrl, diffUrl), BUNDLE_FILE_NAME, ignoreProgress()); - assertEquals(Arrays.asList("/missing-diff.zip", "/full.zip"), mServer.requestedPaths()); - assertFallbackResult(patchResult, null); - assertEquals("asset-diff", patchResult.optString("archive", null)); + assertEquals("the full archive is not downloaded when the patch archive installs", + Arrays.asList("/missing-diff.zip", "/patch.zip"), mServer.requestedPaths()); + assertEquals("applied", patchResult.optString("status", null)); + assertEquals("binary-patch", patchResult.optString("archive", null)); + assertEquals(2, patchResult.optJSONArray("attempts").length()); + assertInstalledContents(updateHash, updateContents); + } + + @Test + public void skipsThePatchArchiveWhenTheAssetDiffFailsInItsBundlePatch() throws IOException { + // Both archives carry that patch byte for byte, so an applier that refused it here + // would refuse it there - and trying it would put a second doomed download in front + // of the full one. + Map updateContents = assetDiffTargetContents(); + String updateHash = packageHashOf(updateContents); + String diffUrl = serve("/diff.zip", zipOf(assetDiffArchiveContents(DROPPED_ASSET_PATH))); + String patchUrl = serve("/patch.zip", zipOf(patchArchiveContentsForAssetDiffTarget())); + String fullUrl = serve("/full.zip", zipOf(updateContents)); + + JSONObject patchResult = updateManager(applierRefusingThePatch()).downloadPackage( + updatePackageWithAssetDiff(updateHash, fullUrl, patchUrl, diffUrl), BUNDLE_FILE_NAME, ignoreProgress()); + + assertEquals(Arrays.asList("/diff.zip", "/full.zip"), mServer.requestedPaths()); assertEquals(1, patchResult.optJSONArray("attempts").length()); assertInstalledContents(updateHash, updateContents); } + @Test + public void givesUpTheDownloadWhenTheNetworkCannotCarryTheAssetDiff() throws IOException { + // A refused connection is the network failing rather than a verdict on the archive, + // and the archives behind it are behind the same network - the full one only larger + // and started over from nothing. + String unreachableDiffUrl = "http://127.0.0.1:" + portNothingListensOn() + "/diff.zip"; + String patchUrl = serve("/patch.zip", zipOf(patchArchiveContents())); + String fullUrl = serve("/full.zip", zipOf(fullArchiveContents())); + + try { + updateManager(applierWriting(TARGET_BUNDLE)).downloadPackage( + updatePackageWithAssetDiff(mPackageHash, fullUrl, patchUrl, unreachableDiffUrl), + BUNDLE_FILE_NAME, ignoreProgress()); + fail("a network that carried nothing must not be reported as an installed update"); + } catch (IOException e) { + assertEquals(CodePushErrorCode.NETWORK, CodePushErrorCode.of(e)); + } + + assertTrue("no archive behind the same network is asked for", mServer.requestedPaths().isEmpty()); + assertFalse("nothing that never arrived reaches the package folder", mPackageFolder.exists()); + } + + + /** A loopback port that is opened only to be closed, so connecting to it is refused. */ + private static int portNothingListensOn() throws IOException { + ServerSocket socket = new ServerSocket(0, 0, InetAddress.getByName("127.0.0.1")); + int port = socket.getLocalPort(); + socket.close(); + return port; + } + @Test public void installsFromThePatchArchiveWhenTheAssetDiffUrlIsAnEmptyString() throws IOException { // An empty slot is not an archive on offer. Attempting it would fail for want of a @@ -500,6 +604,16 @@ public byte[] readBaseBundle(String bundleFileName) { return new CodePushUpdateManager(mDocumentsDirectory, binaryPatch); } + /** An applier that refuses the bundle patch, which every archive of a release carries. */ + private static CodePushBinaryPatch.PatchApplier applierRefusingThePatch() { + return new CodePushBinaryPatch.PatchApplier() { + @Override + public int apply(byte[] base, byte[] patch, String outputPath, long expectedTargetSize) { + return RESULT_APPLY_FAILED; + } + }; + } + private static CodePushBinaryPatch.PatchApplier applierWriting(final byte[] restoredBundle) { return new CodePushBinaryPatch.PatchApplier() { @Override @@ -633,7 +747,10 @@ private String serve(String path, byte[] body) { private static class TestArchiveServer { private final ServerSocket mSocket; + private static final long NO_CONTENT_LENGTH = -1; + private final Map mBodies = new HashMap<>(); + private final Map mDeclaredLengths = new HashMap<>(); private final List mRequestedPaths = Collections.synchronizedList(new ArrayList()); TestArchiveServer() throws IOException { @@ -653,6 +770,18 @@ synchronized String serve(String path, byte[] body) { return urlOf(path); } + /** Serves a body under a `Content-Length` of the server's choosing rather than its own. */ + synchronized String serveClaimingLength(String path, byte[] body, long declaredLength) { + mBodies.put(path, body); + mDeclaredLengths.put(path, declaredLength); + return urlOf(path); + } + + /** Serves a body with no `Content-Length` at all, which the client reads until it closes. */ + synchronized String serveWithoutContentLength(String path, byte[] body) { + return serveClaimingLength(path, body, NO_CONTENT_LENGTH); + } + /** The URL of a path this server answers - with a 404, when nothing is served there. */ String urlOf(String path) { return "http://127.0.0.1:" + mSocket.getLocalPort() + path; @@ -704,8 +833,17 @@ private void respond(Socket connection) throws IOException { if (body == null) { response.write(bytes("HTTP/1.1 404 Not Found\r\nContent-Length: 0\r\nConnection: close\r\n\r\n")); } else { - response.write(bytes("HTTP/1.1 200 OK\r\nContent-Length: " + body.length - + "\r\nConnection: close\r\n\r\n")); + Long declaredLength = declaredLengthFor(path); + if (declaredLength == null) { + response.write(bytes("HTTP/1.1 200 OK\r\nContent-Length: " + body.length + + "\r\nConnection: close\r\n\r\n")); + } else if (declaredLength == NO_CONTENT_LENGTH) { + response.write(bytes("HTTP/1.1 200 OK\r\nConnection: close\r\n\r\n")); + } else { + response.write(bytes("HTTP/1.1 200 OK\r\nContent-Length: " + declaredLength + + "\r\nConnection: close\r\n\r\n")); + } + response.write(body); } response.flush(); @@ -720,6 +858,10 @@ private void respond(Socket connection) throws IOException { private synchronized byte[] bodyFor(String path) { return mBodies.get(path); } + + private synchronized Long declaredLengthFor(String path) { + return mDeclaredLengths.get(path); + } } private static byte[] zipOf(Map contents) throws IOException { diff --git a/docs/diff-updates.ko.md b/docs/diff-updates.ko.md index 6feadd2f..20b74992 100644 --- a/docs/diff-updates.ko.md +++ b/docs/diff-updates.ko.md @@ -85,11 +85,13 @@ binary patch로 배포한 릴리스는 **asset diff 아카이브**도 함께 담 | 2 | binary patch | 항상 쓸 수 있습니다. 모든 클라이언트가 가진 앱 바이너리의 번들을 기준으로 만들기 때문입니다. | | 3 | full | 항상 쓸 수 있습니다. 설치된 업데이트가 없어도 됩니다. | -이 순서에는 예외가 두 가지 있습니다. +이 순서에는 예외가 세 가지 있습니다. **모든 클라이언트가 셋을 다 시도하지는 않습니다.** 쓸 수 있는 asset diff가 없는 클라이언트는 binary patch에서 시작합니다. 앱 바이너리의 번들을 실행 중이거나, 새 릴리스가 diff를 만들지 않은 업데이트를 실행 중인 경우입니다. -**asset diff가 실패해도 항상 binary patch로 넘어가지는 않습니다.** diff가 asset 영역에서 실패했을 때만 넘어갑니다. 설치된 업데이트와 병합하지 못했거나(`asset_merge_failed`), 병합된 내용이 package hash 검증에 실패한 경우(`package_verification_failed`)입니다. 그 밖의 실패는 두 아카이브가 공유하는 bundle patch에서 일어나므로 binary patch도 같은 방식으로 실패합니다. 이때 클라이언트는 곧바로 full 아카이브를 내려받습니다. +**asset diff가 실패해도 항상 binary patch로 넘어가지는 않습니다.** diff가 asset 영역에서 실패했다면 넘어갑니다. 설치된 업데이트와 병합하지 못했거나(`asset_merge_failed`), 병합된 내용이 package hash 검증에 실패한 경우(`package_verification_failed`)입니다. 서버가 diff의 URL에 400 이상으로 응답했을 때도 넘어갑니다. 두 아카이브는 서로 다른 URL에 있으니, diff를 받지 못했다고 해서 binary patch도 받지 못하리라 단정할 수 없습니다. 그 밖의 실패는 두 아카이브가 byte 단위로 똑같이 담고 있는 bundle patch에서 일어납니다. binary patch도 같은 방식으로 실패하므로 곧바로 full 아카이브를 내려받습니다. + +**연결이 실패하면 다운로드가 거기서 멈춥니다.** 다음 아카이브도 같은 네트워크 뒤에 있고 full 아카이브는 셋 중 가장 큽니다. 이어서 시도해 봐야 더 느리게 실패할 뿐이므로, 클라이언트는 연결 오류를 그대로 알립니다. 서버가 응답한 경우는 다릅니다. 한 아카이브의 404는 다음 아카이브를 건너뛸 이유가 되지 않습니다. `onUpdateArchiveResult`는 시도한 아카이브를 모두 보고합니다. [텔레메트리 콜백](telemetry-callbacks.ko.md#onupdatearchiveresult가-보고하는-내용)을 참고하세요. diff --git a/docs/diff-updates.md b/docs/diff-updates.md index aa4e6ed1..727d9b10 100644 --- a/docs/diff-updates.md +++ b/docs/diff-updates.md @@ -118,18 +118,25 @@ A client tries the archives in this order and stops at the first one it can inst | 2 | Binary patch | always. It is built against the bundle in the app binary, which every client has. | | 3 | Full | always. It needs nothing installed. | -There are two exceptions to this order. +There are three exceptions to this order. **Not every client tries all three.** One with no asset diff to use - it is running the bundle in the app binary, or an update this release was not diffed against - starts at the binary patch. -**A failed asset diff does not always reach the binary patch.** It moves on to that archive -only when the diff failed on its asset side: the merge with the installed update failing -(`asset_merge_failed`), or the merged contents failing the package hash -(`package_verification_failed`). Anything else the diff fails on lives in the bundle patch -both archives carry, so the binary patch would fail there the same way and the client goes -straight to the full archive. +**A failed asset diff does not always reach the binary patch.** It does when the diff failed +on its asset side: the merge with the installed update failing (`asset_merge_failed`), or the +merged contents failing the package hash (`package_verification_failed`). It also does when +the server answered the diff's URL with a status of 400 or above. The two archives are at +URLs of their own, so a diff that could not be fetched is no reason to expect the binary +patch cannot be either. Anything else the diff fails on lives in the bundle patch both +archives carry byte for byte. The binary patch would fail there the same way, so the client +goes straight to the full archive. + +**A failed connection stops the download.** The next archive is behind the same network, +and the full one is the largest of the three, so trying it would only fail again more +slowly. The client reports the connection error instead. A server that answered is +different: a 404 on one archive is no reason to skip the next. `onUpdateArchiveResult` reports every archive that was tried - see [Telemetry callbacks](telemetry-callbacks.md#what-onupdatearchiveresult-reports). diff --git a/docs/telemetry-callbacks.ko.md b/docs/telemetry-callbacks.ko.md index 63ebc968..cc8dadfe 100644 --- a/docs/telemetry-callbacks.ko.md +++ b/docs/telemetry-callbacks.ko.md @@ -23,6 +23,26 @@ > [!NOTE] > 타입 정의에서 `onRolloutSkipped`는 두 번째 `error` 매개변수를 선언하지만, 런타임은 항상 `label`만 전달합니다. +## `error`가 담고 있는 것 + +리포트는 메시지가 아니라 `error.code`로 묶으세요. 메시지는 iOS에서 현지화되고 Android에서는 예외 자체의 문구입니다. + +| 플랫폼 | `error.code` | 예 | +|---|---|---| +| iOS | [`NSURLError`](https://developer.apple.com/documentation/foundation/1508628-url_loading_system_error_codes) 코드를 문자열로, CodePush가 직접 던진 오류는 `-1` | `"-1005"`, 연결이 끊김 | +| Android | 실패의 카테고리 | `"CODE_PUSH_NETWORK"`, 연결이 끊김 | + +Android 카테고리는 다음과 같습니다. + +| Code | 뜻 | 다시 받아 볼 가치 | +|---|---|---| +| `CODE_PUSH_NETWORK` | 연결이 끊겼거나, 시간이 초과됐거나, 열리지 않았습니다. | 네트워크가 돌아오면 있음 | +| `CODE_PUSH_HTTP` | 서버가 400 이상으로 응답했습니다. 상태 코드는 메시지에 있습니다. | 상태 코드에 따라 다름 | +| `CODE_PUSH_INTEGRITY` | 다운로드한 내용의 hash가 릴리스의 package hash와 다르거나, 그 안에 앱이 찾는 이름의 JS 번들이 없습니다. | 없음 | +| `CODE_PUSH_UNKNOWN` | 그 밖의 경우입니다. | 알 수 없음 | + +`CodePush.sync()`도 같은 오류로 거절되므로, 반환값을 기다리는 호출자에게는 이 콜백이 필요 없습니다. + ## 등록 [앱에 CodePush 적용하기](../README.md#4-codepush-ify-your-app)의 `CodePush({ ... })` 래퍼에 콜백을 전달하면, `checkFrequency` 값과 무관하게 직접 호출한 `CodePush.sync()`를 포함한 모든 sync에서 실행됩니다. diff --git a/docs/telemetry-callbacks.md b/docs/telemetry-callbacks.md index 88584000..ba58a36a 100644 --- a/docs/telemetry-callbacks.md +++ b/docs/telemetry-callbacks.md @@ -30,6 +30,28 @@ the sync failed before a release was resolved. > The typings declare a second `error` parameter on `onRolloutSkipped`, but the runtime only > ever passes the label. +## What `error` carries + +Group reports by `error.code`, not by the message: the message is localized on iOS and is +the exception's own words on Android. + +| Platform | `error.code` | Example | +|---|---|---| +| iOS | the [`NSURLError`](https://developer.apple.com/documentation/foundation/1508628-url_loading_system_error_codes) code as a string, or `-1` for an error CodePush raised itself | `"-1005"`, the connection dropped | +| Android | the category of the failure | `"CODE_PUSH_NETWORK"`, the connection dropped | + +Android categories: + +| Code | Means | Worth downloading again | +|---|---|---| +| `CODE_PUSH_NETWORK` | The connection dropped, timed out, or never opened. | Once the network is back | +| `CODE_PUSH_HTTP` | The server answered with a status of 400 or above. The status is in the message. | Depends on the status | +| `CODE_PUSH_INTEGRITY` | The downloaded contents do not hash to the release's package hash, or hold no JS bundle by the name the app looks for. | No | +| `CODE_PUSH_UNKNOWN` | Anything else. | Unknown | + +`CodePush.sync()` rejects with the same error, so a caller that awaits it does not need +this callback. + ## Registering them Pass them to the `CodePush({ ... })` wrapper from diff --git a/e2e/helpers/asset-diff-fixtures.ts b/e2e/helpers/asset-diff-fixtures.ts index eec3d36e..e0c66c57 100644 --- a/e2e/helpers/asset-diff-fixtures.ts +++ b/e2e/helpers/asset-diff-fixtures.ts @@ -168,3 +168,20 @@ export function dropDiffArchiveManifestDeletions(archivePath: string): void { fs.writeFileSync(manifestPath, JSON.stringify(manifest)); }); } + +/** + * Deletes the published diff archive, leaving the release that offers it untouched. + * + * The release history still names the diff's URL, so the client asks for it and the + * server answers 404 - the shape of a release whose diff has been cleaned up while the + * archives at the other URLs are still there. Nothing is corrupted and nothing is + * applied: what this isolates is a client deciding what a status answered at one URL says + * about the archives at the others. + */ +export function removeDiffArchive(archivePath: string): void { + if (!fs.existsSync(archivePath)) { + throw new Error(`There is no diff archive at "${archivePath}" to remove`); + } + + fs.rmSync(archivePath); +} diff --git a/e2e/helpers/asset-diff-phase.ts b/e2e/helpers/asset-diff-phase.ts index 0ee25da2..30220440 100644 --- a/e2e/helpers/asset-diff-phase.ts +++ b/e2e/helpers/asset-diff-phase.ts @@ -20,6 +20,7 @@ import { assertReleaseOffersDiff, corruptDiffArchiveAsset, dropDiffArchiveManifestDeletions, + removeDiffArchive, } from "./asset-diff-fixtures"; import { assertReleaseOffersPatch, @@ -192,6 +193,20 @@ export async function runAssetDiffPhase(context: AssetDiffPhaseContext): Promise expectedArchiveResult: "fallback:asset-diff:asset-diff=target_verification_failed", }); + // A diff the server does not serve is not a verdict on the archives it does. Diffs are + // published one per recent version and are the first thing a retention policy clears + // out, while the patch archive at its own URL stays - so a 404 on the diff must not cost + // the patch archive its try. This is the one failure before the bundle is restored that + // still reaches the patch archive. + await runDiffScenario({ + name: "diff the server does not serve falls back to the patch archive", + baseVersion: "1.4.9", + updateVersion: "1.4.10", + breakDiff: () => removeDiffArchive(findAssetDiffArchive(platform, releaseIdentifier)), + expectedDownloads: ["asset-diff", "binary-patch"], + expectedArchiveResult: "applied:binary-patch:asset-diff=no-verdict:binary-patch=applied", + }); + // A manifest that names no files to delete is not one with nothing to delete - the CLI // writes the key on every release, an empty list included. Merging past its absence would // keep the asset the update dropped and install contents the release never published, so diff --git a/ios/CodePush/CodePush.h b/ios/CodePush/CodePush.h index 3b0ba794..06a69ed1 100644 --- a/ios/CodePush/CodePush.h +++ b/ios/CodePush/CodePush.h @@ -157,7 +157,10 @@ failCallback:(void (^)(NSError *err))failCallback; @interface CodePushErrorUtils : NSObject + (NSError *)errorWithMessage:(NSString *)errorMessage; ++ (NSError *)errorWithMessage:(NSString *)errorMessage httpStatusCode:(NSInteger)statusCode; + (BOOL)isCodePushError:(NSError *)error; ++ (BOOL)isNetworkFailure:(NSError *)error; ++ (BOOL)isHttpStatusError:(NSError *)error; @end diff --git a/ios/CodePush/CodePush.mm b/ios/CodePush/CodePush.mm index ea74b58e..aa347bf7 100644 --- a/ios/CodePush/CodePush.mm +++ b/ios/CodePush/CodePush.mm @@ -772,7 +772,7 @@ -(void)loadBundleOnTick:(NSTimer *)timer { NSDictionary *newPackage = [CodePushPackage getPackage:mutableUpdatePackage[PackageHashKey] error:&err]; if (err) { - return reject([NSString stringWithFormat: @"%lu", (long)err.code], err.localizedDescription, err); + return reject([NSString stringWithFormat: @"%ld", (long)err.code], err.localizedDescription, err); } if (updateArchiveResult) { @@ -796,7 +796,7 @@ -(void)loadBundleOnTick:(NSTimer *)timer { // Stop observing frame updates if the download fails. _didUpdateProgress = NO; self.paused = YES; - reject([NSString stringWithFormat: @"%lu", (long)err.code], err.localizedDescription, err); + reject([NSString stringWithFormat: @"%ld", (long)err.code], err.localizedDescription, err); }]; } @@ -876,7 +876,7 @@ - (void)restartAppInternal:(BOOL)onlyIfUpdateIsPending NSMutableDictionary *package = [[CodePushPackage getCurrentPackage:&error] mutableCopy]; if (error) { - return reject([NSString stringWithFormat: @"%lu", (long)error.code], error.localizedDescription, error); + return reject([NSString stringWithFormat: @"%ld", (long)error.code], error.localizedDescription, error); } else if (package == nil) { // The app hasn't downloaded any CodePush updates yet, // so we simply return nil regardless if the user @@ -930,7 +930,7 @@ - (void)restartAppInternal:(BOOL)onlyIfUpdateIsPending error:&error]; if (error) { - reject([NSString stringWithFormat: @"%lu", (long)error.code], error.localizedDescription, error); + reject([NSString stringWithFormat: @"%ld", (long)error.code], error.localizedDescription, error); } else { [self savePendingUpdate:updatePackage[PackageHashKey] isLoading:NO]; diff --git a/ios/CodePush/CodePushDownloadHandler.m b/ios/CodePush/CodePushDownloadHandler.m index d52e9e2e..4173f8c1 100644 --- a/ios/CodePush/CodePushDownloadHandler.m +++ b/ios/CodePush/CodePushDownloadHandler.m @@ -54,7 +54,8 @@ - (void)connection:(NSURLConnection *)connection didReceiveResponse:(NSURLRespon if (statusCode >= 400) { [self.outputFileStream close]; [connection cancel]; - NSError *err = [CodePushErrorUtils errorWithMessage:[NSString stringWithFormat: @"Received %ld response from %@", (long)statusCode, self.downloadUrl]]; + NSError *err = [CodePushErrorUtils errorWithMessage:[NSString stringWithFormat: @"Received %ld response from %@", (long)statusCode, self.downloadUrl] + httpStatusCode:statusCode]; self.failCallback(err); return; } diff --git a/ios/CodePush/CodePushErrorUtils.m b/ios/CodePush/CodePushErrorUtils.m index 97dede4a..0bb37674 100644 --- a/ios/CodePush/CodePushErrorUtils.m +++ b/ios/CodePush/CodePushErrorUtils.m @@ -4,6 +4,13 @@ @implementation CodePushErrorUtils static NSString *const CodePushErrorDomain = @"CodePushError"; static const int CodePushErrorCode = -1; +/* + * The status a server answered a download with, on the error raised for it. + * + * Carried in the user info rather than in the error code, because the code is what JS reads + * an error by and every error of this domain has always been -1 there. + */ +static NSString *const CodePushHttpStatusCodeKey = @"CodePushHttpStatusCode"; + (NSError *)errorWithMessage:(NSString *)errorMessage { @@ -12,9 +19,57 @@ + (NSError *)errorWithMessage:(NSString *)errorMessage userInfo:@{ NSLocalizedDescriptionKey: NSLocalizedString(errorMessage, nil) }]; } ++ (NSError *)errorWithMessage:(NSString *)errorMessage httpStatusCode:(NSInteger)statusCode +{ + return [NSError errorWithDomain:CodePushErrorDomain + code:CodePushErrorCode + userInfo:@{ NSLocalizedDescriptionKey: NSLocalizedString(errorMessage, nil), + CodePushHttpStatusCodeKey: @(statusCode) }]; +} + + (BOOL)isCodePushError:(NSError *)err { return err != nil && [CodePushErrorDomain isEqualToString:err.domain]; } +/* + * Whether the download failed because the server answered it with a status rather than with + * a body to install. + */ ++ (BOOL)isHttpStatusError:(NSError *)err +{ + return [self isCodePushError:err] && err.userInfo[CodePushHttpStatusCodeKey] != nil; +} + +/* + * Whether the request failed because the network did not carry it. + * + * A server that answered is not this, however it answered: the connection worked, and + * asking a different URL over it is worth doing. A connection that never opened or that + * dropped is, and every URL behind it is equally out of reach. + * + * Named from the codes rather than the domain, because `NSURLErrorDomain` also covers a + * URL that was malformed or a scheme that is not supported - failures of the request + * rather than of the network under it. + */ ++ (BOOL)isNetworkFailure:(NSError *)err +{ + if (err == nil || ![NSURLErrorDomain isEqualToString:err.domain]) { + return NO; + } + + switch (err.code) { + case NSURLErrorTimedOut: + case NSURLErrorCannotFindHost: + case NSURLErrorCannotConnectToHost: + case NSURLErrorNetworkConnectionLost: + case NSURLErrorDNSLookupFailed: + case NSURLErrorNotConnectedToInternet: + case NSURLErrorSecureConnectionFailed: + return YES; + default: + return NO; + } +} + @end \ No newline at end of file diff --git a/ios/CodePush/CodePushPackage.m b/ios/CodePush/CodePushPackage.m index 83947dc9..bd37d581 100644 --- a/ios/CodePush/CodePushPackage.m +++ b/ios/CodePush/CodePushPackage.m @@ -116,15 +116,17 @@ + (void)downloadPackage:(NSDictionary *)updatePackage /* * Tries the first archive of the queue, and decides what a failure of it means for the * rest. A failure after the bundle was restored is on the asset side of the archive, so - * the next archive - which does not share it - is worth trying. A failure before that - * point is in the bundle patch every archive carries byte for byte, or is something no - * verdict exists for, and either way the remaining archives are passed over: they could - * only fail the same way, and trying them would put more doomed downloads in front of the - * full one. + * the next archive - which does not share it - is worth trying, and so is one the server + * never served, which is a verdict on one URL rather than on the archives at the others. + * A failure between those is in the bundle patch every archive carries byte for byte, or + * is something no verdict exists for, and either way the remaining archives are passed + * over: they could only fail the same way, and trying them would put more doomed downloads + * in front of the full one. * - * Every way the ladder can end, the update is installed - by an archive of the queue or - * by the full download behind it - so no failure here reaches the caller as an error, and - * the result retells the attempts one by one. + * Every verdict on an archive ends with the update installed - by an archive of the queue + * or by the full download behind it - so no verdict reaches the caller as an error, and the + * result retells the attempts one by one. A network that did not carry the archive is not a + * verdict on it: it is raised, because the full download is behind the same network. */ + (void)tryNextArchive:(NSArray *)archivesToTry attemptsSoFar:(NSMutableArray *)attempts @@ -143,6 +145,9 @@ + (void)tryNextArchive:(NSArray *)archivesToTry // Set once the applier has restored the bundle, which is also what tells a failure // that follows apart from one that came before. __block NSNumber *applyDurationMs = nil; + // Set when the server answered this archive's URL with a status instead of the archive, + // which is a verdict on that URL and not on the archives at the others. + __block BOOL archiveWasNotServed = NO; void (^giveUpAttempt)(NSString *failureReason) = ^(NSString *failureReason) { [self deleteBinaryPatchFolder]; @@ -151,7 +156,7 @@ + (void)tryNextArchive:(NSArray *)archivesToTry applyDurationMs:applyDurationMs attemptStartTime:attemptStartTime]]; - if (applyDurationMs != nil && [remainingArchives count] > 0) { + if ((applyDurationMs != nil || archiveWasNotServed) && [remainingArchives count] > 0) { [self tryNextArchive:remainingArchives attemptsSoFar:attempts firstAttemptStartTime:firstAttemptStartTime @@ -201,6 +206,19 @@ + (void)tryNextArchive:(NSArray *)archivesToTry firstAttemptStartTime:firstAttemptStartTime]); } failCallback:^(NSError *err) { + if ([CodePushErrorUtils isNetworkFailure:err]) { + // The network is what failed, not the archive, and the full + // archive is behind the same network - only larger, and + // started over from nothing. Falling back here would spend a + // second download to reach the failure already in hand. + CPLog(@"The %@ archive could not be downloaded (%@). Giving up on the download.", + archive, err.localizedDescription); + [self deleteBinaryPatchFolder]; + failCallback(err); + return; + } + + archiveWasNotServed = [CodePushErrorUtils isHttpStatusError:err]; CPLog(@"The %@ archive could not be applied (%@). Falling back.", archive, err.localizedDescription); // An error raised after the bundle was restored is the restored // update failing the checks every update passes before it is diff --git a/ios/CodePushTests/CodePushErrorUtilsTests.m b/ios/CodePushTests/CodePushErrorUtilsTests.m new file mode 100644 index 00000000..44ea03ba --- /dev/null +++ b/ios/CodePushTests/CodePushErrorUtilsTests.m @@ -0,0 +1,64 @@ +#import +#import "CodePush.h" + +@interface CodePushErrorUtilsTests : XCTestCase +@end + +@implementation CodePushErrorUtilsTests + +static NSError *urlError(NSInteger code) +{ + return [NSError errorWithDomain:NSURLErrorDomain code:code userInfo:nil]; +} + +- (void)testNamesAConnectionThatDroppedAsANetworkFailure +{ + XCTAssertTrue([CodePushErrorUtils isNetworkFailure:urlError(NSURLErrorNetworkConnectionLost)]); + XCTAssertTrue([CodePushErrorUtils isNetworkFailure:urlError(NSURLErrorTimedOut)]); +} + +- (void)testNamesAConnectionThatNeverOpenedAsANetworkFailure +{ + XCTAssertTrue([CodePushErrorUtils isNetworkFailure:urlError(NSURLErrorNotConnectedToInternet)]); + XCTAssertTrue([CodePushErrorUtils isNetworkFailure:urlError(NSURLErrorCannotConnectToHost)]); + XCTAssertTrue([CodePushErrorUtils isNetworkFailure:urlError(NSURLErrorDNSLookupFailed)]); + XCTAssertTrue([CodePushErrorUtils isNetworkFailure:urlError(NSURLErrorSecureConnectionFailed)]); +} + +- (void)testDoesNotNameAMalformedRequestAsANetworkFailure +{ + // The request never reached a network to fail on, so retrying it anywhere else is + // no more likely to work. + XCTAssertFalse([CodePushErrorUtils isNetworkFailure:urlError(NSURLErrorBadURL)]); + XCTAssertFalse([CodePushErrorUtils isNetworkFailure:urlError(NSURLErrorUnsupportedURL)]); +} + +- (void)testDoesNotNameAnErrorStatusAsANetworkFailure +{ + // What the download handler raises for a status of 400 or above: the connection + // worked, so the archives behind it are worth asking for. + NSError *error = [CodePushErrorUtils errorWithMessage:@"Received 503 response from https://cdn.example.test/full.zip"]; + + XCTAssertTrue([CodePushErrorUtils isCodePushError:error]); + XCTAssertFalse([CodePushErrorUtils isNetworkFailure:error]); +} + +- (void)testNamesAnErrorStatusApartFromTheOtherErrorsCodePushRaises +{ + NSError *status = [CodePushErrorUtils errorWithMessage:@"Received 404 response from https://cdn.example.test/diff.zip" + httpStatusCode:404]; + NSError *other = [CodePushErrorUtils errorWithMessage:@"Received empty response from https://cdn.example.test/diff.zip"]; + + XCTAssertTrue([CodePushErrorUtils isHttpStatusError:status]); + XCTAssertFalse([CodePushErrorUtils isHttpStatusError:other]); + XCTAssertFalse([CodePushErrorUtils isHttpStatusError:[NSError errorWithDomain:NSURLErrorDomain + code:NSURLErrorTimedOut + userInfo:nil]]); +} + +- (void)testDoesNotNameANilErrorAsANetworkFailure +{ + XCTAssertFalse([CodePushErrorUtils isNetworkFailure:nil]); +} + +@end diff --git a/ios/CodePushTests/CodePushPackageTests.m b/ios/CodePushTests/CodePushPackageTests.m index 2dca9ed3..d19521e5 100644 --- a/ios/CodePushTests/CodePushPackageTests.m +++ b/ios/CodePushTests/CodePushPackageTests.m @@ -632,10 +632,14 @@ - (void)testMergesAnAssetDiffWhoseManifestDeletesNothing { [self assertInstalledContentsOf:packageHash matchStaging:updateStaging]; } -- (void)testSkipsThePatchArchiveWhenTheAssetDiffCannotBeDownloaded { - // A diff that never arrived left no verdict at all: nothing says the patch archive is - // any better off, and the full download is the one that cannot fail - so a client is - // never walked through two doomed downloads on its way there. +- (void)testSkipsThePatchArchiveWhenTheAssetDiffUrlAnswersWithNoStatusAtAll { + // A URL that answers with nothing - no archive and no status to read it by - left no + // verdict of any kind, and the full download is the one that cannot fail, so a client is + // never walked through two doomed downloads on its way there. A server that answers a + // status is the other case: that is a verdict on one URL and the patch archive is at + // another, so it is tried. The archives here are served as files, which have no status + // to answer with, so that case is covered by the Android suite and by + // CodePushErrorUtilsTests rather than here. [self installPackageWithContents:[self stageInstalledArchiveContents]]; NSString *updateStaging = [self stageAssetDiffTargetContents]; NSString *packageHash = CPTestFolderHash(updateStaging);