From 754c6dc612a6ebdf05df40af8294b5d1b569bd2a Mon Sep 17 00:00:00 2001 From: Andrew Bulat Date: Fri, 5 Apr 2024 11:30:19 +0100 Subject: [PATCH 1/3] Fix incorrect default value for `httpRequestTimeout` client option It should be 10 seconds according to TO3l4 --- src/common/lib/util/defaults.ts | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/common/lib/util/defaults.ts b/src/common/lib/util/defaults.ts index 16d373811..15e127141 100644 --- a/src/common/lib/util/defaults.ts +++ b/src/common/lib/util/defaults.ts @@ -73,7 +73,7 @@ const Defaults = { disconnectedRetryTimeout: 15000, suspendedRetryTimeout: 30000, /* Undocumented, but part of the api and can be used by customers: */ - httpRequestTimeout: 15000, + httpRequestTimeout: 10000, channelRetryTimeout: 15000, fallbackRetryTimeout: 600000, /* For internal / test use only: */ From 8998461b3e352ea284f20cb6a3920a90292e482c Mon Sep 17 00:00:00 2001 From: Andrew Bulat Date: Fri, 5 Apr 2024 11:43:27 +0100 Subject: [PATCH 2/3] Add max elapsed time check for rest fallback host retries for TO3l6 Add missing `httpMaxRetryDuration` value to Defaults.TIMEOUTS Resolves #1717 --- src/common/lib/util/defaults.ts | 2 ++ src/common/types/http.ts | 14 ++++++++++++++ 2 files changed, 16 insertions(+) diff --git a/src/common/lib/util/defaults.ts b/src/common/lib/util/defaults.ts index 15e127141..3d7fdf32d 100644 --- a/src/common/lib/util/defaults.ts +++ b/src/common/lib/util/defaults.ts @@ -23,6 +23,7 @@ type CompleteDefaults = IDefaults & { disconnectedRetryTimeout: number; suspendedRetryTimeout: number; httpRequestTimeout: number; + httpMaxRetryDuration: number; channelRetryTimeout: number; fallbackRetryTimeout: number; connectionStateTtl: number; @@ -74,6 +75,7 @@ const Defaults = { suspendedRetryTimeout: 30000, /* Undocumented, but part of the api and can be used by customers: */ httpRequestTimeout: 10000, + httpMaxRetryDuration: 15000, channelRetryTimeout: 15000, fallbackRetryTimeout: 600000, /* For internal / test use only: */ diff --git a/src/common/types/http.ts b/src/common/types/http.ts index 6cda77104..a39bd3b42 100644 --- a/src/common/types/http.ts +++ b/src/common/types/http.ts @@ -192,10 +192,24 @@ export class Http { return this.doUri(method, uriFromHost(hosts[0]), headers, body, params); } + let tryAHostStartedAt: Date | null = null; const tryAHost = async (candidateHosts: Array, persistOnSuccess?: boolean): Promise => { const host = candidateHosts.shift(); + tryAHostStartedAt = tryAHostStartedAt ?? new Date(); const result = await this.doUri(method, uriFromHost(host as string), headers, body, params); if (result.error && this.platformHttp.shouldFallback(result.error as ErrnoException) && candidateHosts.length) { + // TO3l6 + const elapsedTime = Date.now() - tryAHostStartedAt.getTime(); + if (elapsedTime > client.options.timeouts.httpMaxRetryDuration) { + return { + error: new ErrorInfo( + `Timeout for trying fallback hosts retries. Total elapsed time exceeded the ${client.options.timeouts.httpMaxRetryDuration}ms limit`, + 50003, + 500, + ), + }; + } + return tryAHost(candidateHosts, true); } if (persistOnSuccess) { From 5419ef3058d3e761af62faadeeab942cc24e7d90 Mon Sep 17 00:00:00 2001 From: Andrew Bulat Date: Fri, 5 Apr 2024 11:49:40 +0100 Subject: [PATCH 3/3] Add tests for TO3l6 --- test/realtime/init.test.js | 7 ++++- test/rest/fallbacks.test.js | 53 +++++++++++++++++++++++++++++++++++++ 2 files changed, 59 insertions(+), 1 deletion(-) diff --git a/test/realtime/init.test.js b/test/realtime/init.test.js index bca2f44f8..f80fb64a5 100644 --- a/test/realtime/init.test.js +++ b/test/realtime/init.test.js @@ -238,6 +238,7 @@ define(['ably', 'shared_helper', 'chai'], function (Ably, helper, chai) { disconnectedRetryTimeout: 123, suspendedRetryTimeout: 456, httpRequestTimeout: 789, + httpMaxRetryDuration: 321, }); /* Note: uses internal knowledge of connectionManager */ try { @@ -251,7 +252,11 @@ define(['ably', 'shared_helper', 'chai'], function (Ably, helper, chai) { ); expect(realtime.connection.connectionManager.options.timeouts.httpRequestTimeout).to.equal( 789, - 'Verify suspended retry frequency is settable', + 'Verify http request timeout is settable', + ); + expect(realtime.connection.connectionManager.options.timeouts.httpMaxRetryDuration).to.equal( + 321, + 'Verify http max retry duration is settable', ); } catch (err) { closeAndFinish(done, realtime, err); diff --git a/test/rest/fallbacks.test.js b/test/rest/fallbacks.test.js index e0883def5..6e6a2d016 100644 --- a/test/rest/fallbacks.test.js +++ b/test/rest/fallbacks.test.js @@ -50,5 +50,58 @@ define(['shared_helper', 'async', 'chai'], function (helper, async, chai) { expect(currentFallback && currentFallback.host).to.equal(goodHost, 'Check good host set again'); expect(currentFallback.validUntil > now, 'Check validUntil has been re-set').to.be.ok; }); + + // TO3l6 + describe('Max elapsed time for host retries', function () { + it('can timeout after default host', async function () { + const httpRequestTimeout = 1000; + // set httpMaxRetryDuration lower than httpRequestTimeout so it would timeout after default host attempt + const httpMaxRetryDuration = Math.floor(httpRequestTimeout / 2); + const rest = helper.AblyRest({ + restHost: helper.unroutableHost, + fallbackHosts: [helper.unroutableHost], + httpRequestTimeout, + httpMaxRetryDuration, + }); + + let thrownError = null; + try { + // we expect it to fail due to max elapsed time reached for host retries + await rest.time(); + } catch (error) { + thrownError = error; + } + + expect(thrownError).not.to.be.null; + expect(thrownError.message).to.equal( + `Timeout for trying fallback hosts retries. Total elapsed time exceeded the ${httpMaxRetryDuration}ms limit`, + ); + }); + + it('can timeout after fallback host retries', async function () { + const httpRequestTimeout = 1000; + // set httpMaxRetryDuration higher than httpRequestTimeout and lower than 2*httpRequestTimeout so it would timeout after first fallback host retry attempt + const httpMaxRetryDuration = Math.floor(httpRequestTimeout * 1.5); + const rest = helper.AblyRest({ + restHost: helper.unroutableHost, + fallbackHosts: [helper.unroutableHost, helper.unroutableHost], + httpRequestTimeout, + httpMaxRetryDuration, + }); + + let thrownError = null; + try { + // we expect it to fail due to max elapsed time reached for host retries + await rest.time(); + } catch (error) { + thrownError = error; + } + + expect(thrownError).not.to.be.null; + expect(thrownError.message).to.equal( + `Timeout for trying fallback hosts retries. Total elapsed time exceeded the ${httpMaxRetryDuration}ms limit`, + ); + }); + }); }); });