Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[ECO-4721] Fix rest fallback behavior #1721

Merged
merged 3 commits into from
Apr 17, 2024
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
4 changes: 3 additions & 1 deletion src/common/lib/util/defaults.ts
Original file line number Diff line number Diff line change
Expand Up @@ -23,6 +23,7 @@ type CompleteDefaults = IDefaults & {
disconnectedRetryTimeout: number;
suspendedRetryTimeout: number;
httpRequestTimeout: number;
httpMaxRetryDuration: number;
channelRetryTimeout: number;
fallbackRetryTimeout: number;
connectionStateTtl: number;
Expand Down Expand Up @@ -73,7 +74,8 @@ const Defaults = {
disconnectedRetryTimeout: 15000,
suspendedRetryTimeout: 30000,
/* Undocumented, but part of the api and can be used by customers: */
httpRequestTimeout: 15000,
httpRequestTimeout: 10000,
httpMaxRetryDuration: 15000,
channelRetryTimeout: 15000,
fallbackRetryTimeout: 600000,
/* For internal / test use only: */
Expand Down
14 changes: 14 additions & 0 deletions src/common/types/http.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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<string>, persistOnSuccess?: boolean): Promise<RequestResult> => {
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) {
Expand Down
7 changes: 6 additions & 1 deletion test/realtime/init.test.js
Original file line number Diff line number Diff line change
Expand Up @@ -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 {
Expand All @@ -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);
Expand Down
53 changes: 53 additions & 0 deletions test/rest/fallbacks.test.js
Original file line number Diff line number Diff line change
Expand Up @@ -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`,
);
});
});
});
});