From be2eddab6c4a329b08dfc7b825756b86966a5313 Mon Sep 17 00:00:00 2001 From: Eli Bishop Date: Mon, 25 Jun 2018 15:38:37 -0700 Subject: [PATCH 1/7] treat most 4xx errors as unrecoverable --- errors.js | 7 +++++++ event_processor.js | 10 ++++------ eventsource.js | 21 ++++++++++---------- messages.js | 10 +++++++++- polling.js | 5 +++-- test/event_processor-test.js | 37 ++++++++++++++++++++++++++++-------- 6 files changed, 62 insertions(+), 28 deletions(-) diff --git a/errors.js b/errors.js index 5901a86..775d872 100644 --- a/errors.js +++ b/errors.js @@ -17,3 +17,10 @@ exports.LDStreamingError = createCustomError('LaunchDarklyStreamingError'); exports.LDUnexpectedResponseError = createCustomError('LaunchDarklyUnexpectedResponseError'); exports.LDInvalidSDKKeyError = createCustomError('LaunchDarklyInvalidSDKKeyError'); exports.LDClientError = createCustomError('LaunchDarklyClientError'); + +exports.isHttpErrorRecoverable = function(status) { + if (status >= 400 && status < 500) { + return status == 408 == 429; + } + return true; +} diff --git a/event_processor.js b/event_processor.js index ccca40e..ab453b1 100644 --- a/event_processor.js +++ b/event_processor.js @@ -3,6 +3,7 @@ var request = require('request'); var EventSummarizer = require('./event_summarizer'); var UserFilter = require('./user_filter'); var errors = require('./errors'); +var messages = require('./messages'); var wrapPromiseCallback = require('./utils/wrapPromiseCallback'); function EventProcessor(sdkKey, config, errorReporter) { @@ -200,15 +201,12 @@ function EventProcessor(sdkKey, config, errorReporter) { } } if (resp.statusCode > 204) { - var err = new errors.LDUnexpectedResponseError("Unexpected status code " + resp.statusCode + "; events may not have been processed", - resp.statusCode); + var err = new errors.LDUnexpectedResponseError(messages.httpErrorMessage(resp.statusCode, 'event posting', 'some events were dropped')); errorReporter && errorReporter(err); - if (resp.statusCode === 401) { + if (!errors.isHttpErrorRecoverable(resp.statusCode)) { reject(err); - var err1 = new errors.LDInvalidSDKKeyError("Received 401 error, no further events will be posted since SDK key is invalid"); - errorReporter && errorReporter(err1); shutdown = true; - } else if (resp.statusCode >= 500) { + } else { retryOrReject(err); } } else { diff --git a/eventsource.js b/eventsource.js index 9b567bb..4cee815 100644 --- a/eventsource.js +++ b/eventsource.js @@ -4,7 +4,9 @@ var parse = require('url').parse , events = require('events') , https = require('https') , http = require('http') - , util = require('util'); + , util = require('util') + , errors = require('./errors') + , messages = require('./messages'); function isPlainObject(obj) { return Object.getPrototypeOf(obj) === Object.prototype; @@ -104,18 +106,15 @@ function EventSource(url, eventSourceInitDict) { } if (res.statusCode !== 200) { - // reconnect after an error, unless it's a 401 - if (res.statusCode === 401) { + // reconnect after an error, unless it's an unrecoverable error + _emit('error', new Event('error', { + message: messages.httpErrorMessage(res.statusCode, 'streaming connection', 'will retry'), + status: res.statusCode + })); + + if (!errors.isHttpErrorRecoverable(res.statusCode)) { readyState = EventSource.CLOSED; - _emit('error', new Event('error', { - message: 'Received 401 error, no further streaming connection will be made since SDK key is invalid', - status: 401 - })); } else { - _emit('error', new Event('error', { - message: 'Streaming connection returned status code: ' + res.statusCode, - status: res.statusCode - })); backoffDelay = Math.min(backoffDelay * 2, 15000); setTimeout(connect, backoffDelay); } diff --git a/messages.js b/messages.js index 95ae5e9..5b52836 100644 --- a/messages.js +++ b/messages.js @@ -1,4 +1,12 @@ +var errors = require('./errors'); exports.deprecated = function(oldName, newName) { return '"' + oldName + '" is deprecated, please use "' + newName + '"'; -} +}; + +exports.httpErrorMessage = function(status, context, retryMessage) { + return 'Received error ' + status + + (status == 401 ? ' (invalid SDK key)' : '') + + ' for ' + context + + ' - ' + (errors.isHttpErrorRecoverable(status) ? retryMessage : 'giving up permanently'); +}; diff --git a/polling.js b/polling.js index f58baf4..5eb9a3c 100644 --- a/polling.js +++ b/polling.js @@ -1,4 +1,5 @@ var errors = require('./errors'); +var messages = require('./messages'); var dataKind = require('./versioned_data_kind'); function PollingProcessor(config, requestor) { @@ -22,8 +23,8 @@ function PollingProcessor(config, requestor) { sleepFor = Math.max(config.pollInterval * 1000 - elapsed, 0); config.logger.debug("Elapsed: %d ms, sleeping for %d ms", elapsed, sleepFor); if (err) { - cb(new errors.LDPollingError('Failed to fetch all feature flags: ' + (err.message || JSON.stringify(err))), err.status); - if (err.status === 401) { + cb(new errors.LDPollingError(messages.httpErrorMessage(err.status, 'polling request', 'will retry'))); + if (!errors.isHttpErrorRecoverable(err.status)) { config.logger.error('Received 401 error, no further polling requests will be made since SDK key is invalid'); } else { // Recursively call poll after the appropriate delay diff --git a/test/event_processor-test.js b/test/event_processor-test.js index e42b4ea..acdfebc 100644 --- a/test/event_processor-test.js +++ b/test/event_processor-test.js @@ -25,6 +25,7 @@ describe('EventProcessor', function() { if (ep) { ep.close(); } + nock.cleanAll(); }); function flushAndGetRequest(options, cb) { @@ -394,13 +395,13 @@ describe('EventProcessor', function() { }); }); - it('stops sending events after a 401 error', function(done) { + function verifyUnrecoverableHttpError(done, status) { ep = EventProcessor(sdkKey, defaultConfig); var e = { kind: 'identify', creationDate: 1000, user: user }; ep.sendEvent(e); - flushAndGetRequest({ status: 401 }, function(body, headers, error) { - expect(error.message).toContain("status code 401"); + flushAndGetRequest({ status: status }, function(body, headers, error) { + expect(error.message).toContain("error " + status); ep.sendEvent(e); @@ -412,21 +413,41 @@ describe('EventProcessor', function() { done(); }); }); - }); + } - it('retries once after a 5xx error', function(done) { + function verifyRecoverableHttpError(done, status) { ep = EventProcessor(sdkKey, defaultConfig); var e = { kind: 'identify', creationDate: 1000, user: user }; ep.sendEvent(e); - nock(eventsUri).post('/bulk').reply(503); - nock(eventsUri).post('/bulk').reply(503); + nock(eventsUri).post('/bulk').reply(status); + nock(eventsUri).post('/bulk').reply(status); // since we only queued two responses, Nock will throw an error if it gets a third. ep.flush().then( function() {}, function(err) { - expect(err.message).toContain('Unexpected status code 503'); + expect(err.message).toContain('error ' + status); done(); }); + } + + it('stops sending events after a 401 error', function(done) { + verifyUnrecoverableHttpError(done, 401); + }); + + it('stops sending events after a 403 error', function(done) { + verifyUnrecoverableHttpError(done, 403); + }); + + it('retries after a 408 error', function(done) { + verifyRecoverableHttpError(done, 408); + }); + + it('retries after a 429 error', function(done) { + verifyRecoverableHttpError(done, 429); + }); + + it('retries after a 503 error', function(done) { + verifyRecoverableHttpError(done, 503); }); }); From e0b527a5e4bafdd0c78975f5e1a45530c67f183c Mon Sep 17 00:00:00 2001 From: Eli Bishop Date: Mon, 25 Jun 2018 15:43:58 -0700 Subject: [PATCH 2/7] typo --- errors.js | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/errors.js b/errors.js index 775d872..c31d7ab 100644 --- a/errors.js +++ b/errors.js @@ -20,7 +20,7 @@ exports.LDClientError = createCustomError('LaunchDarklyClientError'); exports.isHttpErrorRecoverable = function(status) { if (status >= 400 && status < 500) { - return status == 408 == 429; + return status == 408 || status == 429; } return true; } From 955590d4586c17aa38c4a5d9970e94a23c84b433 Mon Sep 17 00:00:00 2001 From: Eli Bishop Date: Mon, 25 Jun 2018 16:02:57 -0700 Subject: [PATCH 3/7] strict equality --- errors.js | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/errors.js b/errors.js index c31d7ab..74b7495 100644 --- a/errors.js +++ b/errors.js @@ -20,7 +20,7 @@ exports.LDClientError = createCustomError('LaunchDarklyClientError'); exports.isHttpErrorRecoverable = function(status) { if (status >= 400 && status < 500) { - return status == 408 || status == 429; + return status === 408 || status === 429; } return true; } From 188d29f44e269b889c2214322dd7654a35acab10 Mon Sep 17 00:00:00 2001 From: Eli Bishop Date: Mon, 25 Jun 2018 17:42:23 -0700 Subject: [PATCH 4/7] add new "failed" event and waitForInitialization() --- index.d.ts | 12 ++++++++++ index.js | 17 ++++++++++++++ test/LDClient-test.js | 53 ++++++++++++++++++++++++++++++++++++++++++- 3 files changed, 81 insertions(+), 1 deletion(-) diff --git a/index.d.ts b/index.d.ts index 994a963..a156748 100644 --- a/index.d.ts +++ b/index.d.ts @@ -376,10 +376,22 @@ declare module "ldclient-node" { initialized: () => boolean; /** + * Returns a Promise that will be resolved if and when the client is successfully initialized. + * If initialization fails, the Promise will not resolve, but will not be rejected either + * (unlike waitForInitialization). * @returns a Promise containing the initialization state of the client */ waitUntilReady: () => Promise; + /** + * Returns a Promise that will be resolved if the client successfully initializes, or + * rejected if client initialization has irrevocably failed (for instance, if it detects + * that the SDK key is invalid). The failure case can also be detected by listening for + * the event "failed". + * @returns a Promise containing the initialization state of the client + */ + waitForInitialization: () => Promise; + /** * Retrieves a flag's value. * diff --git a/index.js b/index.js index 59c22e0..097facc 100644 --- a/index.js +++ b/index.js @@ -52,6 +52,7 @@ function NullUpdateProcessor() { var newClient = function(sdkKey, config) { var client = new EventEmitter(), initComplete = false, + failure, queue = [], requestor, updateProcessor, @@ -109,6 +110,8 @@ var newClient = function(sdkKey, config) { } maybeReportError(error); + client.emit('failed', error); + failure = error; } else if (!initComplete) { initComplete = true; client.emit('ready'); @@ -129,6 +132,20 @@ var newClient = function(sdkKey, config) { }); }; + client.waitForInitialization = function() { + if (initComplete) { + return Promise.resolve(); + } + if (failure) { + return Promise.reject(failure); + } + + return new Promise(function(resolve, reject) { + client.once('ready', resolve); + client.once('failed', reject); + }); + }; + client.variation = function(key, user, defaultVal, callback) { return wrapPromiseCallback(new Promise(function(resolve, reject) { sanitizeUser(user); diff --git a/test/LDClient-test.js b/test/LDClient-test.js index 138d4e3..2e13c85 100644 --- a/test/LDClient-test.js +++ b/test/LDClient-test.js @@ -24,7 +24,7 @@ describe('LDClient', function() { var updateProcessor = { start: function(callback) { - setImmediate(callback, null); + setImmediate(callback, updateProcessor.error); } }; @@ -32,6 +32,7 @@ describe('LDClient', function() { logger.info = jest.fn(); logger.warn = jest.fn(); eventProcessor.events = []; + updateProcessor.error = null; }); it('should trigger the ready event in offline mode', function(done) { @@ -309,4 +310,54 @@ describe('LDClient', function() { }).catch(done.error) }); }); + + describe('waitForInitialization()', function () { + it('should resolve when ready', function(done) { + var callback = jest.fn(); + var client = createOnlineClientWithFlags({}); + + client.waitForInitialization().then(callback) + .then(() => { + expect(callback).toHaveBeenCalled(); + done(); + }).catch(done.error) + }); + + it('should resolve even if the client is already ready', function(done) { + var callback = jest.fn(); + var client = createOnlineClientWithFlags({}); + + client.waitForInitialization() + .then(() => { + client.waitForInitialization().then(callback) + .then(() => { + expect(callback).toHaveBeenCalled(); + done(); + }).catch(done.error) + }).catch(done.error) + }); + + it('should be rejected if initialization fails', function(done) { + updateProcessor.error = { status: 403 }; + var client = createOnlineClientWithFlags({}); + + client.waitForInitialization() + .catch(err => { + expect(err).toEqual(updateProcessor.error); + done(); + }); + }); + }); + + describe('failed event', function() { + it('should be fired if initialization fails', function(done) { + updateProcessor.error = { status: 403 }; + var client = createOnlineClientWithFlags({}); + + client.on('failed', err => { + expect(err).toEqual(updateProcessor.error); + done(); + }); + }); + }) }); From cb44853ddc2032824f20982955011d2d504d2345 Mon Sep 17 00:00:00 2001 From: Eli Bishop Date: Tue, 26 Jun 2018 12:42:36 -0700 Subject: [PATCH 5/7] mark waitUntilReady as deprecated --- index.d.ts | 8 ++++++-- index.js | 2 ++ 2 files changed, 8 insertions(+), 2 deletions(-) diff --git a/index.d.ts b/index.d.ts index a156748..e37f8e4 100644 --- a/index.d.ts +++ b/index.d.ts @@ -379,6 +379,10 @@ declare module "ldclient-node" { * Returns a Promise that will be resolved if and when the client is successfully initialized. * If initialization fails, the Promise will not resolve, but will not be rejected either * (unlike waitForInitialization). + * + * This method is deprecated and will be removed in a future release. Instead, use + * waitForInitialization(), which waits for either success or failure. + * * @returns a Promise containing the initialization state of the client */ waitUntilReady: () => Promise; @@ -386,8 +390,8 @@ declare module "ldclient-node" { /** * Returns a Promise that will be resolved if the client successfully initializes, or * rejected if client initialization has irrevocably failed (for instance, if it detects - * that the SDK key is invalid). The failure case can also be detected by listening for - * the event "failed". + * that the SDK key is invalid). The sucess and failure cases can also be detected by listening + * for the events "ready" and "failed". * @returns a Promise containing the initialization state of the client */ waitForInitialization: () => Promise; diff --git a/index.js b/index.js index b082c42..cc58ae1 100644 --- a/index.js +++ b/index.js @@ -124,6 +124,8 @@ var newClient = function(sdkKey, config) { }; client.waitUntilReady = function() { + config.logger.warn(messages.deprecated("waitUntilReady", "waitForInitialization")); + if (initComplete) { return Promise.resolve(); } From cb624e11ec11eb7aad9161a8fcb4708aeaac4d0a Mon Sep 17 00:00:00 2001 From: Eli Bishop Date: Tue, 26 Jun 2018 14:14:15 -0700 Subject: [PATCH 6/7] add Node version note to readme --- README.md | 5 +++++ 1 file changed, 5 insertions(+) diff --git a/README.md b/README.md index fbad381..ab8074e 100644 --- a/README.md +++ b/README.md @@ -3,6 +3,11 @@ LaunchDarkly SDK for Node.js [![Circle CI](https://circleci.com/gh/launchdarkly/node-client/tree/master.svg?style=svg)](https://circleci.com/gh/launchdarkly/node-client/tree/master) +Supported Node versions +----------------------- + +This version of the LaunchDarkly SDK has been tested with Node versions 6.14 and up. + Quick setup ----------- From 5b928e5ebc7ddc8f315de398b7985c18a1b75681 Mon Sep 17 00:00:00 2001 From: Eli Bishop Date: Tue, 26 Jun 2018 14:57:13 -0700 Subject: [PATCH 7/7] version 5.1.0 --- CHANGELOG.md | 14 ++++++++++++++ package.json | 2 +- 2 files changed, 15 insertions(+), 1 deletion(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 4a2463d..b0590a2 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -2,6 +2,20 @@ All notable changes to the LaunchDarkly Node.js SDK will be documented in this file. This project adheres to [Semantic Versioning](http://semver.org). +## [5.1.0] - 2018-06-26 + +### Added: +- The new event `"failed"` will fire if client initialization failed due to any of the unrecoverable errors described below. If you prefer to use Promises, there is a new method `waitForInitialization()`, which behaves exactly like `waitUntilReady()` except that its Promise will be rejected if the "failed" event fires. (For backward compatibility, the Promise returned by `waitUntilReady()` will never be rejected.) ([#96](https://github.com/launchdarkly/node-client/issues/96)) + +### Changed: +- The client now treats most HTTP 4xx errors as unrecoverable: that is, after receiving such an error, it will not make any more HTTP requests for the lifetime of the client instance, in effect taking the client offline. This is because such errors indicate either a configuration problem (invalid SDK key) or a bug, which is not likely to resolve without a restart or an upgrade. This does not apply if the error is 400, 408, 429, or any 5xx error. + +### Fixed: +- Fixed a bug that would cause a null reference error if you called `close()` on an offline client. (Thanks, [dylanlingelbach](https://github.com/launchdarkly/node-client/pull/100)!) + +### Deprecated: +- The `waitUntilReady()` method is now deprecated in favor of `waitForInitialization()` (see above). + ## [5.0.2] - 2018-06-15 ### Fixed: diff --git a/package.json b/package.json index 4afacf6..279ccaa 100644 --- a/package.json +++ b/package.json @@ -1,6 +1,6 @@ { "name": "ldclient-node", - "version": "5.0.2", + "version": "5.1.0", "description": "LaunchDarkly SDK for Node.js", "main": "index.js", "scripts": {