From 88e05928703f5a19970c0015191e6ece55719459 Mon Sep 17 00:00:00 2001 From: Philipp von Weitershausen Date: Mon, 31 Jan 2011 20:55:48 -0800 Subject: [PATCH] Bug 591102 - Correctly identify network errors. r=mconnor --- services/sync/modules/service.js | 107 +++++++++++------- .../test_service_sync_checkServerError.js | 54 ++++++++- .../tests/unit/test_service_verifyLogin.js | 18 +++ 3 files changed, 136 insertions(+), 43 deletions(-) diff --git a/services/sync/modules/service.js b/services/sync/modules/service.js index da7fe154960..259b300b380 100644 --- a/services/sync/modules/service.js +++ b/services/sync/modules/service.js @@ -694,18 +694,19 @@ WeaveSvc.prototype = { /** * Perform the info fetch as part of a login or key fetch. */ - _fetchInfo: function _fetchInfo(url, logout) { + _fetchInfo: function _fetchInfo(url) { let infoURL = url || this.infoURL; this._log.trace("In _fetchInfo: " + infoURL); - let info = new Resource(infoURL).get(); + let info; + try { + info = new Resource(infoURL).get(); + } catch (ex) { + this._checkServerError(ex); + throw ex; + } if (!info.success) { - if (info.status == 401) { - if (logout) { - this.logout(); - Status.login = LOGIN_FAILED_LOGIN_REJECTED; - } - } + this._checkServerError(info); throw "aborting sync, failed to get collections"; } return info; @@ -830,15 +831,6 @@ WeaveSvc.prototype = { verifyLogin: function verifyLogin() this._notify("verify-login", "", function() { - // Make sure we have a cluster to verify against - // this is a little weird, if we don't get a node we pretend - // to succeed, since that probably means we just don't have storage - if (this.clusterURL == "" && !this._setCluster()) { - Status.sync = NO_SYNC_NODE_FOUND; - Svc.Obs.notify("weave:service:sync:delayed"); - return true; - } - if (!this.username) { this._log.warn("No username in verifyLogin."); Status.login = LOGIN_FAILED_NO_USERNAME; @@ -846,21 +838,30 @@ WeaveSvc.prototype = { } // Unlock master password, or return. + // Attaching auth credentials to a request requires access to + // passwords, which means that Resource.get can throw MP-related + // exceptions! + // Try to fetch the passphrase first, while we still have control. try { - // Fetch collection info on every startup. - // Attaching auth credentials to a request requires access to - // passwords, which means that Resource.get can throw MP-related - // exceptions! - // Try to fetch the passphrase first, while we still have control. - try { - this.passphrase; - } catch (ex) { - this._log.debug("Fetching passphrase threw " + ex + - "; assuming master password locked."); - Status.login = MASTER_PASSWORD_LOCKED; - return false; + this.passphrase; + } catch (ex) { + this._log.debug("Fetching passphrase threw " + ex + + "; assuming master password locked."); + Status.login = MASTER_PASSWORD_LOCKED; + return false; + } + + try { + // Make sure we have a cluster to verify against + // this is a little weird, if we don't get a node we pretend + // to succeed, since that probably means we just don't have storage + if (this.clusterURL == "" && !this._setCluster()) { + Status.sync = NO_SYNC_NODE_FOUND; + Svc.Obs.notify("weave:service:sync:delayed"); + return true; } + // Fetch collection info on every startup. let test = new Resource(this.infoURL).get(); switch (test.status) { @@ -1706,7 +1707,7 @@ WeaveSvc.prototype = { } // Figure out what the last modified time is for each collection - let info = this._fetchInfo(infoURL, true); + let info = this._fetchInfo(infoURL); this.globalScore = 0; // Convert the response to an object and read out the modified times @@ -1974,18 +1975,46 @@ WeaveSvc.prototype = { }, /** - * Check to see if this is a failure - * + * Handle HTTP response results or exceptions and set the appropriate + * Status.* bits. */ _checkServerError: function WeaveSvc__checkServerError(resp) { - if (Utils.checkStatus(resp.status, null, [500, [502, 504]])) { - Status.enforceBackoff = true; - if (resp.status == 503 && resp.headers["retry-after"]) - Svc.Obs.notify("weave:service:backoff:interval", - parseInt(resp.headers["retry-after"], 10)); + switch (resp.status) { + case 400: + if (resp == RESPONSE_OVER_QUOTA) { + Status.sync = OVER_QUOTA; + } + break; + + case 401: + this.logout(); + Status.login = LOGIN_FAILED_LOGIN_REJECTED; + break; + + case 500: + case 502: + case 503: + case 504: + Status.enforceBackoff = true; + if (resp.status == 503 && resp.headers["retry-after"]) { + Svc.Obs.notify("weave:service:backoff:interval", + parseInt(resp.headers["retry-after"], 10)); + } + break; + } + + switch (resp.result) { + case Cr.NS_ERROR_UNKNOWN_HOST: + case Cr.NS_ERROR_CONNECTION_REFUSED: + case Cr.NS_ERROR_NET_TIMEOUT: + case Cr.NS_ERROR_NET_RESET: + case Cr.NS_ERROR_NET_INTERRUPT: + case Cr.NS_ERROR_PROXY_CONNECTION_REFUSED: + // The constant says it's about login, but in fact it just + // indicates general network error. + Status.sync = LOGIN_FAILED_NETWORK_ERROR; + break; } - if (resp.status == 400 && resp == RESPONSE_OVER_QUOTA) - Status.sync = OVER_QUOTA; }, /** * Return a value for a backoff interval. Maximum is eight hours, unless diff --git a/services/sync/tests/unit/test_service_sync_checkServerError.js b/services/sync/tests/unit/test_service_sync_checkServerError.js index dac408394cc..00a1d56aacd 100644 --- a/services/sync/tests/unit/test_service_sync_checkServerError.js +++ b/services/sync/tests/unit/test_service_sync_checkServerError.js @@ -69,8 +69,8 @@ function test_backoff500(next) { Engines.unregister("catapult"); Status.resetBackoff(); Service.startOver(); - server.stop(next); } + server.stop(next); } function test_backoff503(next) { @@ -102,8 +102,8 @@ function test_backoff503(next) { Engines.unregister("catapult"); Status.resetBackoff(); Service.startOver(); - server.stop(next); } + server.stop(next); } function test_overQuota(next) { @@ -128,8 +128,52 @@ function test_overQuota(next) { Engines.unregister("catapult"); Status.resetSync(); Service.startOver(); - server.stop(next); } + server.stop(next); +} + +function test_service_networkError(next) { + setUp(); + // Provoke connection refused. + Service.clusterURL = "http://localhost:12345/"; + try { + do_check_eq(Status.sync, SYNC_SUCCEEDED); + + Service._loggedIn = true; + Service.sync(); + + do_check_eq(Status.sync, LOGIN_FAILED_NETWORK_ERROR); + } finally { + Status.resetSync(); + Service.startOver(); + } + next(); +} + +function test_engine_networkError(next) { + _("Test: Network related exceptions from engine.sync() lead to the right status code."); + let server = sync_httpd_setup(); + setUp(); + + Engines.register(CatapultEngine); + let engine = Engines.get("catapult"); + engine.enabled = true; + engine.exception = Components.Exception("NS_ERROR_UNKNOWN_HOST", + Cr.NS_ERROR_UNKNOWN_HOST); + + try { + do_check_eq(Status.sync, SYNC_SUCCEEDED); + + Service.login(); + Service.sync(); + + do_check_eq(Status.sync, LOGIN_FAILED_NETWORK_ERROR); + } finally { + Engines.unregister("catapult"); + Status.resetSync(); + Service.startOver(); + } + server.stop(next); } // Slightly misplaced test as it doesn't actually test checkServerError, @@ -156,8 +200,8 @@ function test_engine_applyFailed(next) { Engines.unregister("catapult"); Status.resetSync(); Service.startOver(); - server.stop(next); } + server.stop(next); } function run_test() { @@ -168,6 +212,8 @@ function run_test() { asyncChainTests(test_backoff500, test_backoff503, test_overQuota, + test_service_networkError, + test_engine_networkError, test_engine_applyFailed, do_test_finished)(); } diff --git a/services/sync/tests/unit/test_service_verifyLogin.js b/services/sync/tests/unit/test_service_verifyLogin.js index 87d67b97911..7b12bb6724c 100644 --- a/services/sync/tests/unit/test_service_verifyLogin.js +++ b/services/sync/tests/unit/test_service_verifyLogin.js @@ -49,11 +49,13 @@ function run_test() { do_check_eq(Status.service, STATUS_OK); _("Credentials won't check out because we're not configured yet."); + Status.resetSync(); do_check_false(Service.verifyLogin()); do_check_eq(Status.service, CLIENT_NOT_CONFIGURED); do_check_eq(Status.login, LOGIN_FAILED_NO_USERNAME); _("Try again with username and password set."); + Status.resetSync(); Service.username = "johndoe"; Service.password = "ilovejane"; do_check_false(Service.verifyLogin()); @@ -64,12 +66,14 @@ function run_test() { do_check_eq(Service.clusterURL, "http://localhost:8080/api/"); _("Success if passphrase is set."); + Status.resetSync(); Service.passphrase = "foo"; do_check_true(Service.verifyLogin()); do_check_eq(Status.service, STATUS_OK); do_check_eq(Status.login, LOGIN_SUCCEEDED); _("If verifyLogin() encounters a server error, it flips on the backoff flag and notifies observers on a 503 with Retry-After."); + Status.resetSync(); Service.username = "janedoe"; do_check_false(Status.enforceBackoff); let backoffInterval; @@ -82,6 +86,20 @@ function run_test() { do_check_eq(Status.service, LOGIN_FAILED); do_check_eq(Status.login, LOGIN_FAILED_SERVER_ERROR); + _("Ensure a network error when finding the cluster sets the right Status bits."); + Status.resetSync(); + Service.serverURL = "http://localhost:12345/"; + do_check_false(Service.verifyLogin()); + do_check_eq(Status.service, LOGIN_FAILED); + do_check_eq(Status.login, LOGIN_FAILED_NETWORK_ERROR); + + _("Ensure a network error when getting the collection info sets the right Status bits."); + Status.resetSync(); + Service.clusterURL = "http://localhost:12345/"; + do_check_false(Service.verifyLogin()); + do_check_eq(Status.service, LOGIN_FAILED); + do_check_eq(Status.login, LOGIN_FAILED_NETWORK_ERROR); + } finally { Svc.Prefs.resetBranch(""); server.stop(do_test_finished);