From c436c84b97ff63c0a407bf855269db0f36eb63ec Mon Sep 17 00:00:00 2001 From: djones Date: Mon, 2 Oct 2017 09:11:39 +0200 Subject: [PATCH 1/9] adding retry logic --- README.md | 31 +++++++++++++++++++++++++++ lib/client.js | 52 +++++++++++++++++++++++++++++++++++++++++++++ package-lock.json | 23 ++++++++++++-------- package.json | 3 ++- test/client.test.js | 28 ++++++++++++------------ 5 files changed, 113 insertions(+), 24 deletions(-) diff --git a/README.md b/README.md index b50f149..51dbb95 100644 --- a/README.md +++ b/README.md @@ -109,6 +109,37 @@ const catWatch = new ServiceClient({ }); ``` +## Fault Tolerant Requests + +Depending on the nature of the data required, it can be a good idea to perform a retry in the instance of a failed request. + +The ServiceClient also has the ability to make a `faultTolerantRequest()` which will preform retries when the request fails, based on the retryOptions configuration. + +Internally `perron` uses [retry](https://github.com/tim-kos/node-retry) in order to perform retries with exponential backoff. + +```js +const ServiceClient = require('perron'); + +const catWatch = new ServiceClient({ + hostname: 'catwatch.opensource.zalan.do', + // These are the default settings + retryOptions: { + retries: 3, + factor: 2, + minTimeout: 500, + maxTimeout: 1000, + randomize: true + } +}); + +catWatch.faultTolerantRequest({ + pathname: '/projects', + query: { + limit: 10 + } +}).then(data => console.log(data)); +``` + ## Filters It's quite often necessary to do some pre- or post-processing of the request. For this purpose `perron` implements a concept of filters, that are just an object with 2 optional methods: `request` and `response`. diff --git a/lib/client.js b/lib/client.js index 31537a4..026a30b 100644 --- a/lib/client.js +++ b/lib/client.js @@ -3,6 +3,7 @@ const request = require('./request'); const url = require('url'); const CircuitBreaker = require('circuit-breaker-js'); +const retry = require('retry'); /** * @typedef {Object.)>} ServiceClientHeaders @@ -216,6 +217,18 @@ class ServiceClient { timeout: DEFAULT_REQUEST_TIMEOUT }, this.options.defaultRequestOptions); + this.options.retryOptions = Object.assign({ + retries: 1, + factor: 2, + minTimeout: 200, + maxTimeout: 200, + randomize: false + }, this.options.retryOptions); + + if (this.options.retryOptions.minTimeout > this.options.retryOptions.maxTimeout) { + throw new Error('The `retryInterval` must be equal to or greater than the `minTimeout`'); + } + if (this.options.circuitBreaker) { const breakerOptions = Object.assign({ windowDuration: 10000, @@ -261,6 +274,45 @@ class ServiceClient { } )); } + + faultTolerantRequest(userParams) { + const params = Object.assign({}, this.options.defaultRequestOptions, userParams); + + params.hostname = this.options.hostname; + params.port = params.port || (params.protocol === 'https:' ? 443 : 80); + + params.headers = Object.assign({ + accept: 'application/json' + }, params.headers); + + return new Promise((resolve, reject) => this.breaker.run( + (success, failure) => { + const opts = { + retries: this.options.retryOptions.retries, + factor: this.options.retryOptions.factor, + minTimeout: this.options.retryOptions.minTimeout, + maxTimeout: this.options.retryOptions.maxTimeout, + randomize: this.options.retryOptions.randomize + }; + const operation = retry.operation(opts); + operation.attempt(() => { + return requestWithFilters(params, this.options.filters) + .then(result => { + success(); resolve(result); + }) + .catch(error => { + if (!operation.retry(error)) { + failure(); + reject(error); + } + }); + }); + }, + () => { + reject(new ServiceClient.Error({}, ServiceClient.CIRCUIT_OPEN)); + } + )); + } } /** diff --git a/package-lock.json b/package-lock.json index 6b9bb29..db92cf5 100644 --- a/package-lock.json +++ b/package-lock.json @@ -1821,6 +1821,11 @@ "signal-exit": "3.0.2" } }, + "retry": { + "version": "0.10.1", + "resolved": "https://registry.npmjs.org/retry/-/retry-0.10.1.tgz", + "integrity": "sha1-52OI0heZLCUnUCQdPTlW/tmNj/Q=" + }, "rimraf": { "version": "2.6.2", "resolved": "https://registry.npmjs.org/rimraf/-/rimraf-2.6.2.tgz", @@ -1971,15 +1976,6 @@ "integrity": "sha512-DBp0lSvX5G9KGRDTkR/R+a29H+Wk2xItOF+MpZLLNDWbEV9tGPnqLPxHEYjmiz8xGtJHRIqmI+hCjmNzqoA4nQ==", "dev": true }, - "string_decoder": { - "version": "1.0.3", - "resolved": "https://registry.npmjs.org/string_decoder/-/string_decoder-1.0.3.tgz", - "integrity": "sha512-4AH6Z5fzNNBcH+6XDMfA/BTt87skxqJlO0lAh3Dker5zThcAxG6mKz+iGu308UKoPPQ8Dcqx/4JhujzltRa+hQ==", - "dev": true, - "requires": { - "safe-buffer": "5.1.1" - } - }, "string-width": { "version": "2.1.1", "resolved": "https://registry.npmjs.org/string-width/-/string-width-2.1.1.tgz", @@ -1990,6 +1986,15 @@ "strip-ansi": "4.0.0" } }, + "string_decoder": { + "version": "1.0.3", + "resolved": "https://registry.npmjs.org/string_decoder/-/string_decoder-1.0.3.tgz", + "integrity": "sha512-4AH6Z5fzNNBcH+6XDMfA/BTt87skxqJlO0lAh3Dker5zThcAxG6mKz+iGu308UKoPPQ8Dcqx/4JhujzltRa+hQ==", + "dev": true, + "requires": { + "safe-buffer": "5.1.1" + } + }, "strip-ansi": { "version": "4.0.0", "resolved": "https://registry.npmjs.org/strip-ansi/-/strip-ansi-4.0.0.tgz", diff --git a/package.json b/package.json index 922a6af..74797f7 100644 --- a/package.json +++ b/package.json @@ -25,6 +25,7 @@ "sinon": "3.3.0" }, "dependencies": { - "circuit-breaker-js": "0.0.1" + "circuit-breaker-js": "0.0.1", + "retry": "0.10.1" } } diff --git a/test/client.test.js b/test/client.test.js index b8b8ab3..8ea86be 100644 --- a/test/client.test.js +++ b/test/client.test.js @@ -73,7 +73,7 @@ describe('ServiceClient', () => { headers: {}, body: JSON.stringify(originalBody) })); - return client.request().then(({ body }) => { + return client.faultTolerantRequest().then(({ body }) => { assert.deepStrictEqual(body, originalBody); }); }); @@ -84,7 +84,7 @@ describe('ServiceClient', () => { headers: {}, body: '' })); - return client.request().then(({ body }) => { + return client.faultTolerantRequest().then(({ body }) => { assert.equal(body, ''); }); }); @@ -96,7 +96,7 @@ describe('ServiceClient', () => { body: '' }; requestStub.returns(Promise.resolve(response)); - client.request().catch((err) => { + client.faultTolerantRequest().catch((err) => { assert(err instanceof ServiceClient.Error); assert.equal(err.type, ServiceClient.BODY_PARSE_FAILED); assert.deepStrictEqual(err.response, response); @@ -111,7 +111,7 @@ describe('ServiceClient', () => { body: '/not a JSON' }; requestStub.returns(Promise.resolve(response)); - client.request().catch(err => { + client.faultTolerantRequest().catch(err => { assert(err instanceof ServiceClient.Error); assert.equal(err.type, ServiceClient.BODY_PARSE_FAILED); assert.deepStrictEqual(err.response, response); @@ -123,7 +123,7 @@ describe('ServiceClient', () => { const client = new ServiceClient(clientOptions); const requestError = new Error('foobar'); requestStub.returns(Promise.reject(requestError)); - client.request().catch(err => { + client.faultTolerantRequest().catch(err => { assert(err instanceof ServiceClient.Error); assert.equal(err.type, ServiceClient.REQUEST_FAILED); done(); @@ -137,7 +137,7 @@ describe('ServiceClient', () => { } }]; const client = new ServiceClient(clientOptions); - client.request().catch(err => { + client.faultTolerantRequest().catch(err => { assert(err instanceof ServiceClient.Error); assert.equal(err.type, 'Request filter marked request as failed'); done(); @@ -151,7 +151,7 @@ describe('ServiceClient', () => { headers: {}, body: '{}' })); - client.request().catch(err => { + client.faultTolerantRequest().catch(err => { assert(err instanceof ServiceClient.Error); assert.equal(err.type, 'Response filter marked request as failed'); done(); @@ -169,7 +169,7 @@ describe('ServiceClient', () => { headers: {}, body: '{}' })); - client.request().catch(err => { + client.faultTolerantRequest().catch(err => { assert(err instanceof ServiceClient.Error); assert.equal(err.type, ServiceClient.RESPONSE_FILTER_FAILED); done(); @@ -191,7 +191,7 @@ describe('ServiceClient', () => { headers: {}, body: '{ "error": "non-REST-error" }' })); - client.request().catch(err => { + client.faultTolerantRequest().catch(err => { assert(err instanceof ServiceClient.Error); assert.equal(err.type, ServiceClient.RESPONSE_FILTER_FAILED); assert(err.message.includes('non-REST-error')); @@ -212,7 +212,7 @@ describe('ServiceClient', () => { body: '{ "error": "non-REST-error" }' }; requestStub.returns(Promise.resolve(response)); - client.request().catch(err => { + client.faultTolerantRequest().catch(err => { assert.deepStrictEqual(err.response, response); done(); }); @@ -226,7 +226,7 @@ describe('ServiceClient', () => { } }]; const client = new ServiceClient(clientOptions); - client.request().then(() => { + client.faultTolerantRequest().then(() => { assert.equal( requestStub.firstCall.args[0].path, 'foo-bar-buzz' @@ -248,7 +248,7 @@ describe('ServiceClient', () => { } }]; const client = new ServiceClient(clientOptions); - client.request().then((response) => { + client.faultTolerantRequest().then((response) => { assert.deepStrictEqual(response.headers, headers); assert.deepStrictEqual(response.body, body); done(); @@ -273,11 +273,11 @@ describe('ServiceClient', () => { const client = new ServiceClient(clientOptions); requests.reduce((promise) => { const tick = () => { - return client.request(); + return client.faultTolerantRequest(); }; return promise.then(tick, tick); }, Promise.resolve()).then(() => { - return client.request().catch((err) => { + return client.faultTolerantRequest().catch((err) => { assert(err instanceof ServiceClient.Error); assert(err.type, ServiceClient.CIRCUIT_OPEN); done(); From 56606fa87b726f8608b96b778bbdf18635af85d2 Mon Sep 17 00:00:00 2001 From: Joneser Date: Wed, 4 Oct 2017 08:03:54 +0200 Subject: [PATCH 2/9] test updates --- test/client.test.js | 411 +++++++++++++++++++++++++++++++++++++++++++- 1 file changed, 402 insertions(+), 9 deletions(-) diff --git a/test/client.test.js b/test/client.test.js index 8ea86be..4d1687c 100644 --- a/test/client.test.js +++ b/test/client.test.js @@ -66,6 +66,401 @@ describe('ServiceClient', () => { }); }); + it('should automatically parse response as JSON if content type is not set', () => { + const client = new ServiceClient(clientOptions); + const originalBody = { foo: 'bar' }; + requestStub.returns(Promise.resolve({ + headers: {}, + body: JSON.stringify(originalBody) + })); + return client.request().then(({ body }) => { + assert.deepStrictEqual(body, originalBody); + }); + }); + + it('should not throw an error if body or content-type is not set', () => { + const client = new ServiceClient(clientOptions); + requestStub.returns(Promise.resolve({ + headers: {}, + body: '' + })); + return client.request().then(({ body }) => { + assert.equal(body, ''); + }); + }); + + it('should throw an error if body is not set for application/json content type', (done) => { + const client = new ServiceClient(clientOptions); + const response = { + headers: { 'content-type': 'application/json' }, + body: '' + }; + requestStub.returns(Promise.resolve(response)); + client.request().catch((err) => { + assert(err instanceof ServiceClient.Error); + assert.equal(err.type, ServiceClient.BODY_PARSE_FAILED); + assert.deepStrictEqual(err.response, response); + done(); + }); + }); + + it('should give a custom error object when the parsing of the body fails', (done) => { + const client = new ServiceClient(clientOptions); + const response = { + headers: {}, + body: '/not a JSON' + }; + requestStub.returns(Promise.resolve(response)); + client.request().catch(err => { + assert(err instanceof ServiceClient.Error); + assert.equal(err.type, ServiceClient.BODY_PARSE_FAILED); + assert.deepStrictEqual(err.response, response); + done(); + }); + }); + + it('should give a custom error object when request fails', (done) => { + const client = new ServiceClient(clientOptions); + const requestError = new Error('foobar'); + requestStub.returns(Promise.reject(requestError)); + client.request().catch(err => { + assert(err instanceof ServiceClient.Error); + assert.equal(err.type, ServiceClient.REQUEST_FAILED); + done(); + }); + }); + + it('should allow to mark request as failed in the request filter', (done) => { + clientOptions.filters = [{ + request() { + throw new Error('Failed!'); + } + }]; + const client = new ServiceClient(clientOptions); + client.request().catch(err => { + assert(err instanceof ServiceClient.Error); + assert.equal(err.type, 'Request filter marked request as failed'); + done(); + }); + }); + + it('should by default handle 5xx code in a response-filter', (done) => { + const client = new ServiceClient(clientOptions); + requestStub.returns(Promise.resolve({ + statusCode: 501, + headers: {}, + body: '{}' + })); + client.request().catch(err => { + assert(err instanceof ServiceClient.Error); + assert.equal(err.type, 'Response filter marked request as failed'); + done(); + }); + }); + + it('should be able to handle 4xx code as a response-filter', (done) => { + clientOptions.filters = [ + ServiceClient.treat4xxAsError, + ServiceClient.treat5xxAsError + ]; + const client = new ServiceClient(clientOptions); + requestStub.returns(Promise.resolve({ + statusCode: 403, + headers: {}, + body: '{}' + })); + client.request().catch(err => { + assert(err instanceof ServiceClient.Error); + assert.equal(err.type, ServiceClient.RESPONSE_FILTER_FAILED); + done(); + }); + }); + + it('should be possible to define your own response-filters', (done) => { + clientOptions.filters = [{ + response(response) { + if (response.body.error) { + throw new Error(response.body.error); + } + return response; + } + }]; + const client = new ServiceClient(clientOptions); + requestStub.returns(Promise.resolve({ + statusCode: 200, + headers: {}, + body: '{ "error": "non-REST-error" }' + })); + client.request().catch(err => { + assert(err instanceof ServiceClient.Error); + assert.equal(err.type, ServiceClient.RESPONSE_FILTER_FAILED); + assert(err.message.includes('non-REST-error')); + done(); + }); + }); + + it('should have the original response in a response filter error', (done) => { + clientOptions.filters = [{ + response() { + throw new Error(); + } + }]; + const client = new ServiceClient(clientOptions); + const response = { + statusCode: 200, + headers: {}, + body: '{ "error": "non-REST-error" }' + }; + requestStub.returns(Promise.resolve(response)); + client.request().catch(err => { + assert.deepStrictEqual(err.response, response); + done(); + }); + }); + + it('should allow to specify request-filters to augment the request', (done) => { + clientOptions.filters = [{ + request(request) { + request.path = 'foo-bar-buzz'; + return request; + } + }]; + const client = new ServiceClient(clientOptions); + client.request().then(() => { + assert.equal( + requestStub.firstCall.args[0].path, + 'foo-bar-buzz' + ); + done(); + }); + }); + + it('should allow to specify a request-filters to short-circuit a response', (done) => { + const headers = { + 'x-my-custom-header': 'foobar' + }; + const body = { + foo: 'bar' + }; + clientOptions.filters = [{ + request() { + return new ServiceClient.Response(404, headers, body); + } + }]; + const client = new ServiceClient(clientOptions); + client.request().then((response) => { + assert.deepStrictEqual(response.headers, headers); + assert.deepStrictEqual(response.body, body); + done(); + }); + }); + + it('should open the circuit after 50% from 11 requests failed', (done) => { + const httpErrorResponse = Promise.resolve({ + statusCode: 500, + headers: {}, + body: '{}' + }); + const errorResponse = Promise.resolve(Promise.reject(new Error('timeout'))); + const requests = Array.from({ length: 11 }); + + [ emptySuccessResponse, emptySuccessResponse, httpErrorResponse, emptySuccessResponse, errorResponse, errorResponse, + httpErrorResponse, emptySuccessResponse, httpErrorResponse, errorResponse, emptySuccessResponse + ].forEach((response, index) => { + requestStub.onCall(index).returns(response); + }); + + const client = new ServiceClient(clientOptions); + requests.reduce((promise) => { + const tick = () => { + return client.request(); + }; + return promise.then(tick, tick); + }, Promise.resolve()).then(() => { + return client.request().catch((err) => { + assert(err instanceof ServiceClient.Error); + assert(err.type, ServiceClient.CIRCUIT_OPEN); + done(); + }); + }); + }); + + describe('built-in filter', () => { + it('should return original response if all ok', () => { + [ServiceClient.treat4xxAsError, ServiceClient.treat5xxAsError].forEach(filter => { + const response = { statusCode: 200 }; + assert.deepStrictEqual(filter.response(response), response); + }); + }); + }); + + describe('request params', () => { + const expectedDefaultRequestOptions = { + hostname: 'catwatch.opensource.zalan.do', + protocol: 'https:', + port: 443, + headers: { + accept: 'application/json' + }, + pathname: '/', + timeout: 2000 + }; + it('should pass reasonable request params by default', (done) => { + const client = new ServiceClient(clientOptions); + client.request().then(() => { + assert.deepStrictEqual(requestStub.firstCall.args[0], expectedDefaultRequestOptions); + done(); + }); + }); + it('should allow to pass additional params to the request', (done) => { + const client = new ServiceClient(clientOptions); + client.request({ foo: 'bar' }).then(() => { + assert.deepStrictEqual( + requestStub.firstCall.args[0], + Object.assign({ foo: 'bar' }, expectedDefaultRequestOptions) + ); + done(); + }); + }); + it('should allow to override params of the request', (done) => { + const client = new ServiceClient(clientOptions); + client.request({ pathname: '/foo' }).then(() => { + assert.deepStrictEqual( + requestStub.firstCall.args[0], + Object.assign({}, expectedDefaultRequestOptions, { pathname: '/foo' }) + ); + done(); + }); + }); + it('should allow to specify query params of the request', (done) => { + const client = new ServiceClient(clientOptions); + client.request({ + pathname: '/foo', + query: { param: 1 } + }).then(() => { + assert.deepStrictEqual( + requestStub.firstCall.args[0], + Object.assign({}, expectedDefaultRequestOptions, { + pathname: '/foo', + query: { param: 1 } + }) + ); + done(); + }); + }); + it('should allow to specify default params of the request', (done) => { + const userDefaultRequestOptions = { + pathname: '/foo', + protocol: 'http:', + query: { param: 42 } + }; + const client = new ServiceClient(Object.assign({}, clientOptions, { + defaultRequestOptions: userDefaultRequestOptions + })); + client.request().then(() => { + assert.deepStrictEqual( + requestStub.firstCall.args[0], + Object.assign({}, expectedDefaultRequestOptions, userDefaultRequestOptions, { port: 80 }) + ); + done(); + }); + }); + it('should not allow to override hostname', (done) => { + const client = new ServiceClient(Object.assign({}, clientOptions, { + defaultRequestOptions: { hostname: 'zalando.de' } + })); + client.request().then(() => { + assert.deepStrictEqual( + requestStub.firstCall.args[0], + Object.assign({}, expectedDefaultRequestOptions) + ); + done(); + }); + }); + it('should support taking hostname and default params from a URL instead of an object', (done) => { + const client = new ServiceClient('http://localhost:9999/foo?param=42'); + client.request().then(() => { + assert.deepEqual( + requestStub.firstCall.args[0], + Object.assign({}, expectedDefaultRequestOptions, { + port: 9999, + hostname: 'localhost', + pathname: '/foo', + protocol: 'http:', + query: { + param: '42' + } + }) + ); + done(); + }); + }); + }); +}); + + +describe('ServiceClient with faultTolerantRequest', () => { + let clientOptions; + + const requestStub = sinon.stub(); + const emptySuccessResponse = Promise.resolve({ + statusCode: 200, + headers: {}, + body: '{}' + }); + + const ServiceClient = proxyquire('../lib/client', { + './request': requestStub + }); + + beforeEach(() => { + clientOptions = { + hostname: 'catwatch.opensource.zalan.do' + }; + requestStub.reset(); + requestStub.returns(emptySuccessResponse); + }); + + it('should throw if the service is not provided', () => { + assert.throws(() => { + new ServiceClient({}); // eslint-disable-line no-new + }); + }); + + it('should by default send an `accept` application/json header', () => { + const client = new ServiceClient(clientOptions); + return client.faultTolerantRequest().then(() => { + assert.equal( + requestStub.firstCall.args[0].headers.accept, + 'application/json' + ); + }); + }); + + it('should not add authorization header if there is no token provider', () => { + const client = new ServiceClient(clientOptions); + return client.faultTolerantRequest().then(() => { + assert.strictEqual( + requestStub.firstCall.args[0].headers.authorization, + undefined + ); + }); + }); + + it('should automatically parse response as JSON if content type is set correctly', () => { + const client = new ServiceClient(clientOptions); + const originalBody = { foo: 'bar' }; + requestStub.returns({ + headers: { + 'content-type': 'application/json+something' + }, + body: JSON.stringify(originalBody) + }); + return client.faultTolerantRequest().then(({ body }) => { + assert.deepStrictEqual(body, originalBody); + }); + }); + it('should automatically parse response as JSON if content type is not set', () => { const client = new ServiceClient(clientOptions); const originalBody = { foo: 'bar' }; @@ -255,18 +650,16 @@ describe('ServiceClient', () => { }); }); - it('should open the circuit after 50% from 11 requests failed', (done) => { + it('should open the circuit after 50% from 5 requests failed', (done) => { const httpErrorResponse = Promise.resolve({ statusCode: 500, headers: {}, body: '{}' }); const errorResponse = Promise.resolve(Promise.reject(new Error('timeout'))); - const requests = Array.from({ length: 11 }); + const requests = Array.from({ length: 3 }); - [ emptySuccessResponse, emptySuccessResponse, httpErrorResponse, emptySuccessResponse, errorResponse, errorResponse, - httpErrorResponse, emptySuccessResponse, httpErrorResponse, errorResponse, emptySuccessResponse - ].forEach((response, index) => { + [ httpErrorResponse, emptySuccessResponse, errorResponse ].forEach((response, index) => { requestStub.onCall(index).returns(response); }); @@ -307,7 +700,7 @@ describe('ServiceClient', () => { }; it('should pass reasonable request params by default', (done) => { const client = new ServiceClient(clientOptions); - client.request().then(() => { + client.faultTolerantRequest().then(() => { assert.deepStrictEqual(requestStub.firstCall.args[0], expectedDefaultRequestOptions); done(); }); @@ -357,7 +750,7 @@ describe('ServiceClient', () => { const client = new ServiceClient(Object.assign({}, clientOptions, { defaultRequestOptions: userDefaultRequestOptions })); - client.request().then(() => { + client.faultTolerantRequest().then(() => { assert.deepStrictEqual( requestStub.firstCall.args[0], Object.assign({}, expectedDefaultRequestOptions, userDefaultRequestOptions, { port: 80 }) @@ -369,7 +762,7 @@ describe('ServiceClient', () => { const client = new ServiceClient(Object.assign({}, clientOptions, { defaultRequestOptions: { hostname: 'zalando.de' } })); - client.request().then(() => { + client.faultTolerantRequest().then(() => { assert.deepStrictEqual( requestStub.firstCall.args[0], Object.assign({}, expectedDefaultRequestOptions) @@ -379,7 +772,7 @@ describe('ServiceClient', () => { }); it('should support taking hostname and default params from a URL instead of an object', (done) => { const client = new ServiceClient('http://localhost:9999/foo?param=42'); - client.request().then(() => { + client.faultTolerantRequest().then(() => { assert.deepEqual( requestStub.firstCall.args[0], Object.assign({}, expectedDefaultRequestOptions, { From 6bff0ca17ff94fdcd45a8dfba05d36af2009c12e Mon Sep 17 00:00:00 2001 From: djones Date: Wed, 11 Oct 2017 09:09:32 +0200 Subject: [PATCH 3/9] add retry to request function and transient error config --- README.md | 31 ---- lib/client.js | 51 +++--- test/client.test.js | 395 +------------------------------------------- 3 files changed, 21 insertions(+), 456 deletions(-) diff --git a/README.md b/README.md index 51dbb95..b50f149 100644 --- a/README.md +++ b/README.md @@ -109,37 +109,6 @@ const catWatch = new ServiceClient({ }); ``` -## Fault Tolerant Requests - -Depending on the nature of the data required, it can be a good idea to perform a retry in the instance of a failed request. - -The ServiceClient also has the ability to make a `faultTolerantRequest()` which will preform retries when the request fails, based on the retryOptions configuration. - -Internally `perron` uses [retry](https://github.com/tim-kos/node-retry) in order to perform retries with exponential backoff. - -```js -const ServiceClient = require('perron'); - -const catWatch = new ServiceClient({ - hostname: 'catwatch.opensource.zalan.do', - // These are the default settings - retryOptions: { - retries: 3, - factor: 2, - minTimeout: 500, - maxTimeout: 1000, - randomize: true - } -}); - -catWatch.faultTolerantRequest({ - pathname: '/projects', - query: { - limit: 10 - } -}).then(data => console.log(data)); -``` - ## Filters It's quite often necessary to do some pre- or post-processing of the request. For this purpose `perron` implements a concept of filters, that are just an object with 2 optional methods: `request` and `response`. diff --git a/lib/client.js b/lib/client.js index 026a30b..25350e6 100644 --- a/lib/client.js +++ b/lib/client.js @@ -22,6 +22,13 @@ const retry = require('retry'); * service: string, * filters?: Array., * timing?: boolean, + * retryOptions?: { + * retries?: number, + * factor?: number, + * minTimeout?: number, + * maxTimeout?: number, + * randomize?: boolean + * }, * circuitBreaker?: (false|{ * windowDuration?: number, * numBuckets?: number, @@ -218,11 +225,16 @@ class ServiceClient { }, this.options.defaultRequestOptions); this.options.retryOptions = Object.assign({ - retries: 1, + retries: 0, factor: 2, minTimeout: 200, - maxTimeout: 200, - randomize: false + maxTimeout: 300, + randomize: true, + // this is the default function to check transient errors + // users should define their own check in order to handle errors as they require + transientErrorCheck: (err) => { + return (err && err.response && err.response.statusCode >= 500) || (err && err.toString().indexOf('timeout') > -1); + } }, this.options.retryOptions); if (this.options.retryOptions.minTimeout > this.options.retryOptions.maxTimeout) { @@ -259,32 +271,6 @@ class ServiceClient { accept: 'application/json' }, params.headers); - return new Promise((resolve, reject) => this.breaker.run( - (success, failure) => { - return requestWithFilters(params, this.options.filters) - .then(result => { - success(); resolve(result); - }) - .catch(error => { - failure(); reject(error); - }); - }, - () => { - reject(new ServiceClient.Error({}, ServiceClient.CIRCUIT_OPEN)); - } - )); - } - - faultTolerantRequest(userParams) { - const params = Object.assign({}, this.options.defaultRequestOptions, userParams); - - params.hostname = this.options.hostname; - params.port = params.port || (params.protocol === 'https:' ? 443 : 80); - - params.headers = Object.assign({ - accept: 'application/json' - }, params.headers); - return new Promise((resolve, reject) => this.breaker.run( (success, failure) => { const opts = { @@ -298,11 +284,12 @@ class ServiceClient { operation.attempt(() => { return requestWithFilters(params, this.options.filters) .then(result => { - success(); resolve(result); + success(); + resolve(result); }) .catch(error => { - if (!operation.retry(error)) { - failure(); + failure(); + if (!this.options.retryOptions.transientErrorCheck(error) || !operation.retry(error)) { reject(error); } }); diff --git a/test/client.test.js b/test/client.test.js index 4d1687c..20a6066 100644 --- a/test/client.test.js +++ b/test/client.test.js @@ -1,5 +1,7 @@ 'use strict'; +/* eslint-disable */ + const assert = require('assert'); const proxyquire = require('proxyquire').noCallThru(); const sinon = require('sinon'); @@ -397,396 +399,3 @@ describe('ServiceClient', () => { }); }); }); - - -describe('ServiceClient with faultTolerantRequest', () => { - let clientOptions; - - const requestStub = sinon.stub(); - const emptySuccessResponse = Promise.resolve({ - statusCode: 200, - headers: {}, - body: '{}' - }); - - const ServiceClient = proxyquire('../lib/client', { - './request': requestStub - }); - - beforeEach(() => { - clientOptions = { - hostname: 'catwatch.opensource.zalan.do' - }; - requestStub.reset(); - requestStub.returns(emptySuccessResponse); - }); - - it('should throw if the service is not provided', () => { - assert.throws(() => { - new ServiceClient({}); // eslint-disable-line no-new - }); - }); - - it('should by default send an `accept` application/json header', () => { - const client = new ServiceClient(clientOptions); - return client.faultTolerantRequest().then(() => { - assert.equal( - requestStub.firstCall.args[0].headers.accept, - 'application/json' - ); - }); - }); - - it('should not add authorization header if there is no token provider', () => { - const client = new ServiceClient(clientOptions); - return client.faultTolerantRequest().then(() => { - assert.strictEqual( - requestStub.firstCall.args[0].headers.authorization, - undefined - ); - }); - }); - - it('should automatically parse response as JSON if content type is set correctly', () => { - const client = new ServiceClient(clientOptions); - const originalBody = { foo: 'bar' }; - requestStub.returns({ - headers: { - 'content-type': 'application/json+something' - }, - body: JSON.stringify(originalBody) - }); - return client.faultTolerantRequest().then(({ body }) => { - assert.deepStrictEqual(body, originalBody); - }); - }); - - it('should automatically parse response as JSON if content type is not set', () => { - const client = new ServiceClient(clientOptions); - const originalBody = { foo: 'bar' }; - requestStub.returns(Promise.resolve({ - headers: {}, - body: JSON.stringify(originalBody) - })); - return client.faultTolerantRequest().then(({ body }) => { - assert.deepStrictEqual(body, originalBody); - }); - }); - - it('should not throw an error if body or content-type is not set', () => { - const client = new ServiceClient(clientOptions); - requestStub.returns(Promise.resolve({ - headers: {}, - body: '' - })); - return client.faultTolerantRequest().then(({ body }) => { - assert.equal(body, ''); - }); - }); - - it('should throw an error if body is not set for application/json content type', (done) => { - const client = new ServiceClient(clientOptions); - const response = { - headers: { 'content-type': 'application/json' }, - body: '' - }; - requestStub.returns(Promise.resolve(response)); - client.faultTolerantRequest().catch((err) => { - assert(err instanceof ServiceClient.Error); - assert.equal(err.type, ServiceClient.BODY_PARSE_FAILED); - assert.deepStrictEqual(err.response, response); - done(); - }); - }); - - it('should give a custom error object when the parsing of the body fails', (done) => { - const client = new ServiceClient(clientOptions); - const response = { - headers: {}, - body: '/not a JSON' - }; - requestStub.returns(Promise.resolve(response)); - client.faultTolerantRequest().catch(err => { - assert(err instanceof ServiceClient.Error); - assert.equal(err.type, ServiceClient.BODY_PARSE_FAILED); - assert.deepStrictEqual(err.response, response); - done(); - }); - }); - - it('should give a custom error object when request fails', (done) => { - const client = new ServiceClient(clientOptions); - const requestError = new Error('foobar'); - requestStub.returns(Promise.reject(requestError)); - client.faultTolerantRequest().catch(err => { - assert(err instanceof ServiceClient.Error); - assert.equal(err.type, ServiceClient.REQUEST_FAILED); - done(); - }); - }); - - it('should allow to mark request as failed in the request filter', (done) => { - clientOptions.filters = [{ - request() { - throw new Error('Failed!'); - } - }]; - const client = new ServiceClient(clientOptions); - client.faultTolerantRequest().catch(err => { - assert(err instanceof ServiceClient.Error); - assert.equal(err.type, 'Request filter marked request as failed'); - done(); - }); - }); - - it('should by default handle 5xx code in a response-filter', (done) => { - const client = new ServiceClient(clientOptions); - requestStub.returns(Promise.resolve({ - statusCode: 501, - headers: {}, - body: '{}' - })); - client.faultTolerantRequest().catch(err => { - assert(err instanceof ServiceClient.Error); - assert.equal(err.type, 'Response filter marked request as failed'); - done(); - }); - }); - - it('should be able to handle 4xx code as a response-filter', (done) => { - clientOptions.filters = [ - ServiceClient.treat4xxAsError, - ServiceClient.treat5xxAsError - ]; - const client = new ServiceClient(clientOptions); - requestStub.returns(Promise.resolve({ - statusCode: 403, - headers: {}, - body: '{}' - })); - client.faultTolerantRequest().catch(err => { - assert(err instanceof ServiceClient.Error); - assert.equal(err.type, ServiceClient.RESPONSE_FILTER_FAILED); - done(); - }); - }); - - it('should be possible to define your own response-filters', (done) => { - clientOptions.filters = [{ - response(response) { - if (response.body.error) { - throw new Error(response.body.error); - } - return response; - } - }]; - const client = new ServiceClient(clientOptions); - requestStub.returns(Promise.resolve({ - statusCode: 200, - headers: {}, - body: '{ "error": "non-REST-error" }' - })); - client.faultTolerantRequest().catch(err => { - assert(err instanceof ServiceClient.Error); - assert.equal(err.type, ServiceClient.RESPONSE_FILTER_FAILED); - assert(err.message.includes('non-REST-error')); - done(); - }); - }); - - it('should have the original response in a response filter error', (done) => { - clientOptions.filters = [{ - response() { - throw new Error(); - } - }]; - const client = new ServiceClient(clientOptions); - const response = { - statusCode: 200, - headers: {}, - body: '{ "error": "non-REST-error" }' - }; - requestStub.returns(Promise.resolve(response)); - client.faultTolerantRequest().catch(err => { - assert.deepStrictEqual(err.response, response); - done(); - }); - }); - - it('should allow to specify request-filters to augment the request', (done) => { - clientOptions.filters = [{ - request(request) { - request.path = 'foo-bar-buzz'; - return request; - } - }]; - const client = new ServiceClient(clientOptions); - client.faultTolerantRequest().then(() => { - assert.equal( - requestStub.firstCall.args[0].path, - 'foo-bar-buzz' - ); - done(); - }); - }); - - it('should allow to specify a request-filters to short-circuit a response', (done) => { - const headers = { - 'x-my-custom-header': 'foobar' - }; - const body = { - foo: 'bar' - }; - clientOptions.filters = [{ - request() { - return new ServiceClient.Response(404, headers, body); - } - }]; - const client = new ServiceClient(clientOptions); - client.faultTolerantRequest().then((response) => { - assert.deepStrictEqual(response.headers, headers); - assert.deepStrictEqual(response.body, body); - done(); - }); - }); - - it('should open the circuit after 50% from 5 requests failed', (done) => { - const httpErrorResponse = Promise.resolve({ - statusCode: 500, - headers: {}, - body: '{}' - }); - const errorResponse = Promise.resolve(Promise.reject(new Error('timeout'))); - const requests = Array.from({ length: 3 }); - - [ httpErrorResponse, emptySuccessResponse, errorResponse ].forEach((response, index) => { - requestStub.onCall(index).returns(response); - }); - - const client = new ServiceClient(clientOptions); - requests.reduce((promise) => { - const tick = () => { - return client.faultTolerantRequest(); - }; - return promise.then(tick, tick); - }, Promise.resolve()).then(() => { - return client.faultTolerantRequest().catch((err) => { - assert(err instanceof ServiceClient.Error); - assert(err.type, ServiceClient.CIRCUIT_OPEN); - done(); - }); - }); - }); - - describe('built-in filter', () => { - it('should return original response if all ok', () => { - [ServiceClient.treat4xxAsError, ServiceClient.treat5xxAsError].forEach(filter => { - const response = { statusCode: 200 }; - assert.deepStrictEqual(filter.response(response), response); - }); - }); - }); - - describe('request params', () => { - const expectedDefaultRequestOptions = { - hostname: 'catwatch.opensource.zalan.do', - protocol: 'https:', - port: 443, - headers: { - accept: 'application/json' - }, - pathname: '/', - timeout: 2000 - }; - it('should pass reasonable request params by default', (done) => { - const client = new ServiceClient(clientOptions); - client.faultTolerantRequest().then(() => { - assert.deepStrictEqual(requestStub.firstCall.args[0], expectedDefaultRequestOptions); - done(); - }); - }); - it('should allow to pass additional params to the request', (done) => { - const client = new ServiceClient(clientOptions); - client.request({ foo: 'bar' }).then(() => { - assert.deepStrictEqual( - requestStub.firstCall.args[0], - Object.assign({ foo: 'bar' }, expectedDefaultRequestOptions) - ); - done(); - }); - }); - it('should allow to override params of the request', (done) => { - const client = new ServiceClient(clientOptions); - client.request({ pathname: '/foo' }).then(() => { - assert.deepStrictEqual( - requestStub.firstCall.args[0], - Object.assign({}, expectedDefaultRequestOptions, { pathname: '/foo' }) - ); - done(); - }); - }); - it('should allow to specify query params of the request', (done) => { - const client = new ServiceClient(clientOptions); - client.request({ - pathname: '/foo', - query: { param: 1 } - }).then(() => { - assert.deepStrictEqual( - requestStub.firstCall.args[0], - Object.assign({}, expectedDefaultRequestOptions, { - pathname: '/foo', - query: { param: 1 } - }) - ); - done(); - }); - }); - it('should allow to specify default params of the request', (done) => { - const userDefaultRequestOptions = { - pathname: '/foo', - protocol: 'http:', - query: { param: 42 } - }; - const client = new ServiceClient(Object.assign({}, clientOptions, { - defaultRequestOptions: userDefaultRequestOptions - })); - client.faultTolerantRequest().then(() => { - assert.deepStrictEqual( - requestStub.firstCall.args[0], - Object.assign({}, expectedDefaultRequestOptions, userDefaultRequestOptions, { port: 80 }) - ); - done(); - }); - }); - it('should not allow to override hostname', (done) => { - const client = new ServiceClient(Object.assign({}, clientOptions, { - defaultRequestOptions: { hostname: 'zalando.de' } - })); - client.faultTolerantRequest().then(() => { - assert.deepStrictEqual( - requestStub.firstCall.args[0], - Object.assign({}, expectedDefaultRequestOptions) - ); - done(); - }); - }); - it('should support taking hostname and default params from a URL instead of an object', (done) => { - const client = new ServiceClient('http://localhost:9999/foo?param=42'); - client.faultTolerantRequest().then(() => { - assert.deepEqual( - requestStub.firstCall.args[0], - Object.assign({}, expectedDefaultRequestOptions, { - port: 9999, - hostname: 'localhost', - pathname: '/foo', - protocol: 'http:', - query: { - param: '42' - } - }) - ); - done(); - }); - }); - }); -}); From 8329ddb8d41a58b044c572773c80cc6974710d42 Mon Sep 17 00:00:00 2001 From: djones Date: Wed, 11 Oct 2017 09:15:53 +0200 Subject: [PATCH 4/9] update readme --- README.md | 32 ++++++++++++++++++++++++++++++++ 1 file changed, 32 insertions(+) diff --git a/README.md b/README.md index b50f149..e7a618a 100644 --- a/README.md +++ b/README.md @@ -109,6 +109,38 @@ const catWatch = new ServiceClient({ }); ``` +## Retry Logic + +For application critical requests it can be a good idea to retry failed requests to the responsible services. + +Occasionaly target server can have high latency for a short period of time, or in the case of a stack of servers, one server can be having issues +and retrying the request will allow perron to attempt to access one of the other servers that currently aren't facing issues. + +By default `perron` has retry logic implemented, but configured to perform 0 retries. Internally `perron` [node-retry](https://github.com/tim-kos/node-retry) to handle the retry logic +and configuration. All of the existing options provided by `node-retry` can be passed via configuration options through `perron`. + +There is also the addition of a transient error check function. This can be defined in any way by the consumer and is used in the try logic to determine whether to +attempt the retries or not depending on the type of error. If the function returns true and the number of retries hasn't been exceeded, the request can be retried. + +```js +const ServiceClient = require('perron'); + +const catWatch = new ServiceClient({ + hostname: 'catwatch.opensource.zalan.do', + // These are the default settings + retryOptions: { + retries: 1, + factor: 2, + minTimeout: 200, + maxTimeout: 300, + randomize: true, + transientErrorCheck: (err) => { + return (err && err.response && err.response.statusCode >= 500); + } + } +}); +``` + ## Filters It's quite often necessary to do some pre- or post-processing of the request. For this purpose `perron` implements a concept of filters, that are just an object with 2 optional methods: `request` and `response`. From a987bdfe50eaf9adbaff21ef2b5f2d8e1fcddc0f Mon Sep 17 00:00:00 2001 From: djones Date: Wed, 11 Oct 2017 15:29:02 +0200 Subject: [PATCH 5/9] cleaning up pr comments --- README.md | 4 ++-- lib/client.js | 6 +++--- test/client.test.js | 2 -- 3 files changed, 5 insertions(+), 7 deletions(-) diff --git a/README.md b/README.md index e7a618a..bd2f6bc 100644 --- a/README.md +++ b/README.md @@ -116,7 +116,7 @@ For application critical requests it can be a good idea to retry failed requests Occasionaly target server can have high latency for a short period of time, or in the case of a stack of servers, one server can be having issues and retrying the request will allow perron to attempt to access one of the other servers that currently aren't facing issues. -By default `perron` has retry logic implemented, but configured to perform 0 retries. Internally `perron` [node-retry](https://github.com/tim-kos/node-retry) to handle the retry logic +By default `perron` has retry logic implemented, but configured to perform 0 retries. Internally `perron` uses [node-retry](https://github.com/tim-kos/node-retry) to handle the retry logic and configuration. All of the existing options provided by `node-retry` can be passed via configuration options through `perron`. There is also the addition of a transient error check function. This can be defined in any way by the consumer and is used in the try logic to determine whether to @@ -134,7 +134,7 @@ const catWatch = new ServiceClient({ minTimeout: 200, maxTimeout: 300, randomize: true, - transientErrorCheck: (err) => { + transientErrorCheck(err) { return (err && err.response && err.response.statusCode >= 500); } } diff --git a/lib/client.js b/lib/client.js index 25350e6..c284ec1 100644 --- a/lib/client.js +++ b/lib/client.js @@ -230,10 +230,10 @@ class ServiceClient { minTimeout: 200, maxTimeout: 300, randomize: true, - // this is the default function to check transient errors + // By default all errors are indicated as being transient // users should define their own check in order to handle errors as they require - transientErrorCheck: (err) => { - return (err && err.response && err.response.statusCode >= 500) || (err && err.toString().indexOf('timeout') > -1); + transientErrorCheck() { + return true; } }, this.options.retryOptions); diff --git a/test/client.test.js b/test/client.test.js index 20a6066..b8b8ab3 100644 --- a/test/client.test.js +++ b/test/client.test.js @@ -1,7 +1,5 @@ 'use strict'; -/* eslint-disable */ - const assert = require('assert'); const proxyquire = require('proxyquire').noCallThru(); const sinon = require('sinon'); From e6ef98295ca4f5fdc5f81cfb3966b699e17adc2d Mon Sep 17 00:00:00 2001 From: djones Date: Thu, 12 Oct 2017 11:00:53 +0200 Subject: [PATCH 6/9] update maxTimeout --- README.md | 2 +- lib/client.js | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/README.md b/README.md index bd2f6bc..adaed13 100644 --- a/README.md +++ b/README.md @@ -132,7 +132,7 @@ const catWatch = new ServiceClient({ retries: 1, factor: 2, minTimeout: 200, - maxTimeout: 300, + maxTimeout: 400, randomize: true, transientErrorCheck(err) { return (err && err.response && err.response.statusCode >= 500); diff --git a/lib/client.js b/lib/client.js index c284ec1..eefa27d 100644 --- a/lib/client.js +++ b/lib/client.js @@ -228,7 +228,7 @@ class ServiceClient { retries: 0, factor: 2, minTimeout: 200, - maxTimeout: 300, + maxTimeout: 400, randomize: true, // By default all errors are indicated as being transient // users should define their own check in order to handle errors as they require From c50870c8224544a9f40281f6a16385e2b7d581fa Mon Sep 17 00:00:00 2001 From: djones Date: Fri, 13 Oct 2017 17:40:55 +0200 Subject: [PATCH 7/9] add retry callback and initial test --- README.md | 9 ++++-- lib/client.js | 76 +++++++++++++++++++++++++++++---------------- test/client.test.js | 60 +++++++++++++++++++++++++++++++++++ 3 files changed, 117 insertions(+), 28 deletions(-) diff --git a/README.md b/README.md index adaed13..8d3ceb3 100644 --- a/README.md +++ b/README.md @@ -119,15 +119,17 @@ and retrying the request will allow perron to attempt to access one of the other By default `perron` has retry logic implemented, but configured to perform 0 retries. Internally `perron` uses [node-retry](https://github.com/tim-kos/node-retry) to handle the retry logic and configuration. All of the existing options provided by `node-retry` can be passed via configuration options through `perron`. -There is also the addition of a transient error check function. This can be defined in any way by the consumer and is used in the try logic to determine whether to +There is a transient error check function. This can be defined in any way by the consumer and is used in the try logic to determine whether to attempt the retries or not depending on the type of error. If the function returns true and the number of retries hasn't been exceeded, the request can be retried. +There is also an onRetry function which can be defined by the user of `perron`. This function is called every time a retry request will be triggered. +It is provided the currentAttempt, as well as the error that is causing the retry. + ```js const ServiceClient = require('perron'); const catWatch = new ServiceClient({ hostname: 'catwatch.opensource.zalan.do', - // These are the default settings retryOptions: { retries: 1, factor: 2, @@ -136,6 +138,9 @@ const catWatch = new ServiceClient({ randomize: true, transientErrorCheck(err) { return (err && err.response && err.response.statusCode >= 500); + }, + onRetry(currentAttempt, err) { + console.log('Retry attempt #' + currentAttempt + 'due to ' + err); } } }); diff --git a/lib/client.js b/lib/client.js index eefa27d..2a4cd57 100644 --- a/lib/client.js +++ b/lib/client.js @@ -232,8 +232,19 @@ class ServiceClient { randomize: true, // By default all errors are indicated as being transient // users should define their own check in order to handle errors as they require - transientErrorCheck() { + // it takes the error as an argument + transientErrorCheck(err) { + // by default don't retry if the circuit breaker is open + if (err.type === ServiceClient.CIRCUIT_OPEN) { + return false; + } return true; + }, + // this is called each time a retry is triggered + // it receives the current retry attempt and error as args + // eslint-disable-next-line + onRetry(currentAttempt, err) { + return currentAttempt; } }, this.options.retryOptions); @@ -271,34 +282,47 @@ class ServiceClient { accept: 'application/json' }, params.headers); - return new Promise((resolve, reject) => this.breaker.run( - (success, failure) => { - const opts = { - retries: this.options.retryOptions.retries, - factor: this.options.retryOptions.factor, - minTimeout: this.options.retryOptions.minTimeout, - maxTimeout: this.options.retryOptions.maxTimeout, - randomize: this.options.retryOptions.randomize - }; - const operation = retry.operation(opts); - operation.attempt(() => { - return requestWithFilters(params, this.options.filters) - .then(result => { - success(); - resolve(result); - }) - .catch(error => { - failure(); - if (!this.options.retryOptions.transientErrorCheck(error) || !operation.retry(error)) { - reject(error); - } - }); - }); + const { + retries, + factor, + minTimeout, + maxTimeout, + randomize, + transientErrorCheck, + onRetry + } = this.options.retryOptions; + + const opts = { + retries, + factor, + minTimeout, + maxTimeout, + randomize + }; + + const operation = retry.operation(opts); + + return new Promise((resolve, reject) => operation.attempt((currentAttempt) => { + this.breaker.run((success, failure) => { + return requestWithFilters(params, this.options.filters) + .then(result => { + success(); + resolve(result); + }) + .catch(error => { + failure(); + const shouldRetry = operation.retry(error); + if (!transientErrorCheck(error) || !shouldRetry) { + reject(error); + return; + } + onRetry(currentAttempt, error); + }); }, () => { reject(new ServiceClient.Error({}, ServiceClient.CIRCUIT_OPEN)); - } - )); + }); + })); } } diff --git a/test/client.test.js b/test/client.test.js index b8b8ab3..5d349d0 100644 --- a/test/client.test.js +++ b/test/client.test.js @@ -396,4 +396,64 @@ describe('ServiceClient', () => { }); }); }); + + it('should perform the desired number of retries based on the configuration', (done) => { + let numberOfRetries = 0; + clientOptions.retryOptions = { + retries: 1, + onRetry() { + numberOfRetries += 1; + } + }; + const client = new ServiceClient(clientOptions); + requestStub.returns(Promise.resolve({ + statusCode: 501, + headers: {}, + body: '{}' + })); + client.request().catch(err => { + assert.equal(numberOfRetries, 1); + assert.equal(err instanceof ServiceClient.Error, true); + assert.equal(err.type, 'Response filter marked request as failed'); + done(); + }); + }); + + it('should open the circuit after 50% from 11 requests failed and correct number of retries were performed', (done) => { + const httpErrorResponse = Promise.resolve({ + statusCode: 500, + headers: {}, + body: '{}' + }); + let numberOfRetries = 0; + clientOptions.retryOptions = { + retries: 1, + onRetry() { + numberOfRetries += 1; + } + }; + const errorResponse = Promise.resolve(Promise.reject(new Error('timeout'))); + const requests = Array.from({ length: 11 }); + + [ emptySuccessResponse, emptySuccessResponse, errorResponse, emptySuccessResponse, httpErrorResponse, errorResponse, + httpErrorResponse, emptySuccessResponse, errorResponse, httpErrorResponse, emptySuccessResponse + ].forEach((response, index) => { + requestStub.onCall(index).returns(response); + }); + + const client = new ServiceClient(clientOptions); + requests.reduce((promise) => { + const tick = () => { + return client.request(); + }; + return promise.then(tick, tick); + }, Promise.resolve()).then(() => { + return client.request(); + }).catch((err) => { + assert.equal(numberOfRetries, 4); + assert.equal(err instanceof ServiceClient.Error, true); + assert.equal(err.type, ServiceClient.CIRCUIT_OPEN); + done(); + }); + }); }); From 3dcacfd8e760d1e7c82652b78c30ab7d2342807e Mon Sep 17 00:00:00 2001 From: djones Date: Tue, 17 Oct 2017 11:39:11 +0200 Subject: [PATCH 8/9] some cleanup based on PR comments --- README.md | 4 ++-- lib/client.js | 32 ++++++++++++++------------------ test/client.test.js | 32 ++++++++++++++++++++++++++++++-- 3 files changed, 46 insertions(+), 22 deletions(-) diff --git a/README.md b/README.md index 8d3ceb3..c0e4c7f 100644 --- a/README.md +++ b/README.md @@ -119,7 +119,7 @@ and retrying the request will allow perron to attempt to access one of the other By default `perron` has retry logic implemented, but configured to perform 0 retries. Internally `perron` uses [node-retry](https://github.com/tim-kos/node-retry) to handle the retry logic and configuration. All of the existing options provided by `node-retry` can be passed via configuration options through `perron`. -There is a transient error check function. This can be defined in any way by the consumer and is used in the try logic to determine whether to +There is a shouldRetry function which can be defined in any way by the consumer and is used in the try logic to determine whether to attempt the retries or not depending on the type of error. If the function returns true and the number of retries hasn't been exceeded, the request can be retried. There is also an onRetry function which can be defined by the user of `perron`. This function is called every time a retry request will be triggered. @@ -136,7 +136,7 @@ const catWatch = new ServiceClient({ minTimeout: 200, maxTimeout: 400, randomize: true, - transientErrorCheck(err) { + shouldRetry(err) { return (err && err.response && err.response.statusCode >= 500); }, onRetry(currentAttempt, err) { diff --git a/lib/client.js b/lib/client.js index 2a4cd57..debd57c 100644 --- a/lib/client.js +++ b/lib/client.js @@ -27,7 +27,9 @@ const retry = require('retry'); * factor?: number, * minTimeout?: number, * maxTimeout?: number, - * randomize?: boolean + * randomize?: boolean, + * shouldRetry?: Function, + * onRetry?: Function * }, * circuitBreaker?: (false|{ * windowDuration?: number, @@ -230,26 +232,20 @@ class ServiceClient { minTimeout: 200, maxTimeout: 400, randomize: true, - // By default all errors are indicated as being transient - // users should define their own check in order to handle errors as they require - // it takes the error as an argument - transientErrorCheck(err) { - // by default don't retry if the circuit breaker is open - if (err.type === ServiceClient.CIRCUIT_OPEN) { - return false; - } + // Users should define their own check in order to handle errors as they require + // It takes the error as an argument and the function should return a boolean + // eslint-disable-next-line + shouldRetry(err) { return true; }, - // this is called each time a retry is triggered - // it receives the current retry attempt and error as args + // This is called each time a retry is triggered + // It receives the current retry attempt and error as args // eslint-disable-next-line - onRetry(currentAttempt, err) { - return currentAttempt; - } + onRetry(currentAttempt, err) {} }, this.options.retryOptions); if (this.options.retryOptions.minTimeout > this.options.retryOptions.maxTimeout) { - throw new Error('The `retryInterval` must be equal to or greater than the `minTimeout`'); + throw new Error('The `maxTimeout` must be equal to or greater than the `minTimeout`'); } if (this.options.circuitBreaker) { @@ -288,7 +284,7 @@ class ServiceClient { minTimeout, maxTimeout, randomize, - transientErrorCheck, + shouldRetry, onRetry } = this.options.retryOptions; @@ -311,8 +307,8 @@ class ServiceClient { }) .catch(error => { failure(); - const shouldRetry = operation.retry(error); - if (!transientErrorCheck(error) || !shouldRetry) { + const willRetry = operation.retry(error); + if (!shouldRetry(error) || !willRetry) { reject(error); return; } diff --git a/test/client.test.js b/test/client.test.js index 5d349d0..fb69bf1 100644 --- a/test/client.test.js +++ b/test/client.test.js @@ -400,7 +400,7 @@ describe('ServiceClient', () => { it('should perform the desired number of retries based on the configuration', (done) => { let numberOfRetries = 0; clientOptions.retryOptions = { - retries: 1, + retries: 3, onRetry() { numberOfRetries += 1; } @@ -412,7 +412,7 @@ describe('ServiceClient', () => { body: '{}' })); client.request().catch(err => { - assert.equal(numberOfRetries, 1); + assert.equal(numberOfRetries, 3); assert.equal(err instanceof ServiceClient.Error, true); assert.equal(err.type, 'Response filter marked request as failed'); done(); @@ -456,4 +456,32 @@ describe('ServiceClient', () => { done(); }); }); + + it('should not retry if the shouldRetry function returns false', (done) => { + let numberOfRetries = 0; + clientOptions.retryOptions = { + retries: 1, + shouldRetry(err) { + if (err.response.statusCode === 501) { + return false; + } + return true; + }, + onRetry() { + numberOfRetries += 1; + } + }; + const client = new ServiceClient(clientOptions); + requestStub.returns(Promise.resolve({ + statusCode: 501, + headers: {}, + body: '{}' + })); + client.request().catch(err => { + assert.equal(numberOfRetries, 0); + assert.equal(err instanceof ServiceClient.Error, true); + assert.equal(err.type, 'Response filter marked request as failed'); + done(); + }); + }); }); From bbc58dde7bf361ff06e61ee47743d5d4a942e5fc Mon Sep 17 00:00:00 2001 From: djones Date: Tue, 17 Oct 2017 13:45:47 +0200 Subject: [PATCH 9/9] check shouldRetry before operation.retry --- README.md | 8 +++++--- lib/client.js | 15 +++++++-------- 2 files changed, 12 insertions(+), 11 deletions(-) diff --git a/README.md b/README.md index c0e4c7f..d6bb6b2 100644 --- a/README.md +++ b/README.md @@ -119,12 +119,14 @@ and retrying the request will allow perron to attempt to access one of the other By default `perron` has retry logic implemented, but configured to perform 0 retries. Internally `perron` uses [node-retry](https://github.com/tim-kos/node-retry) to handle the retry logic and configuration. All of the existing options provided by `node-retry` can be passed via configuration options through `perron`. -There is a shouldRetry function which can be defined in any way by the consumer and is used in the try logic to determine whether to -attempt the retries or not depending on the type of error. If the function returns true and the number of retries hasn't been exceeded, the request can be retried. +There is a shouldRetry function which can be defined in any way by the consumer and is used in the try logic to determine whether to attempt the retries or not depending on the type of error and the original request object. +If the function returns true and the number of retries hasn't been exceeded, the request can be retried. There is also an onRetry function which can be defined by the user of `perron`. This function is called every time a retry request will be triggered. It is provided the currentAttempt, as well as the error that is causing the retry. +The first time onRetry gets called, the value of currentAttempt will be 2. This is because the first initial request is counted as the first attempt, and the first retry attempted will then be the second request. + ```js const ServiceClient = require('perron'); @@ -136,7 +138,7 @@ const catWatch = new ServiceClient({ minTimeout: 200, maxTimeout: 400, randomize: true, - shouldRetry(err) { + shouldRetry(err, req) { return (err && err.response && err.response.statusCode >= 500); }, onRetry(currentAttempt, err) { diff --git a/lib/client.js b/lib/client.js index debd57c..44f7f9f 100644 --- a/lib/client.js +++ b/lib/client.js @@ -232,14 +232,10 @@ class ServiceClient { minTimeout: 200, maxTimeout: 400, randomize: true, - // Users should define their own check in order to handle errors as they require - // It takes the error as an argument and the function should return a boolean // eslint-disable-next-line - shouldRetry(err) { + shouldRetry(err, req) { return true; }, - // This is called each time a retry is triggered - // It receives the current retry attempt and error as args // eslint-disable-next-line onRetry(currentAttempt, err) {} }, this.options.retryOptions); @@ -307,12 +303,15 @@ class ServiceClient { }) .catch(error => { failure(); - const willRetry = operation.retry(error); - if (!shouldRetry(error) || !willRetry) { + if (!shouldRetry(error, params)) { reject(error); return; } - onRetry(currentAttempt, error); + if (!operation.retry(error)) { + reject(error); + return; + } + onRetry(currentAttempt + 1, error); }); }, () => {