From 7ff4c6fe2d3fc627292604f51ffebfbab6f70c4b Mon Sep 17 00:00:00 2001 From: Ryan Lamb <4955475+kinyoklion@users.noreply.github.com> Date: Wed, 10 Apr 2024 09:16:40 -0700 Subject: [PATCH 1/7] In progress --- packages/shared/common/src/errors.ts | 7 +++ .../shared/mocks/src/streamingProcessor.ts | 3 +- .../sdk-server/__tests__/LDClientImpl.test.ts | 10 +++- .../shared/sdk-server/src/LDClientImpl.ts | 57 ++++++++++++++++++- .../shared/sdk-server/src/api/LDClient.ts | 31 ++++++---- .../src/api/LDWaitForInitializationOptions.ts | 18 ++++++ packages/shared/sdk-server/src/api/index.ts | 1 + 7 files changed, 111 insertions(+), 16 deletions(-) create mode 100644 packages/shared/sdk-server/src/api/LDWaitForInitializationOptions.ts diff --git a/packages/shared/common/src/errors.ts b/packages/shared/common/src/errors.ts index 600d0f03e8..f85a2cc92d 100644 --- a/packages/shared/common/src/errors.ts +++ b/packages/shared/common/src/errors.ts @@ -43,6 +43,13 @@ export class LDClientError extends Error { } } +export class LDTimeoutError extends Error { + constructor(message: string) { + super(message); + this.name = 'LaunchDarklyTimeoutError'; + } +} + /** * Check if the HTTP error is recoverable. This will return false if a request * made with any payload could not recover. If the reason for the failure diff --git a/packages/shared/mocks/src/streamingProcessor.ts b/packages/shared/mocks/src/streamingProcessor.ts index 260db1654e..dc0370a77f 100644 --- a/packages/shared/mocks/src/streamingProcessor.ts +++ b/packages/shared/mocks/src/streamingProcessor.ts @@ -14,6 +14,7 @@ export const setupMockStreamingProcessor = ( patchResponseJson?: any, deleteResponseJson?: any, errorTimeoutSeconds: number = 0, + initTimeoutMs: number = 0, ) => { MockStreamingProcessor.mockImplementation( ( @@ -35,7 +36,7 @@ export const setupMockStreamingProcessor = ( }, errorTimeoutSeconds * 1000); } else { // execute put which will resolve the identify promise - setTimeout(() => listeners.get('put')?.processJson(putResponseJson)); + setTimeout(() => listeners.get('put')?.processJson(putResponseJson), initTimeoutMs); if (patchResponseJson) { setTimeout(() => listeners.get('patch')?.processJson(patchResponseJson)); diff --git a/packages/shared/sdk-server/__tests__/LDClientImpl.test.ts b/packages/shared/sdk-server/__tests__/LDClientImpl.test.ts index b36d966cb7..e34ce40479 100644 --- a/packages/shared/sdk-server/__tests__/LDClientImpl.test.ts +++ b/packages/shared/sdk-server/__tests__/LDClientImpl.test.ts @@ -132,11 +132,19 @@ describe('LDClientImpl', () => { await client.waitForInitialization(); }); - it('creates only one Promise when waiting for initialization', async () => { + it('creates only one Promise when waiting for initialization - when not using a timeout', async () => { client = createClient(); const p1 = client.waitForInitialization(); const p2 = client.waitForInitialization(); expect(p2).toBe(p1); }); + + it('rejects the returned promise when initialization does not complete within the timeout', async () => { + setupMockStreamingProcessor(undefined, undefined, undefined, undefined, undefined, 10000); + client = createClient(); + await expect(client.waitForInitialization({ timeout: 1 })).rejects.toThrow( + 'waitForInitialization timed out after 1 seconds.', + ); + }); }); diff --git a/packages/shared/sdk-server/src/LDClientImpl.ts b/packages/shared/sdk-server/src/LDClientImpl.ts index c0dbc6b077..a9de4c33cf 100644 --- a/packages/shared/sdk-server/src/LDClientImpl.ts +++ b/packages/shared/sdk-server/src/LDClientImpl.ts @@ -8,8 +8,10 @@ import { LDEvaluationDetail, LDEvaluationDetailTyped, LDLogger, + LDTimeoutError, Platform, subsystem, + timedPromise, TypeValidators, } from '@launchdarkly/js-sdk-common'; @@ -25,6 +27,7 @@ import { LDOptions, } from './api'; import { BigSegmentStoreMembership } from './api/interfaces'; +import { LDWaitForInitializationOptions } from './api/LDWaitForInitializationOptions'; import BigSegmentsManager from './BigSegmentsManager'; import BigSegmentStoreStatusProvider from './BigSegmentStatusProviderImpl'; import { createStreamListeners } from './data_sources/createStreamListeners'; @@ -55,6 +58,8 @@ enum InitState { Failed, } +const HIGH_TIMEOUT_THRESHOLD: number = 60; + export interface LDClientCallbacks { onError: (err: Error) => void; onFailed: (err: Error) => void; @@ -232,27 +237,44 @@ export default class LDClientImpl implements LDClient { return this.initState === InitState.Initialized; } - waitForInitialization(): Promise { + waitForInitialization(options?: LDWaitForInitializationOptions): Promise { // An initialization promise is only created if someone is going to use that promise. // If we always created an initialization promise, and there was no call waitForInitialization // by the time the promise was rejected, then that would result in an unhandled promise // rejection. + if (options?.timeout === undefined) { + this.logger?.warn( + 'The waitForInitialization function was called without a timeout specified.' + + ' In a future version a default timeout will be applied.', + ); + } + if (options?.timeout !== undefined && options?.timeout > HIGH_TIMEOUT_THRESHOLD) { + this.logger?.warn( + 'The waitForInitialization function was called with a timeout greater than ' + + `${HIGH_TIMEOUT_THRESHOLD} seconds. We recommend a timeout of less than ` + + `${HIGH_TIMEOUT_THRESHOLD} seconds.`, + ); + } + // Initialization promise was created by a previous call to waitForInitialization. if (this.initializedPromise) { - return this.initializedPromise; + // This promise may already be resolved/rejected, but it doesn't hurt to wrap it in a timeout. + return this.clientWithTimeout(this.initializedPromise, options?.timeout, this.logger); } // Initialization completed before waitForInitialization was called, so we have completed // and there was no promise. So we make a resolved promise and return it. if (this.initState === InitState.Initialized) { this.initializedPromise = Promise.resolve(this); + // Already initialized, no need to timeout. return this.initializedPromise; } // Initialization failed before waitForInitialization was called, so we have completed // and there was no promise. So we make a rejected promise and return it. if (this.initState === InitState.Failed) { + // Already failed, no need to timeout. this.initializedPromise = Promise.reject(this.rejectionReason); return this.initializedPromise; } @@ -263,7 +285,7 @@ export default class LDClientImpl implements LDClient { this.initReject = reject; }); } - return this.initializedPromise; + return this.clientWithTimeout(this.initializedPromise, options?.timeout, this.logger); } variation( @@ -738,4 +760,33 @@ export default class LDClientImpl implements LDClient { this.onReady(); } } + + /** + * Apply a timeout promise to a base promise. This is for use with waitForInitialization. + * Currently it returns a LDClient. In the future it should return a status. + * + * The client isn't always the expected type of the consumer. It returns an LDClient interface + * which is less capable than, for example, the node client interface. + * + * @param basePromise The promise to race against a timeout. + * @param timeout The timeout in seconds. + * @param logger A logger to log when the timeout expires. + * @returns + */ + private clientWithTimeout( + basePromise: Promise, + timeout?: number, + logger?: LDLogger, + ): Promise { + if (timeout) { + const timeoutPromise = timedPromise(timeout, 'waitForInitialization'); + return Promise.race([basePromise, timeoutPromise.then(() => this)]).catch((reason) => { + if (reason instanceof LDTimeoutError) { + logger?.error(reason.message); + } + throw reason; + }); + } + return basePromise; + } } diff --git a/packages/shared/sdk-server/src/api/LDClient.ts b/packages/shared/sdk-server/src/api/LDClient.ts index 177727c44d..7635d5b7d6 100644 --- a/packages/shared/sdk-server/src/api/LDClient.ts +++ b/packages/shared/sdk-server/src/api/LDClient.ts @@ -9,6 +9,7 @@ import { LDMigrationOpEvent, LDMigrationVariation } from './data'; import { LDFlagsState } from './data/LDFlagsState'; import { LDFlagsStateOptions } from './data/LDFlagsStateOptions'; import { LDMigrationStage } from './data/LDMigrationStage'; +import { LDWaitForInitializationOptions } from './LDWaitForInitializationOptions'; /** * The LaunchDarkly SDK client object. @@ -44,24 +45,32 @@ export interface LDClient { * Note that you can also use event listeners ({@link on}) for the same purpose: the event * `"ready"` indicates success, and `"failed"` indicates failure. * - * There is no built-in timeout for this method. If you want your code to stop waiting on the - * Promise after some amount of time, you could use - * {@link https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/Promise/race|`Promise.race()`} - * or one of the several NPM helper packages that provides a standard mechanism for this. + * This method takes an optional parameters which include a timeout. The timeout controls how long + * a specific call to waitForInitialization will wait before rejecting its promise. If a + * subsequent call is made to waitForInitialization with a timeout, then it will again wait up to + * that maximum time. + * * Regardless of whether you continue to wait, the SDK will still retry all connection failures * indefinitely unless it gets an unrecoverable error as described above. * + * Waiting indefinitely, or depending only on the "ready" or "failed" events can result in an + * application waiting indefinitely. It is recommended to use a timeout which is reasonable + * for your application. + * + * @param options Options which control the behavior of `waitForInitialization`. + * * @returns - * A Promise that will be resolved if the client initializes successfully, or rejected if it - * fails. If successful, the result is the same client object. + * A Promise that will be resolved if the client initializes successfully, or rejected if it + * fails. If successful, the result is the same client object. It is not recommended to use the + * returned client object. It will be removed in a future version. * * @example * This example shows use of Promise chaining methods for specifying handlers: * ```javascript - * client.waitForInitialization().then(() => { + * client.waitForInitialization({timeoutSeconds: 10}).then(() => { * // do whatever is appropriate if initialization has succeeded * }).catch(err => { - * // do whatever is appropriate if initialization has failed + * // do whatever is appropriate if initialization has failed or timed out * }) * ``` * @@ -69,14 +78,14 @@ export interface LDClient { * This example shows use of `async`/`await` syntax for specifying handlers: * ```javascript * try { - * await client.waitForInitialization(); + * await client.waitForInitialization({timeoutSeconds: 10}); * // do whatever is appropriate if initialization has succeeded * } catch (err) { - * // do whatever is appropriate if initialization has failed + * // do whatever is appropriate if initialization has failed or timed out * } * ``` */ - waitForInitialization(): Promise; + waitForInitialization(options: LDWaitForInitializationOptions): Promise; /** * Determines the variation of a feature flag for a context. diff --git a/packages/shared/sdk-server/src/api/LDWaitForInitializationOptions.ts b/packages/shared/sdk-server/src/api/LDWaitForInitializationOptions.ts new file mode 100644 index 0000000000..d8ebb249af --- /dev/null +++ b/packages/shared/sdk-server/src/api/LDWaitForInitializationOptions.ts @@ -0,0 +1,18 @@ +/** + * Options for the waitForInitialization method. + */ +export interface LDWaitForInitializationOptions { + /** + * The amount of time, in seconds, to wait for initialization before rejecting the promise. + * + * If no options are specified on the `waitForInitialization`, then the promise will resolve + * only when initialization completes successfully or encounters a failure. + * + * Using a high timeout, or no timeout, is not recommended because it could result in a long + * delay when conditions prevent successful initialization. + * + * A value of 0 will cause the promise to resolve without waiting. In that scenario it would be + * more effective to not call `waitForInitialization`. + */ + timeout: number; +} diff --git a/packages/shared/sdk-server/src/api/index.ts b/packages/shared/sdk-server/src/api/index.ts index 41e35f8bb3..d3ff930cf7 100644 --- a/packages/shared/sdk-server/src/api/index.ts +++ b/packages/shared/sdk-server/src/api/index.ts @@ -4,6 +4,7 @@ export * from './LDClient'; export * from './LDMigration'; export * from './interfaces/DataKind'; export * from './subsystems/LDFeatureStore'; +export * from './LDWaitForInitializationOptions'; // These are items that should be less frequently used, and therefore they // are namespaced to reduce clutter amongst the top level exports. From 08e36559834adfdec195f68161008cac9e1edfe1 Mon Sep 17 00:00:00 2001 From: Ryan Lamb <4955475+kinyoklion@users.noreply.github.com> Date: Fri, 12 Apr 2024 09:28:43 -0700 Subject: [PATCH 2/7] feat: Add optional timeout to waitForInitialization. --- .../sdk-server/__tests__/LDClientImpl.test.ts | 36 ++++++++++++++++++- 1 file changed, 35 insertions(+), 1 deletion(-) diff --git a/packages/shared/sdk-server/__tests__/LDClientImpl.test.ts b/packages/shared/sdk-server/__tests__/LDClientImpl.test.ts index e34ce40479..e4ed8855cd 100644 --- a/packages/shared/sdk-server/__tests__/LDClientImpl.test.ts +++ b/packages/shared/sdk-server/__tests__/LDClientImpl.test.ts @@ -5,6 +5,7 @@ import { } from '@launchdarkly/private-js-mocks'; import { LDClientImpl, LDOptions } from '../src'; +import TestLogger, { LogLevel } from './Logger'; jest.mock('@launchdarkly/js-sdk-common', () => { const actual = jest.requireActual('@launchdarkly/js-sdk-common'); @@ -143,8 +144,41 @@ describe('LDClientImpl', () => { it('rejects the returned promise when initialization does not complete within the timeout', async () => { setupMockStreamingProcessor(undefined, undefined, undefined, undefined, undefined, 10000); client = createClient(); - await expect(client.waitForInitialization({ timeout: 1 })).rejects.toThrow( + await expect(async () => client.waitForInitialization({ timeout: 1 })).rejects.toThrow( 'waitForInitialization timed out after 1 seconds.', ); }); + + it('does not reject the returned promise when initialization completes within the timeout', async () => { + setupMockStreamingProcessor(undefined, undefined, undefined, undefined, undefined, 1000); + client = createClient(); + await expect(async () => client.waitForInitialization({ timeout: 5 })).not.toThrow(); + }); + + it('logs when no timeout is set', async () => { + const logger = new TestLogger(); + client = createClient({ logger }); + await client.waitForInitialization(); + logger.expectMessages([ + { + level: LogLevel.Warn, + matches: + /The waitForInitialization function was called without a timeout specified. In a future version a default timeout will be applied./, + }, + ]); + }); + + it('logs when the timeout is too high', async () => { + const logger = new TestLogger(); + client = createClient({ logger }); + await client.waitForInitialization({ timeout: Number.MAX_SAFE_INTEGER }); + + logger.expectMessages([ + { + level: LogLevel.Warn, + matches: + /The waitForInitialization function was called with a timeout greater than 60 seconds. We recommend a timeout of less than 60 seconds./, + }, + ]); + }); }); From d6d707886bb4119eccb9f12c4bc9b4dc7e381e1a Mon Sep 17 00:00:00 2001 From: Ryan Lamb <4955475+kinyoklion@users.noreply.github.com> Date: Fri, 12 Apr 2024 09:34:57 -0700 Subject: [PATCH 3/7] Add more tests. --- .../shared/sdk-server/__tests__/LDClientImpl.test.ts | 10 ++++++++++ 1 file changed, 10 insertions(+) diff --git a/packages/shared/sdk-server/__tests__/LDClientImpl.test.ts b/packages/shared/sdk-server/__tests__/LDClientImpl.test.ts index e4ed8855cd..0c1259ae13 100644 --- a/packages/shared/sdk-server/__tests__/LDClientImpl.test.ts +++ b/packages/shared/sdk-server/__tests__/LDClientImpl.test.ts @@ -181,4 +181,14 @@ describe('LDClientImpl', () => { }, ]); }); + + it.each([1, 30, 59])( + 'does not log when timeout is under high timeout threshold', + async (timeout) => { + const logger = new TestLogger(); + client = createClient({ logger }); + await client.waitForInitialization({ timeout }); + expect(logger.getCount(LogLevel.Warn)).toBe(0); + }, + ); }); From eeb4ee084d129632566d2571d3ed7604121a926b Mon Sep 17 00:00:00 2001 From: Ryan Lamb <4955475+kinyoklion@users.noreply.github.com> Date: Fri, 12 Apr 2024 09:40:34 -0700 Subject: [PATCH 4/7] Make options optional in the interface. --- packages/shared/sdk-server/src/api/LDClient.ts | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/packages/shared/sdk-server/src/api/LDClient.ts b/packages/shared/sdk-server/src/api/LDClient.ts index 627c3aeed5..221d762f8f 100644 --- a/packages/shared/sdk-server/src/api/LDClient.ts +++ b/packages/shared/sdk-server/src/api/LDClient.ts @@ -86,7 +86,7 @@ export interface LDClient { * } * ``` */ - waitForInitialization(options: LDWaitForInitializationOptions): Promise; + waitForInitialization(options?: LDWaitForInitializationOptions): Promise; /** * Determines the variation of a feature flag for a context. From 2a2924f7126c7cf91ddc393c7b861c6b2e7b58ce Mon Sep 17 00:00:00 2001 From: Ryan Lamb <4955475+kinyoklion@users.noreply.github.com> Date: Fri, 12 Apr 2024 09:54:31 -0700 Subject: [PATCH 5/7] Update for Offline/ldd. --- contract-tests/sdkClientEntity.js | 5 +- .../LDClientNode.bigSegments.test.ts | 4 +- .../LDClientNode.fileDataSource.test.ts | 6 +- .../__tests__/LDClientNode.proxy.test.ts | 6 +- .../__tests__/LDClientNode.tls.test.ts | 6 +- .../sdk-server-edge/src/api/LDClient.test.ts | 2 +- .../__tests__/LDClient.allFlags.test.ts | 2 +- .../__tests__/LDClient.evaluation.test.ts | 6 +- .../__tests__/LDClient.events.test.ts | 2 +- .../__tests__/LDClient.hooks.test.ts | 4 +- .../__tests__/LDClient.migrations.test.ts | 2 +- .../LDClientImpl.bigSegments.test.ts | 8 +-- .../sdk-server/__tests__/LDClientImpl.test.ts | 56 ++++++++++++++----- .../sdk-server/__tests__/Migration.test.ts | 2 +- .../__tests__/MigrationOpEvent.test.ts | 2 +- .../shared/sdk-server/src/LDClientImpl.ts | 11 +++- 16 files changed, 78 insertions(+), 46 deletions(-) diff --git a/contract-tests/sdkClientEntity.js b/contract-tests/sdkClientEntity.js index 0734ba5829..be6620c542 100644 --- a/contract-tests/sdkClientEntity.js +++ b/contract-tests/sdkClientEntity.js @@ -109,10 +109,7 @@ export async function newSdkClientEntity(options) { makeSdkConfig(options.configuration, options.tag), ); try { - await Promise.race([ - client.waitForInitialization(), - new Promise((resolve) => setTimeout(resolve, timeout)), - ]); + await client.waitForInitialization({timeout: timeout}); } catch (_) { // if waitForInitialization() rejects, the client failed to initialize, see next line } diff --git a/packages/sdk/server-node/__tests__/LDClientNode.bigSegments.test.ts b/packages/sdk/server-node/__tests__/LDClientNode.bigSegments.test.ts index 63a939e256..13ef44f57d 100644 --- a/packages/sdk/server-node/__tests__/LDClientNode.bigSegments.test.ts +++ b/packages/sdk/server-node/__tests__/LDClientNode.bigSegments.test.ts @@ -93,7 +93,7 @@ describe('given test data with big segments', () => { bigSegments: bigSegmentsConfig, }); - await client.waitForInitialization(); + await client.waitForInitialization({ timeout: 10 }); }); it('can require status', async () => { @@ -135,7 +135,7 @@ describe('given test data with big segments', () => { logger, }); - await client.waitForInitialization(); + await client.waitForInitialization({ timeout: 10 }); }); it('Can observe the status change', (done) => { diff --git a/packages/sdk/server-node/__tests__/LDClientNode.fileDataSource.test.ts b/packages/sdk/server-node/__tests__/LDClientNode.fileDataSource.test.ts index d85dc1d35f..f99d40f71c 100644 --- a/packages/sdk/server-node/__tests__/LDClientNode.fileDataSource.test.ts +++ b/packages/sdk/server-node/__tests__/LDClientNode.fileDataSource.test.ts @@ -97,7 +97,7 @@ describe('When using a file data source', () => { sendEvents: false, }); - await client.waitForInitialization(); + await client.waitForInitialization({ timeout: 10 }); const f1Var = await client.variation(flag1Key, { key: 'user1' }, 'default'); expect(f1Var).toEqual('off'); @@ -121,7 +121,7 @@ describe('When using a file data source', () => { sendEvents: false, }); - await client.waitForInitialization(); + await client.waitForInitialization({ timeout: 10 }); const f1Var = await client.variation(flag1Key, { key: 'user1' }, 'default'); expect(f1Var).toEqual('off'); @@ -143,7 +143,7 @@ describe('When using a file data source', () => { sendEvents: false, }); - await client.waitForInitialization(); + await client.waitForInitialization({ timeout: 10 }); const f1Var = await client.variation(flag1Key, { key: 'user1' }, 'default'); expect(f1Var).toEqual('off'); diff --git a/packages/sdk/server-node/__tests__/LDClientNode.proxy.test.ts b/packages/sdk/server-node/__tests__/LDClientNode.proxy.test.ts index b3865b6c33..771d5beb92 100644 --- a/packages/sdk/server-node/__tests__/LDClientNode.proxy.test.ts +++ b/packages/sdk/server-node/__tests__/LDClientNode.proxy.test.ts @@ -53,7 +53,7 @@ describe('When using a proxy', () => { closeable.push(proxy, server, client); - await client.waitForInitialization(); + await client.waitForInitialization({ timeout: 10 }); expect(client.initialized()).toBe(true); // If the proxy server did not log a request then the SDK did not actually use the proxy @@ -81,7 +81,7 @@ describe('When using a proxy', () => { closeable.push(proxy, server, events, client); - await client.waitForInitialization(); + await client.waitForInitialization({ timeout: 10 }); expect(client.initialized()).toBe(true); // If the proxy server did not log a request then the SDK did not actually use the proxy @@ -110,7 +110,7 @@ describe('When using a proxy', () => { closeable.push(proxy, pollingServer, eventsServer, client); - await client.waitForInitialization(); + await client.waitForInitialization({ timeout: 10 }); expect(client.initialized()).toBe(true); // If the proxy server did not log a request then the SDK did not actually use the proxy diff --git a/packages/sdk/server-node/__tests__/LDClientNode.tls.test.ts b/packages/sdk/server-node/__tests__/LDClientNode.tls.test.ts index 8c6baba968..3d68ce9a9a 100644 --- a/packages/sdk/server-node/__tests__/LDClientNode.tls.test.ts +++ b/packages/sdk/server-node/__tests__/LDClientNode.tls.test.ts @@ -27,7 +27,7 @@ describe('When using a TLS connection', () => { tlsParams: { ca: server.certificate }, diagnosticOptOut: true, }); - await client.waitForInitialization(); + await client.waitForInitialization({ timeout: 10 }); }); it('cannot connect via HTTPS to a server with a self-signed certificate, using default config', async () => { @@ -67,7 +67,7 @@ describe('When using a TLS connection', () => { }); // this won't return until the stream receives the "put" event - await client.waitForInitialization(); + await client.waitForInitialization({ timeout: 10 }); events.close(); }); @@ -85,7 +85,7 @@ describe('When using a TLS connection', () => { logger, }); - await client.waitForInitialization(); + await client.waitForInitialization({ timeout: 10 }); client.identify({ key: 'user' }); await client.flush(); diff --git a/packages/shared/sdk-server-edge/src/api/LDClient.test.ts b/packages/shared/sdk-server-edge/src/api/LDClient.test.ts index fe7949811d..6b7465e902 100644 --- a/packages/shared/sdk-server-edge/src/api/LDClient.test.ts +++ b/packages/shared/sdk-server-edge/src/api/LDClient.test.ts @@ -23,7 +23,7 @@ describe('Edge LDClient', () => { const client = new LDClient('client-side-id', basicPlatform.info, { sendEvents: true, }); - await client.waitForInitialization(); + await client.waitForInitialization({ timeout: 10 }); const passedConfig = mockEventProcessor.mock.calls[0][0]; expect(passedConfig).toMatchObject({ diff --git a/packages/shared/sdk-server/__tests__/LDClient.allFlags.test.ts b/packages/shared/sdk-server/__tests__/LDClient.allFlags.test.ts index 681d7c9a31..097839c424 100644 --- a/packages/shared/sdk-server/__tests__/LDClient.allFlags.test.ts +++ b/packages/shared/sdk-server/__tests__/LDClient.allFlags.test.ts @@ -26,7 +26,7 @@ describe('given an LDClient with test data', () => { makeCallbacks(true), ); - await client.waitForInitialization(); + await client.waitForInitialization({ timeout: 10 }); }); afterEach(() => { diff --git a/packages/shared/sdk-server/__tests__/LDClient.evaluation.test.ts b/packages/shared/sdk-server/__tests__/LDClient.evaluation.test.ts index 16b5aa4633..06f84cf3b5 100644 --- a/packages/shared/sdk-server/__tests__/LDClient.evaluation.test.ts +++ b/packages/shared/sdk-server/__tests__/LDClient.evaluation.test.ts @@ -44,7 +44,7 @@ describe('given an LDClient with test data', () => { makeCallbacks(true), ); - await client.waitForInitialization(); + await client.waitForInitialization({ timeout: 10 }); }); afterEach(() => { @@ -298,7 +298,7 @@ describe('given an offline client', () => { }); it('returns the default value for variation', async () => { - await client.waitForInitialization(); + await client.waitForInitialization({ timeout: 10 }); td.update(td.flag('flagkey').variations('value').variationForAll(0)); const result = await client.variation('flagkey', defaultUser, 'default'); expect(result).toEqual('default'); @@ -306,7 +306,7 @@ describe('given an offline client', () => { }); it('returns the default value for variationDetail', async () => { - await client.waitForInitialization(); + await client.waitForInitialization({ timeout: 10 }); td.update(td.flag('flagkey').variations('value').variationForAll(0)); const result = await client.variationDetail('flagkey', defaultUser, 'default'); expect(result).toMatchObject({ diff --git a/packages/shared/sdk-server/__tests__/LDClient.events.test.ts b/packages/shared/sdk-server/__tests__/LDClient.events.test.ts index 603b44e60b..3a09d688af 100644 --- a/packages/shared/sdk-server/__tests__/LDClient.events.test.ts +++ b/packages/shared/sdk-server/__tests__/LDClient.events.test.ts @@ -33,7 +33,7 @@ describe('given a client with mock event processor', () => { }, makeCallbacks(false), ); - await client.waitForInitialization(); + await client.waitForInitialization({ timeout: 10 }); }); afterEach(() => { diff --git a/packages/shared/sdk-server/__tests__/LDClient.hooks.test.ts b/packages/shared/sdk-server/__tests__/LDClient.hooks.test.ts index efee536ab8..d676fed2ec 100644 --- a/packages/shared/sdk-server/__tests__/LDClient.hooks.test.ts +++ b/packages/shared/sdk-server/__tests__/LDClient.hooks.test.ts @@ -31,7 +31,7 @@ describe('given an LDClient with test data', () => { makeCallbacks(true), ); - await client.waitForInitialization(); + await client.waitForInitialization({ timeout: 10 }); }); afterEach(() => { @@ -361,7 +361,7 @@ it('can add a hook after initialization', async () => { makeCallbacks(true), ); - await client.waitForInitialization(); + await client.waitForInitialization({ timeout: 10 }); td.update(td.flag('flagKey').booleanFlag().on(true)); const testHook = new TestHook(); diff --git a/packages/shared/sdk-server/__tests__/LDClient.migrations.test.ts b/packages/shared/sdk-server/__tests__/LDClient.migrations.test.ts index 5775c71697..c8d1a6de44 100644 --- a/packages/shared/sdk-server/__tests__/LDClient.migrations.test.ts +++ b/packages/shared/sdk-server/__tests__/LDClient.migrations.test.ts @@ -42,7 +42,7 @@ describe('given an LDClient with test data', () => { callbacks, ); - await client.waitForInitialization(); + await client.waitForInitialization({ timeout: 10 }); }); afterEach(() => { diff --git a/packages/shared/sdk-server/__tests__/LDClientImpl.bigSegments.test.ts b/packages/shared/sdk-server/__tests__/LDClientImpl.bigSegments.test.ts index 71af891e15..b457e0b526 100644 --- a/packages/shared/sdk-server/__tests__/LDClientImpl.bigSegments.test.ts +++ b/packages/shared/sdk-server/__tests__/LDClientImpl.bigSegments.test.ts @@ -85,7 +85,7 @@ describe('given test data with big segments', () => { makeCallbacks(true), ); - await client.waitForInitialization(); + await client.waitForInitialization({ timeout: 10 }); }); afterEach(() => { @@ -124,7 +124,7 @@ describe('given test data with big segments', () => { makeCallbacks(true), ); - await client.waitForInitialization(); + await client.waitForInitialization({ timeout: 10 }); }); afterEach(() => { @@ -163,7 +163,7 @@ describe('given test data with big segments', () => { makeCallbacks(true), ); - await client.waitForInitialization(); + await client.waitForInitialization({ timeout: 10 }); }); afterEach(() => { @@ -189,7 +189,7 @@ describe('given test data with big segments', () => { makeCallbacks(true), ); - await client.waitForInitialization(); + await client.waitForInitialization({ timeout: 10 }); }); afterEach(() => { diff --git a/packages/shared/sdk-server/__tests__/LDClientImpl.test.ts b/packages/shared/sdk-server/__tests__/LDClientImpl.test.ts index 0c1259ae13..3be4ac1488 100644 --- a/packages/shared/sdk-server/__tests__/LDClientImpl.test.ts +++ b/packages/shared/sdk-server/__tests__/LDClientImpl.test.ts @@ -43,7 +43,7 @@ describe('LDClientImpl', () => { it('fires ready event in online mode', async () => { client = createClient(); - const initializedClient = await client.waitForInitialization(); + const initializedClient = await client.waitForInitialization({ timeout: 10 }); expect(initializedClient).toEqual(client); expect(client.initialized()).toBeTruthy(); @@ -57,7 +57,7 @@ describe('LDClientImpl', () => { client = createClient(); setTimeout(async () => { - const initializedClient = await client.waitForInitialization(); + const initializedClient = await client.waitForInitialization({ timeout: 10 }); expect(initializedClient).toEqual(client); done(); }, 10); @@ -65,15 +65,15 @@ describe('LDClientImpl', () => { it('waiting for initialization the second time produces the same result', async () => { client = createClient(); - await client.waitForInitialization(); + await client.waitForInitialization({ timeout: 10 }); - const initializedClient = await client.waitForInitialization(); + const initializedClient = await client.waitForInitialization({ timeout: 10 }); expect(initializedClient).toEqual(client); }); it('fires ready event in offline mode', async () => { client = createClient({ offline: true }); - const initializedClient = await client.waitForInitialization(); + const initializedClient = await client.waitForInitialization({ timeout: 10 }); expect(initializedClient).toEqual(client); expect(client.initialized()).toBeTruthy(); @@ -86,7 +86,7 @@ describe('LDClientImpl', () => { setupMockStreamingProcessor(true); client = createClient(); - await expect(client.waitForInitialization()).rejects.toThrow('failed'); + await expect(client.waitForInitialization({ timeout: 10 })).rejects.toThrow('failed'); expect(client.initialized()).toBeFalsy(); expect(callbacks.onReady).not.toBeCalled(); @@ -99,7 +99,7 @@ describe('LDClientImpl', () => { client = createClient(); setTimeout(async () => { - await expect(client.waitForInitialization()).rejects.toThrow('failed'); + await expect(client.waitForInitialization({ timeout: 10 })).rejects.toThrow('failed'); expect(client.initialized()).toBeFalsy(); expect(callbacks.onReady).not.toBeCalled(); @@ -113,8 +113,8 @@ describe('LDClientImpl', () => { setupMockStreamingProcessor(true); client = createClient(); - await expect(client.waitForInitialization()).rejects.toThrow('failed'); - await expect(client.waitForInitialization()).rejects.toThrow('failed'); + await expect(client.waitForInitialization({ timeout: 10 })).rejects.toThrow('failed'); + await expect(client.waitForInitialization({ timeout: 10 })).rejects.toThrow('failed'); }); it('isOffline returns true in offline mode', () => { @@ -129,14 +129,14 @@ describe('LDClientImpl', () => { it('resolves immediately if the client is already ready', async () => { client = createClient(); - await client.waitForInitialization(); - await client.waitForInitialization(); + await client.waitForInitialization({ timeout: 10 }); + await client.waitForInitialization({ timeout: 10 }); }); it('creates only one Promise when waiting for initialization - when not using a timeout', async () => { client = createClient(); - const p1 = client.waitForInitialization(); - const p2 = client.waitForInitialization(); + const p1 = client.waitForInitialization({ timeout: 10 }); + const p2 = client.waitForInitialization({ timeout: 10 }); expect(p2).toBe(p1); }); @@ -158,7 +158,7 @@ describe('LDClientImpl', () => { it('logs when no timeout is set', async () => { const logger = new TestLogger(); client = createClient({ logger }); - await client.waitForInitialization(); + await client.waitForInitialization({ timeout: 10 }); logger.expectMessages([ { level: LogLevel.Warn, @@ -191,4 +191,32 @@ describe('LDClientImpl', () => { expect(logger.getCount(LogLevel.Warn)).toBe(0); }, ); + + it('does not log when offline and no timeout it set', async () => { + const logger = new TestLogger(); + client = createClient({ logger, offline: true }); + await client.waitForInitialization({ timeout: 10 }); + expect(logger.getCount(LogLevel.Warn)).toBe(0); + }); + + it('does not log when the timeout is too high and client is offline', async () => { + const logger = new TestLogger(); + client = createClient({ logger, offline: true }); + await client.waitForInitialization({ timeout: Number.MAX_SAFE_INTEGER }); + expect(logger.getCount(LogLevel.Warn)).toBe(0); + }); + + it('does not log when useLdd is true and no timeout it set', async () => { + const logger = new TestLogger(); + client = createClient({ logger, offline: true }); + await client.waitForInitialization({ timeout: 10 }); + expect(logger.getCount(LogLevel.Warn)).toBe(0); + }); + + it('does not log when useLdd is true and the timeout is too long', async () => { + const logger = new TestLogger(); + client = createClient({ logger, offline: true }); + await client.waitForInitialization({ timeout: Number.MAX_SAFE_INTEGER }); + expect(logger.getCount(LogLevel.Warn)).toBe(0); + }); }); diff --git a/packages/shared/sdk-server/__tests__/Migration.test.ts b/packages/shared/sdk-server/__tests__/Migration.test.ts index ca1ed82846..c8d24a759f 100644 --- a/packages/shared/sdk-server/__tests__/Migration.test.ts +++ b/packages/shared/sdk-server/__tests__/Migration.test.ts @@ -30,7 +30,7 @@ describe('given an LDClient with test data', () => { callbacks, ); - await client.waitForInitialization(); + await client.waitForInitialization({ timeout: 10 }); }); afterEach(() => { diff --git a/packages/shared/sdk-server/__tests__/MigrationOpEvent.test.ts b/packages/shared/sdk-server/__tests__/MigrationOpEvent.test.ts index f1eb9d88fa..faa5820b7a 100644 --- a/packages/shared/sdk-server/__tests__/MigrationOpEvent.test.ts +++ b/packages/shared/sdk-server/__tests__/MigrationOpEvent.test.ts @@ -51,7 +51,7 @@ describe('given an LDClient with test data', () => { callbacks, ); - await client.waitForInitialization(); + await client.waitForInitialization({ timeout: 10 }); }); afterEach(() => { diff --git a/packages/shared/sdk-server/src/LDClientImpl.ts b/packages/shared/sdk-server/src/LDClientImpl.ts index a6d18daea8..e740d9db1d 100644 --- a/packages/shared/sdk-server/src/LDClientImpl.ts +++ b/packages/shared/sdk-server/src/LDClientImpl.ts @@ -259,13 +259,20 @@ export default class LDClientImpl implements LDClient { // by the time the promise was rejected, then that would result in an unhandled promise // rejection. - if (options?.timeout === undefined) { + // If there is no update processor, then there is functionally no initialization + // so it is fine not to wait. + + if (options?.timeout === undefined && this.updateProcessor !== undefined) { this.logger?.warn( 'The waitForInitialization function was called without a timeout specified.' + ' In a future version a default timeout will be applied.', ); } - if (options?.timeout !== undefined && options?.timeout > HIGH_TIMEOUT_THRESHOLD) { + if ( + options?.timeout !== undefined && + options?.timeout > HIGH_TIMEOUT_THRESHOLD && + this.updateProcessor !== undefined + ) { this.logger?.warn( 'The waitForInitialization function was called with a timeout greater than ' + `${HIGH_TIMEOUT_THRESHOLD} seconds. We recommend a timeout of less than ` + From 816cc572315739dd22e09b7e8c3d24760a3dd9ed Mon Sep 17 00:00:00 2001 From: Ryan Lamb <4955475+kinyoklion@users.noreply.github.com> Date: Fri, 12 Apr 2024 09:59:09 -0700 Subject: [PATCH 6/7] Remove some timeouts. --- packages/shared/sdk-server/__tests__/LDClientImpl.test.ts | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/packages/shared/sdk-server/__tests__/LDClientImpl.test.ts b/packages/shared/sdk-server/__tests__/LDClientImpl.test.ts index 3be4ac1488..4fcf64e3e9 100644 --- a/packages/shared/sdk-server/__tests__/LDClientImpl.test.ts +++ b/packages/shared/sdk-server/__tests__/LDClientImpl.test.ts @@ -135,8 +135,8 @@ describe('LDClientImpl', () => { it('creates only one Promise when waiting for initialization - when not using a timeout', async () => { client = createClient(); - const p1 = client.waitForInitialization({ timeout: 10 }); - const p2 = client.waitForInitialization({ timeout: 10 }); + const p1 = client.waitForInitialization(); + const p2 = client.waitForInitialization(); expect(p2).toBe(p1); }); @@ -158,7 +158,7 @@ describe('LDClientImpl', () => { it('logs when no timeout is set', async () => { const logger = new TestLogger(); client = createClient({ logger }); - await client.waitForInitialization({ timeout: 10 }); + await client.waitForInitialization(); logger.expectMessages([ { level: LogLevel.Warn, From bf9dd70eb3e174b5a81000ea69072715ef889298 Mon Sep 17 00:00:00 2001 From: Ryan Lamb <4955475+kinyoklion@users.noreply.github.com> Date: Mon, 15 Apr 2024 13:20:49 -0700 Subject: [PATCH 7/7] Add test which verifies the timeout error was logged. --- .../sdk-server/__tests__/LDClientImpl.test.ts | 17 +++++++++++++++++ 1 file changed, 17 insertions(+) diff --git a/packages/shared/sdk-server/__tests__/LDClientImpl.test.ts b/packages/shared/sdk-server/__tests__/LDClientImpl.test.ts index 4fcf64e3e9..20ea30de2b 100644 --- a/packages/shared/sdk-server/__tests__/LDClientImpl.test.ts +++ b/packages/shared/sdk-server/__tests__/LDClientImpl.test.ts @@ -149,6 +149,23 @@ describe('LDClientImpl', () => { ); }); + it('logs an error when the initialization does not complete within the timeout', async () => { + setupMockStreamingProcessor(undefined, undefined, undefined, undefined, undefined, 10000); + const logger = new TestLogger(); + client = createClient({ logger }); + try { + await client.waitForInitialization({ timeout: 1 }); + } catch { + // Not being tested in this test. + } + logger.expectMessages([ + { + level: LogLevel.Error, + matches: /waitForInitialization timed out after 1 seconds./, + }, + ]); + }); + it('does not reject the returned promise when initialization completes within the timeout', async () => { setupMockStreamingProcessor(undefined, undefined, undefined, undefined, undefined, 1000); client = createClient();