From 2d91c2313f0f38827edb9c142eef42687e77588f Mon Sep 17 00:00:00 2001 From: chris stevens Date: Fri, 15 Nov 2024 12:19:11 +0000 Subject: [PATCH 1/3] feat: circuit broken errors no longer retried --- lib/client.js | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/lib/client.js b/lib/client.js index 91979ec..4a92be0 100644 --- a/lib/client.js +++ b/lib/client.js @@ -340,7 +340,7 @@ class HttpTransportClient { } function isCriticalError(err) { - if (err && err.statusCode < 500) { + if (err && (err.statusCode < 500 || err.isBrokenCircuitError)) { return false; } return true; From 3ab35a1a43b84a612ac4791413a9bfdb9984f871 Mon Sep 17 00:00:00 2001 From: chris stevens Date: Wed, 20 Nov 2024 16:04:00 +0000 Subject: [PATCH 2/3] feat: allow criticalErrorDetector configuration in builder --- lib/builder.js | 30 ++++++++++++++++++++++++++++++ lib/client.js | 10 ++++++---- lib/context.js | 2 ++ test/client.js | 16 ++++++++++++++++ 4 files changed, 54 insertions(+), 4 deletions(-) diff --git a/lib/builder.js b/lib/builder.js index cdd9d25..98b018b 100644 --- a/lib/builder.js +++ b/lib/builder.js @@ -69,6 +69,36 @@ class HttpTransportBuilder { return this; } + /** + * default the criticalErrorDetector which parses errors and decides if they are critical. This mainly enables retry logic. + * @param {function} criticalErrorDetector (err, ctx) => bool - a function that takes the error and the context and returns a boolean which evaluates whether an error is a critical error. + * This is useful if you want to customise error behaviour. See below for example that prevents circuitBreaker errors from being classed as critical errors. + * criticalErrors trigger retry behaviour and so may not be desirable in all scenarios. + * @return a HttpTransportBuilder instance + * @example + * const httpTransport = require('@bbc/http-transport'); + * + * const builder = httpTransport.createBuilder(); + * builder.criticalErrorDetector(() => { + * if (err && (err.statusCode < 500 || err.isBrokenCircuitError)) { + * return false; + * } + * return true; + * }); + * + * @default + * (err, ctx) => { + * if (err && err.statusCode < 500) { + * return false; + * } + * return true; + * } + */ + criticalErrorDetector(criticalErrorDetector) { + _.set(this._defaults, 'ctx.criticalErrorDetector', criticalErrorDetector); + return this; + } + /** * Registers a global plugin, which is used for all requests * diff --git a/lib/client.js b/lib/client.js index 4a92be0..d1fcf6d 100644 --- a/lib/client.js +++ b/lib/client.js @@ -339,8 +339,10 @@ class HttpTransportClient { } } -function isCriticalError(err) { - if (err && (err.statusCode < 500 || err.isBrokenCircuitError)) { +function isCriticalError(err, ctx) { + if (ctx.criticalErrorDetector) return ctx.criticalErrorDetector(err, ctx); + + if (err && err.statusCode < 500) { return false; } return true; @@ -363,14 +365,14 @@ function retry(fn, ctx) { function attempt(i) { return fn(ctx) .catch((err) => { - if (i < maxAttempts && isCriticalError(err) && ctx.retryDelay && ctx.retryDelay > 0) { + if (i < maxAttempts && isCriticalError(err, ctx) && ctx.retryDelay && ctx.retryDelay > 0) { const delayBy = rejectedPromise(ctx.retryDelay); return delayBy(err); } throw err; }) .catch((err) => { - if (i < maxAttempts && isCriticalError(err)) { + if (i < maxAttempts && isCriticalError(err, ctx)) { ctx.retryAttempts.push(toRetry(err)); return attempt(++i); } diff --git a/lib/context.js b/lib/context.js index 110d668..7805f00 100644 --- a/lib/context.js +++ b/lib/context.js @@ -19,6 +19,7 @@ class Context { this.plugins = []; this.req = Request.create(); this.res = Response.create(); + this.criticalErrorDetector = undefined; if (defaults) this._applyDefaults(defaults); } @@ -42,6 +43,7 @@ class Context { this.userAgent = defaults.ctx?.userAgent || USER_AGENT; this.retries = defaults.ctx?.retries || RETRIES; this.retryDelay = defaults.ctx?.retryDelay || RETRY_DELAY; + this.criticalErrorDetector = defaults.ctx?.criticalErrorDetector; } static create(defaults) { diff --git a/test/client.js b/test/client.js index e26c41a..e5b8917 100644 --- a/test/client.js +++ b/test/client.js @@ -141,6 +141,22 @@ describe('HttpTransportClient', () => { assert.equal(ctx.retries, 50); assert.equal(ctx.retryDelay, 2000); }); + + it('sets default criticalErrorDetector in the context', async () => { + const transport = new Transport(); + sandbox.stub(transport, 'execute').returns(Promise.resolve()); + + const client = HttpTransport.createBuilder(transport) + .criticalErrorDetector(() => false) + .createClient(); + + await client + .get(url) + .asResponse(); + + const ctx = transport.execute.getCall(0).args[0]; + assert.equal(ctx.criticalErrorDetector.toString(), (() => false).toString()); + }); }); describe('.retries', () => { From e67515261c50f335922b5e26e05b0f360d204509 Mon Sep 17 00:00:00 2001 From: chris stevens Date: Wed, 20 Nov 2024 16:14:02 +0000 Subject: [PATCH 3/3] fix: added typings for criticalErrorDetector --- index.d.ts | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/index.d.ts b/index.d.ts index 0d2977b..ddc913d 100644 --- a/index.d.ts +++ b/index.d.ts @@ -100,6 +100,7 @@ export declare class HttpTransportBuilder< userAgent(userAgent: string): HttpTransportBuilder; retries(retries: number): HttpTransportBuilder; retryDelay(retryDelay: number): HttpTransportBuilder; + criticalErrorDetector(criticalErrorDetector: (err, ctx: Context) => boolean); use( fn: Plugin ): HttpTransportBuilder; @@ -141,7 +142,7 @@ export declare class HttpTransportClient< ResponseBody >; asResponse(): Promise< - AsResponse + AsResponse >; }