From 5ec18c0fab702b4214295da45b2ddaa3fd481102 Mon Sep 17 00:00:00 2001 From: Yusinto Ngadiman Date: Tue, 2 Apr 2024 14:28:51 -0700 Subject: [PATCH 01/15] chore: Added identify options. --- packages/shared/sdk-client/src/LDClientImpl.ts | 16 +++++++++++++++- .../sdk-client/src/api/LDIdentifyOptions.ts | 6 ++++++ 2 files changed, 21 insertions(+), 1 deletion(-) create mode 100644 packages/shared/sdk-client/src/api/LDIdentifyOptions.ts diff --git a/packages/shared/sdk-client/src/LDClientImpl.ts b/packages/shared/sdk-client/src/LDClientImpl.ts index 57efc8bd4a..149e745118 100644 --- a/packages/shared/sdk-client/src/LDClientImpl.ts +++ b/packages/shared/sdk-client/src/LDClientImpl.ts @@ -19,6 +19,7 @@ import { import { ConnectionMode, LDClient, type LDOptions } from './api'; import LDEmitter, { EventName } from './api/LDEmitter'; +import { LDIdentifyOptions } from './api/LDIdentifyOptions'; import Configuration from './configuration'; import createDiagnosticsManager from './diagnostics/createDiagnosticsManager'; import createEventProcessor from './events/createEventProcessor'; @@ -29,6 +30,10 @@ import { addAutoEnv, calculateFlagChanges, ensureKey } from './utils'; const { createErrorEvaluationDetail, createSuccessEvaluationDetail, ClientMessages, ErrorKinds } = internal; +const defaultIdentifyOptions: LDIdentifyOptions = { + timeoutSeconds: 15, +}; + export default class LDClientImpl implements LDClient { config: Configuration; context?: LDContext; @@ -269,7 +274,16 @@ export default class LDClientImpl implements LDClient { return f ? JSON.parse(f) : undefined; } - async identify(pristineContext: LDContext): Promise { + /** + * TODO: implement timeout. + * + * @param pristineContext + * @param _timeoutSeconds + */ + async identify( + pristineContext: LDContext, + { timeoutSeconds }: LDIdentifyOptions = defaultIdentifyOptions, + ): Promise { let context = await ensureKey(pristineContext, this.platform); if (this.autoEnvAttributes === AutoEnvAttributes.Enabled) { diff --git a/packages/shared/sdk-client/src/api/LDIdentifyOptions.ts b/packages/shared/sdk-client/src/api/LDIdentifyOptions.ts new file mode 100644 index 0000000000..9fa891590d --- /dev/null +++ b/packages/shared/sdk-client/src/api/LDIdentifyOptions.ts @@ -0,0 +1,6 @@ +export interface LDIdentifyOptions { + /** + * Identify timeout in seconds. Defaults to 15 seconds. + */ + timeoutSeconds: number; +} From f78ff8f82e1d544ac2aae0b7140124baccdd57e6 Mon Sep 17 00:00:00 2001 From: Yusinto Ngadiman Date: Tue, 2 Apr 2024 15:13:45 -0700 Subject: [PATCH 02/15] chore: Added timeout function. Added comments. --- packages/shared/common/src/utils/index.ts | 2 ++ packages/shared/common/src/utils/timeout.ts | 13 ++++++++++++ .../shared/sdk-client/src/LDClientImpl.ts | 21 ++++++++++++------- 3 files changed, 29 insertions(+), 7 deletions(-) create mode 100644 packages/shared/common/src/utils/timeout.ts diff --git a/packages/shared/common/src/utils/index.ts b/packages/shared/common/src/utils/index.ts index 2c2130fe37..fbbb63bed7 100644 --- a/packages/shared/common/src/utils/index.ts +++ b/packages/shared/common/src/utils/index.ts @@ -6,6 +6,7 @@ import fastDeepEqual from './fast-deep-equal'; import { base64UrlEncode, defaultHeaders, httpErrorMessage, LDHeaders, shouldRetry } from './http'; import noop from './noop'; import sleep from './sleep'; +import timeout from './timeout'; import { VoidFunction } from './VoidFunction'; export { @@ -21,5 +22,6 @@ export { shouldRetry, secondsToMillis, sleep, + timeout, VoidFunction, }; diff --git a/packages/shared/common/src/utils/timeout.ts b/packages/shared/common/src/utils/timeout.ts new file mode 100644 index 0000000000..ed46dda90c --- /dev/null +++ b/packages/shared/common/src/utils/timeout.ts @@ -0,0 +1,13 @@ +/** + * Returns a promise which errors after t seconds. + * + * @param t Timeout in seconds. + * @param error Throw this error when when time is up. Should be a meaningful + * description of what task has timed out to aid debugging. + */ +const timeout = (t: number, error?: string) => + new Promise((_res, reject) => { + setTimeout(() => reject(new Error(error ?? `Timed out after ${t} seconds.`)), t * 1000); + }); + +export default timeout; diff --git a/packages/shared/sdk-client/src/LDClientImpl.ts b/packages/shared/sdk-client/src/LDClientImpl.ts index 149e745118..e33b9d6489 100644 --- a/packages/shared/sdk-client/src/LDClientImpl.ts +++ b/packages/shared/sdk-client/src/LDClientImpl.ts @@ -14,6 +14,7 @@ import { Platform, ProcessStreamResponse, EventName as StreamEventName, + timeout, TypeValidators, } from '@launchdarkly/js-sdk-common'; @@ -242,9 +243,9 @@ export default class LDClientImpl implements LDClient { ); } - private createIdentifyPromise() { + private createIdentifyPromise(timeoutSeconds: number) { let res: any; - const p = new Promise((resolve, reject) => { + const slow = new Promise((resolve, reject) => { res = resolve; if (this.identifyChangeListener) { @@ -260,13 +261,16 @@ export default class LDClientImpl implements LDClient { this.identifyErrorListener = (c: LDContext, err: any) => { this.logger.debug(`error: ${err}, context: ${JSON.stringify(c)}`); reject(err); + f; }; this.emitter.on('change', this.identifyChangeListener); this.emitter.on('error', this.identifyErrorListener); }); + const timed = timeout(timeoutSeconds, 'identify timed out.'); + const raced = Promise.race([timed, slow]); - return { identifyPromise: p, identifyResolve: res }; + return { identifyPromise: raced, identifyResolve: res }; } private async getFlagsFromStorage(canonicalKey: string): Promise { @@ -275,10 +279,13 @@ export default class LDClientImpl implements LDClient { } /** - * TODO: implement timeout. + * Identifies a context to LaunchDarkly. See {@link LDClient.identify}. * - * @param pristineContext - * @param _timeoutSeconds + * @param pristineContext The LDContext object to be identified. + * @param LDIdentifyOptions You can specify additional options like a timeout when + * identifying. + * + * @returns {Promise}. */ async identify( pristineContext: LDContext, @@ -299,7 +306,7 @@ export default class LDClientImpl implements LDClient { } this.eventProcessor?.sendEvent(this.eventFactoryDefault.identifyEvent(checkedContext)); - const { identifyPromise, identifyResolve } = this.createIdentifyPromise(); + const { identifyPromise, identifyResolve } = this.createIdentifyPromise(timeoutSeconds); this.logger.debug(`Identifying ${JSON.stringify(context)}`); const flagsStorage = await this.getFlagsFromStorage(checkedContext.canonicalKey); From c9e5fb1536721bcc90a5408ece56b2542b95bb00 Mon Sep 17 00:00:00 2001 From: Yusinto Ngadiman Date: Tue, 2 Apr 2024 15:14:17 -0700 Subject: [PATCH 03/15] Update LDClientImpl.ts --- packages/shared/sdk-client/src/LDClientImpl.ts | 1 - 1 file changed, 1 deletion(-) diff --git a/packages/shared/sdk-client/src/LDClientImpl.ts b/packages/shared/sdk-client/src/LDClientImpl.ts index e33b9d6489..4b7c48b6ed 100644 --- a/packages/shared/sdk-client/src/LDClientImpl.ts +++ b/packages/shared/sdk-client/src/LDClientImpl.ts @@ -261,7 +261,6 @@ export default class LDClientImpl implements LDClient { this.identifyErrorListener = (c: LDContext, err: any) => { this.logger.debug(`error: ${err}, context: ${JSON.stringify(c)}`); reject(err); - f; }; this.emitter.on('change', this.identifyChangeListener); From 59f66a58dd9dc4cbd792456e9903ad9eae599ad2 Mon Sep 17 00:00:00 2001 From: Yusinto Ngadiman Date: Tue, 2 Apr 2024 18:54:24 -0700 Subject: [PATCH 04/15] chore: Added unit tests for timeout. --- .../shared/mocks/src/streamingProcessor.ts | 3 +- .../src/LDClientImpl.timeout.test.ts | 108 ++++++++++++++++++ 2 files changed, 110 insertions(+), 1 deletion(-) create mode 100644 packages/shared/sdk-client/src/LDClientImpl.timeout.test.ts diff --git a/packages/shared/mocks/src/streamingProcessor.ts b/packages/shared/mocks/src/streamingProcessor.ts index e8891a89ea..87ba783cd2 100644 --- a/packages/shared/mocks/src/streamingProcessor.ts +++ b/packages/shared/mocks/src/streamingProcessor.ts @@ -13,6 +13,7 @@ export const setupMockStreamingProcessor = ( putResponseJson: any = { data: { flags: {}, segments: {} } }, patchResponseJson?: any, deleteResponseJson?: any, + errorTimeoutSeconds: number = 0, ) => { MockStreamingProcessor.mockImplementation( ( @@ -33,7 +34,7 @@ export const setupMockStreamingProcessor = ( message: 'test-error', }; errorHandler(unauthorized); - }); + }, errorTimeoutSeconds * 1000); } else { // execute put which will resolve the identify promise setTimeout(() => listeners.get('put')?.processJson(putResponseJson)); diff --git a/packages/shared/sdk-client/src/LDClientImpl.timeout.test.ts b/packages/shared/sdk-client/src/LDClientImpl.timeout.test.ts new file mode 100644 index 0000000000..cc3bf646de --- /dev/null +++ b/packages/shared/sdk-client/src/LDClientImpl.timeout.test.ts @@ -0,0 +1,108 @@ +import { AutoEnvAttributes, clone, LDContext } from '@launchdarkly/js-sdk-common'; +import { basicPlatform, logger, setupMockStreamingProcessor } from '@launchdarkly/private-js-mocks'; + +import * as mockResponseJson from './evaluation/mockResponse.json'; +import LDClientImpl from './LDClientImpl'; +import { Flags } from './types'; +import { toMulti } from './utils/addAutoEnv'; + +jest.mock('@launchdarkly/js-sdk-common', () => { + const actual = jest.requireActual('@launchdarkly/js-sdk-common'); + const m = jest.requireActual('@launchdarkly/private-js-mocks'); + return { + ...actual, + ...{ + internal: { + ...actual.internal, + StreamingProcessor: m.MockStreamingProcessor, + }, + }, + }; +}); + +const testSdkKey = 'test-sdk-key'; +let ldc: LDClientImpl; +let defaultPutResponse: Flags; + +describe('sdk-client identify timeout', () => { + beforeAll(() => { + jest.useFakeTimers(); + }); + + beforeEach(() => { + defaultPutResponse = clone(mockResponseJson); + + // simulate streamer error after a long timeout + setupMockStreamingProcessor(true, defaultPutResponse, undefined, undefined, 30); + + ldc = new LDClientImpl(testSdkKey, AutoEnvAttributes.Enabled, basicPlatform, { + logger, + sendEvents: false, + }); + jest + .spyOn(LDClientImpl.prototype as any, 'createStreamUriPath') + .mockReturnValue('/stream/path'); + }); + + afterEach(() => { + jest.resetAllMocks(); + }); + + test('rejects with default timeout of 15s', async () => { + const carContext: LDContext = { kind: 'car', key: 'test-car' }; + jest.advanceTimersByTimeAsync(15000).then(); + await expect(ldc.identify(carContext)).rejects.toThrow(/identify timed out/); + }); + + test('rejects with custom timeout', async () => { + const timeoutSeconds = 5; + const carContext: LDContext = { kind: 'car', key: 'test-car' }; + + jest.advanceTimersByTimeAsync(timeoutSeconds * 1000).then(); + + await expect(ldc.identify(carContext, { timeoutSeconds })).rejects.toThrow( + /identify timed out/, + ); + }); + + test('resolves with default timeout', async () => { + const carContext: LDContext = { kind: 'car', key: 'test-car' }; + setupMockStreamingProcessor(false, defaultPutResponse, undefined, undefined); + jest.advanceTimersByTimeAsync(15 * 1000).then(); + + await expect(ldc.identify(carContext)).resolves.toBeUndefined(); + + expect(ldc.getContext()).toEqual(expect.objectContaining(toMulti(carContext))); + expect(ldc.allFlags()).toEqual({ + 'dev-test-flag': true, + 'easter-i-tunes-special': false, + 'easter-specials': 'no specials', + fdsafdsafdsafdsa: true, + 'log-level': 'warn', + 'moonshot-demo': true, + test1: 's1', + 'this-is-a-test': true, + }); + }); + + test('resolves with custom timeout', async () => { + const timeoutSeconds = 5; + const carContext: LDContext = { kind: 'car', key: 'test-car' }; + setupMockStreamingProcessor(false, defaultPutResponse, undefined, undefined); + jest.advanceTimersByTimeAsync(timeoutSeconds).then(); + + await expect(ldc.identify(carContext, { timeoutSeconds })).resolves.toBeUndefined(); + + expect(ldc.getContext()).toEqual(expect.objectContaining(toMulti(carContext))); + expect(ldc.allFlags()).toEqual({ + 'dev-test-flag': true, + 'easter-i-tunes-special': false, + 'easter-specials': 'no specials', + fdsafdsafdsafdsa: true, + 'log-level': 'warn', + 'moonshot-demo': true, + test1: 's1', + 'this-is-a-test': true, + }); + }); +}); From 31888fe452011a4cd19c8146b7e669a89319b4ba Mon Sep 17 00:00:00 2001 From: Yusinto Ngadiman Date: Tue, 2 Apr 2024 22:26:58 -0700 Subject: [PATCH 05/15] fix: Change default timeout to 5 seconds. Improved in-line comments. --- .../src/LDClientImpl.timeout.test.ts | 18 ++++++++++-------- packages/shared/sdk-client/src/LDClientImpl.ts | 4 ++-- packages/shared/sdk-client/src/api/LDClient.ts | 7 +++++-- .../sdk-client/src/api/LDIdentifyOptions.ts | 7 ++++++- 4 files changed, 23 insertions(+), 13 deletions(-) diff --git a/packages/shared/sdk-client/src/LDClientImpl.timeout.test.ts b/packages/shared/sdk-client/src/LDClientImpl.timeout.test.ts index cc3bf646de..b4f7ee7042 100644 --- a/packages/shared/sdk-client/src/LDClientImpl.timeout.test.ts +++ b/packages/shared/sdk-client/src/LDClientImpl.timeout.test.ts @@ -2,7 +2,7 @@ import { AutoEnvAttributes, clone, LDContext } from '@launchdarkly/js-sdk-common import { basicPlatform, logger, setupMockStreamingProcessor } from '@launchdarkly/private-js-mocks'; import * as mockResponseJson from './evaluation/mockResponse.json'; -import LDClientImpl from './LDClientImpl'; +import LDClientImpl, { defaultIdentifyOptions } from './LDClientImpl'; import { Flags } from './types'; import { toMulti } from './utils/addAutoEnv'; @@ -48,14 +48,16 @@ describe('sdk-client identify timeout', () => { jest.resetAllMocks(); }); - test('rejects with default timeout of 15s', async () => { + // streamer is setup to error in beforeEach hence causing the timeout + test('rejects with default timeout of 5s', async () => { const carContext: LDContext = { kind: 'car', key: 'test-car' }; - jest.advanceTimersByTimeAsync(15000).then(); + jest.advanceTimersByTimeAsync(defaultIdentifyOptions.timeoutSeconds * 1000).then(); await expect(ldc.identify(carContext)).rejects.toThrow(/identify timed out/); }); + // streamer is setup to error in beforeEach hence causing the timeout test('rejects with custom timeout', async () => { - const timeoutSeconds = 5; + const timeoutSeconds = 15; const carContext: LDContext = { kind: 'car', key: 'test-car' }; jest.advanceTimersByTimeAsync(timeoutSeconds * 1000).then(); @@ -67,8 +69,8 @@ describe('sdk-client identify timeout', () => { test('resolves with default timeout', async () => { const carContext: LDContext = { kind: 'car', key: 'test-car' }; - setupMockStreamingProcessor(false, defaultPutResponse, undefined, undefined); - jest.advanceTimersByTimeAsync(15 * 1000).then(); + setupMockStreamingProcessor(false, defaultPutResponse); + jest.advanceTimersByTimeAsync(defaultIdentifyOptions.timeoutSeconds * 1000).then(); await expect(ldc.identify(carContext)).resolves.toBeUndefined(); @@ -86,9 +88,9 @@ describe('sdk-client identify timeout', () => { }); test('resolves with custom timeout', async () => { - const timeoutSeconds = 5; + const timeoutSeconds = 15; const carContext: LDContext = { kind: 'car', key: 'test-car' }; - setupMockStreamingProcessor(false, defaultPutResponse, undefined, undefined); + setupMockStreamingProcessor(false, defaultPutResponse); jest.advanceTimersByTimeAsync(timeoutSeconds).then(); await expect(ldc.identify(carContext, { timeoutSeconds })).resolves.toBeUndefined(); diff --git a/packages/shared/sdk-client/src/LDClientImpl.ts b/packages/shared/sdk-client/src/LDClientImpl.ts index 4b7c48b6ed..4347f7d4e1 100644 --- a/packages/shared/sdk-client/src/LDClientImpl.ts +++ b/packages/shared/sdk-client/src/LDClientImpl.ts @@ -31,8 +31,8 @@ import { addAutoEnv, calculateFlagChanges, ensureKey } from './utils'; const { createErrorEvaluationDetail, createSuccessEvaluationDetail, ClientMessages, ErrorKinds } = internal; -const defaultIdentifyOptions: LDIdentifyOptions = { - timeoutSeconds: 15, +export const defaultIdentifyOptions: LDIdentifyOptions = { + timeoutSeconds: 5, }; export default class LDClientImpl implements LDClient { diff --git a/packages/shared/sdk-client/src/api/LDClient.ts b/packages/shared/sdk-client/src/api/LDClient.ts index 6e31aab3b2..98b3bd924e 100644 --- a/packages/shared/sdk-client/src/api/LDClient.ts +++ b/packages/shared/sdk-client/src/api/LDClient.ts @@ -8,6 +8,7 @@ import { } from '@launchdarkly/js-sdk-common'; import ConnectionMode from './ConnectionMode'; +import { LDIdentifyOptions } from './LDIdentifyOptions'; /** * The basic interface for the LaunchDarkly client. Platform-specific SDKs may add some methods of their own. @@ -107,11 +108,13 @@ export interface LDClient { * await the Promise to determine when the new flag values are available. * * @param context - * The LDContext object. + * The LDContext object. + * @param identifyOptions + * Optional configuration. Please see {@link LDIdentifyOptions}. * @returns * A Promise which resolves when the flag values for the specified context are available. */ - identify(context: LDContext): Promise; + identify(context: LDContext, identifyOptions?: LDIdentifyOptions): Promise; /** * Determines the json variation of a feature flag. diff --git a/packages/shared/sdk-client/src/api/LDIdentifyOptions.ts b/packages/shared/sdk-client/src/api/LDIdentifyOptions.ts index 9fa891590d..128088042f 100644 --- a/packages/shared/sdk-client/src/api/LDIdentifyOptions.ts +++ b/packages/shared/sdk-client/src/api/LDIdentifyOptions.ts @@ -1,6 +1,11 @@ export interface LDIdentifyOptions { /** - * Identify timeout in seconds. Defaults to 15 seconds. + * Determines when the identify promise resolves if no flags have been + * returned from the network. If you use a large timeout and await it, then + * any network delays will cause your application to wait a long time before + * continuing execution. + * + * Defaults to 5 seconds. */ timeoutSeconds: number; } From 59906a050185b2af162e96602c872308cfd403e7 Mon Sep 17 00:00:00 2001 From: Yusinto Ngadiman Date: Wed, 3 Apr 2024 00:00:42 -0700 Subject: [PATCH 06/15] chore: Add defaultIdentifyTimeout class variable so each sdk can specify its own value. Set rn default to 15s. --- .../react-native/src/ReactNativeLDClient.ts | 1 + .../src/internal/events/LDInternalOptions.ts | 7 +++++ .../src/LDClientImpl.timeout.test.ts | 10 +++---- .../shared/sdk-client/src/LDClientImpl.ts | 29 ++++++++++--------- 4 files changed, 29 insertions(+), 18 deletions(-) diff --git a/packages/sdk/react-native/src/ReactNativeLDClient.ts b/packages/sdk/react-native/src/ReactNativeLDClient.ts index 8c60490386..d3bfad59bf 100644 --- a/packages/sdk/react-native/src/ReactNativeLDClient.ts +++ b/packages/sdk/react-native/src/ReactNativeLDClient.ts @@ -47,6 +47,7 @@ export default class ReactNativeLDClient extends LDClientImpl { const internalOptions: internal.LDInternalOptions = { analyticsEventPath: `/mobile`, diagnosticEventPath: `/mobile/events/diagnostic`, + defaultIdentifyTimeout: 15, }; super( diff --git a/packages/shared/common/src/internal/events/LDInternalOptions.ts b/packages/shared/common/src/internal/events/LDInternalOptions.ts index 898197051a..69fa9786d0 100644 --- a/packages/shared/common/src/internal/events/LDInternalOptions.ts +++ b/packages/shared/common/src/internal/events/LDInternalOptions.ts @@ -12,4 +12,11 @@ export type LDInternalOptions = { analyticsEventPath?: string; diagnosticEventPath?: string; includeAuthorizationHeader?: boolean; + + /** + * Sets the default identify timeout in seconds. + * If not specified, this is set to 5 seconds in LDClientImpl.constructor. + * Mobile sdks should set this to 15 seconds. + */ + defaultIdentifyTimeout?: number; }; diff --git a/packages/shared/sdk-client/src/LDClientImpl.timeout.test.ts b/packages/shared/sdk-client/src/LDClientImpl.timeout.test.ts index b4f7ee7042..19e498b3b7 100644 --- a/packages/shared/sdk-client/src/LDClientImpl.timeout.test.ts +++ b/packages/shared/sdk-client/src/LDClientImpl.timeout.test.ts @@ -2,7 +2,7 @@ import { AutoEnvAttributes, clone, LDContext } from '@launchdarkly/js-sdk-common import { basicPlatform, logger, setupMockStreamingProcessor } from '@launchdarkly/private-js-mocks'; import * as mockResponseJson from './evaluation/mockResponse.json'; -import LDClientImpl, { defaultIdentifyOptions } from './LDClientImpl'; +import LDClientImpl from './LDClientImpl'; import { Flags } from './types'; import { toMulti } from './utils/addAutoEnv'; @@ -48,14 +48,14 @@ describe('sdk-client identify timeout', () => { jest.resetAllMocks(); }); - // streamer is setup to error in beforeEach hence causing the timeout + // streamer is setup to error in beforeEach to cause a timeout test('rejects with default timeout of 5s', async () => { const carContext: LDContext = { kind: 'car', key: 'test-car' }; - jest.advanceTimersByTimeAsync(defaultIdentifyOptions.timeoutSeconds * 1000).then(); + jest.advanceTimersByTimeAsync(ldc.defaultIdentifyTimeout * 1000).then(); await expect(ldc.identify(carContext)).rejects.toThrow(/identify timed out/); }); - // streamer is setup to error in beforeEach hence causing the timeout + // streamer is setup to error in beforeEach to cause a timeout test('rejects with custom timeout', async () => { const timeoutSeconds = 15; const carContext: LDContext = { kind: 'car', key: 'test-car' }; @@ -70,7 +70,7 @@ describe('sdk-client identify timeout', () => { test('resolves with default timeout', async () => { const carContext: LDContext = { kind: 'car', key: 'test-car' }; setupMockStreamingProcessor(false, defaultPutResponse); - jest.advanceTimersByTimeAsync(defaultIdentifyOptions.timeoutSeconds * 1000).then(); + jest.advanceTimersByTimeAsync(ldc.defaultIdentifyTimeout * 1000).then(); await expect(ldc.identify(carContext)).resolves.toBeUndefined(); diff --git a/packages/shared/sdk-client/src/LDClientImpl.ts b/packages/shared/sdk-client/src/LDClientImpl.ts index 4347f7d4e1..dcca976ba9 100644 --- a/packages/shared/sdk-client/src/LDClientImpl.ts +++ b/packages/shared/sdk-client/src/LDClientImpl.ts @@ -31,10 +31,6 @@ import { addAutoEnv, calculateFlagChanges, ensureKey } from './utils'; const { createErrorEvaluationDetail, createSuccessEvaluationDetail, ClientMessages, ErrorKinds } = internal; -export const defaultIdentifyOptions: LDIdentifyOptions = { - timeoutSeconds: 5, -}; - export default class LDClientImpl implements LDClient { config: Configuration; context?: LDContext; @@ -43,13 +39,14 @@ export default class LDClientImpl implements LDClient { logger: LDLogger; streamer?: internal.StreamingProcessor; + readonly defaultIdentifyTimeout: number; + private eventFactoryDefault = new EventFactory(false); private eventFactoryWithReasons = new EventFactory(true); private emitter: LDEmitter; private flags: Flags = {}; private identifyChangeListener?: (c: LDContext, changedKeys: string[]) => void; private identifyErrorListener?: (c: LDContext, err: any) => void; - private readonly clientContext: ClientContext; /** @@ -82,6 +79,9 @@ export default class LDClientImpl implements LDClient { !this.isOffline(), ); this.emitter = new LDEmitter(); + + // defaults to 5 seconds + this.defaultIdentifyTimeout = internalOptions?.defaultIdentifyTimeout ?? 5; } /** @@ -239,7 +239,7 @@ export default class LDClientImpl implements LDClient { */ protected createStreamUriPath(_context: LDContext): string { throw new Error( - 'createStreamUriPath not implemented. Client sdks must implement createStreamUriPath for streamer to work', + 'createStreamUriPath not implemented. Client sdks must implement createStreamUriPath for streamer to work.', ); } @@ -281,15 +281,18 @@ export default class LDClientImpl implements LDClient { * Identifies a context to LaunchDarkly. See {@link LDClient.identify}. * * @param pristineContext The LDContext object to be identified. - * @param LDIdentifyOptions You can specify additional options like a timeout when - * identifying. + * @param identifyOptions Optional configuration. See {@link LDIdentifyOptions}. * * @returns {Promise}. */ - async identify( - pristineContext: LDContext, - { timeoutSeconds }: LDIdentifyOptions = defaultIdentifyOptions, - ): Promise { + async identify(pristineContext: LDContext, identifyOptions?: LDIdentifyOptions): Promise { + const identifyTimeout = identifyOptions?.timeoutSeconds ?? this.defaultIdentifyTimeout; + if (identifyTimeout > this.defaultIdentifyTimeout) { + this.logger.warn( + `** Your timeout is too high **. We recommend a max of ${this.defaultIdentifyTimeout}s to prevent blocking your application in the event of network delays.`, + ); + } + let context = await ensureKey(pristineContext, this.platform); if (this.autoEnvAttributes === AutoEnvAttributes.Enabled) { @@ -305,7 +308,7 @@ export default class LDClientImpl implements LDClient { } this.eventProcessor?.sendEvent(this.eventFactoryDefault.identifyEvent(checkedContext)); - const { identifyPromise, identifyResolve } = this.createIdentifyPromise(timeoutSeconds); + const { identifyPromise, identifyResolve } = this.createIdentifyPromise(identifyTimeout); this.logger.debug(`Identifying ${JSON.stringify(context)}`); const flagsStorage = await this.getFlagsFromStorage(checkedContext.canonicalKey); From e47fdb2cb5005b96a63672de3039ab3239dcf112 Mon Sep 17 00:00:00 2001 From: Yusinto Ngadiman Date: Wed, 3 Apr 2024 00:30:46 -0700 Subject: [PATCH 07/15] chore: Added more unit tests. --- .../src/ReactNativeLDClient.test.ts | 1 + .../src/LDClientImpl.timeout.test.ts | 51 +++++++++++++++++-- 2 files changed, 47 insertions(+), 5 deletions(-) diff --git a/packages/sdk/react-native/src/ReactNativeLDClient.test.ts b/packages/sdk/react-native/src/ReactNativeLDClient.test.ts index 439a98c1cb..4f97461a18 100644 --- a/packages/sdk/react-native/src/ReactNativeLDClient.test.ts +++ b/packages/sdk/react-native/src/ReactNativeLDClient.test.ts @@ -10,6 +10,7 @@ describe('ReactNativeLDClient', () => { }); test('constructing a new client', () => { + expect(ldc.defaultIdentifyTimeout).toEqual(15); expect(ldc.sdkKey).toEqual('mobile-key'); expect(ldc.config.serviceEndpoints).toEqual({ analyticsEventPath: '/mobile', diff --git a/packages/shared/sdk-client/src/LDClientImpl.timeout.test.ts b/packages/shared/sdk-client/src/LDClientImpl.timeout.test.ts index 19e498b3b7..48edb6ee03 100644 --- a/packages/shared/sdk-client/src/LDClientImpl.timeout.test.ts +++ b/packages/shared/sdk-client/src/LDClientImpl.timeout.test.ts @@ -21,6 +21,8 @@ jest.mock('@launchdarkly/js-sdk-common', () => { }); const testSdkKey = 'test-sdk-key'; +const carContext: LDContext = { kind: 'car', key: 'test-car' }; + let ldc: LDClientImpl; let defaultPutResponse: Flags; @@ -50,7 +52,6 @@ describe('sdk-client identify timeout', () => { // streamer is setup to error in beforeEach to cause a timeout test('rejects with default timeout of 5s', async () => { - const carContext: LDContext = { kind: 'car', key: 'test-car' }; jest.advanceTimersByTimeAsync(ldc.defaultIdentifyTimeout * 1000).then(); await expect(ldc.identify(carContext)).rejects.toThrow(/identify timed out/); }); @@ -58,8 +59,6 @@ describe('sdk-client identify timeout', () => { // streamer is setup to error in beforeEach to cause a timeout test('rejects with custom timeout', async () => { const timeoutSeconds = 15; - const carContext: LDContext = { kind: 'car', key: 'test-car' }; - jest.advanceTimersByTimeAsync(timeoutSeconds * 1000).then(); await expect(ldc.identify(carContext, { timeoutSeconds })).rejects.toThrow( @@ -68,7 +67,6 @@ describe('sdk-client identify timeout', () => { }); test('resolves with default timeout', async () => { - const carContext: LDContext = { kind: 'car', key: 'test-car' }; setupMockStreamingProcessor(false, defaultPutResponse); jest.advanceTimersByTimeAsync(ldc.defaultIdentifyTimeout * 1000).then(); @@ -89,7 +87,6 @@ describe('sdk-client identify timeout', () => { test('resolves with custom timeout', async () => { const timeoutSeconds = 15; - const carContext: LDContext = { kind: 'car', key: 'test-car' }; setupMockStreamingProcessor(false, defaultPutResponse); jest.advanceTimersByTimeAsync(timeoutSeconds).then(); @@ -107,4 +104,48 @@ describe('sdk-client identify timeout', () => { 'this-is-a-test': true, }); }); + + test('setting default timeout with internalOptions', async () => { + const sdkTimeout = 20; + setupMockStreamingProcessor(false, defaultPutResponse); + ldc = new LDClientImpl( + testSdkKey, + AutoEnvAttributes.Enabled, + basicPlatform, + { + logger, + sendEvents: false, + }, + { defaultIdentifyTimeout: sdkTimeout }, + ); + const { defaultIdentifyTimeout } = ldc; + jest.advanceTimersByTimeAsync(defaultIdentifyTimeout * 1000).then(); + + await ldc.identify(carContext); + + expect(defaultIdentifyTimeout).toEqual(sdkTimeout); + expect(logger.warn).not.toHaveBeenCalledWith(expect.stringMatching(/too high/)); + }); + + test('warning when timeout is too high', async () => { + const { defaultIdentifyTimeout } = ldc; + const dangerousTimeout = defaultIdentifyTimeout * 2; + setupMockStreamingProcessor(false, defaultPutResponse); + jest.advanceTimersByTimeAsync(defaultIdentifyTimeout * 1000).then(); + + await ldc.identify(carContext, { timeoutSeconds: dangerousTimeout }); + + expect(logger.warn).toHaveBeenCalledWith(expect.stringMatching(/too high/)); + }); + + test('safe timeout should not warn', async () => { + const { defaultIdentifyTimeout } = ldc; + const safeTimeout = defaultIdentifyTimeout; + setupMockStreamingProcessor(false, defaultPutResponse); + jest.advanceTimersByTimeAsync(defaultIdentifyTimeout * 1000).then(); + + await ldc.identify(carContext, { timeoutSeconds: safeTimeout }); + + expect(logger.warn).not.toHaveBeenCalledWith(expect.stringMatching(/too high/)); + }); }); From 0c80446e7e69ca1801a1abb8683bc3a887abe2a8 Mon Sep 17 00:00:00 2001 From: Yusinto Ngadiman Date: Wed, 3 Apr 2024 01:44:23 -0700 Subject: [PATCH 08/15] chore: Log timeout error. Added tests. Improve error listener arg type. --- .../sdk-client/src/LDClientImpl.timeout.test.ts | 1 + packages/shared/sdk-client/src/LDClientImpl.ts | 13 ++++++++++--- 2 files changed, 11 insertions(+), 3 deletions(-) diff --git a/packages/shared/sdk-client/src/LDClientImpl.timeout.test.ts b/packages/shared/sdk-client/src/LDClientImpl.timeout.test.ts index 48edb6ee03..f41938e006 100644 --- a/packages/shared/sdk-client/src/LDClientImpl.timeout.test.ts +++ b/packages/shared/sdk-client/src/LDClientImpl.timeout.test.ts @@ -54,6 +54,7 @@ describe('sdk-client identify timeout', () => { test('rejects with default timeout of 5s', async () => { jest.advanceTimersByTimeAsync(ldc.defaultIdentifyTimeout * 1000).then(); await expect(ldc.identify(carContext)).rejects.toThrow(/identify timed out/); + expect(logger.error).toHaveBeenCalledWith(new Error('identify timed out.')); }); // streamer is setup to error in beforeEach to cause a timeout diff --git a/packages/shared/sdk-client/src/LDClientImpl.ts b/packages/shared/sdk-client/src/LDClientImpl.ts index dcca976ba9..5fd841f0a7 100644 --- a/packages/shared/sdk-client/src/LDClientImpl.ts +++ b/packages/shared/sdk-client/src/LDClientImpl.ts @@ -258,7 +258,7 @@ export default class LDClientImpl implements LDClient { this.identifyChangeListener = (c: LDContext, changedKeys: string[]) => { this.logger.debug(`change: context: ${JSON.stringify(c)}, flags: ${changedKeys}`); }; - this.identifyErrorListener = (c: LDContext, err: any) => { + this.identifyErrorListener = (c: LDContext, err: Error) => { this.logger.debug(`error: ${err}, context: ${JSON.stringify(c)}`); reject(err); }; @@ -266,8 +266,15 @@ export default class LDClientImpl implements LDClient { this.emitter.on('change', this.identifyChangeListener); this.emitter.on('error', this.identifyErrorListener); }); - const timed = timeout(timeoutSeconds, 'identify timed out.'); - const raced = Promise.race([timed, slow]); + + const timeoutError = 'identify timed out.'; + const timed = timeout(timeoutSeconds, timeoutError); + const raced = Promise.race([timed, slow]).catch((reason: Error) => { + if (reason.message === timeoutError) { + this.logger.error(reason); + } + throw reason; + }); return { identifyPromise: raced, identifyResolve: res }; } From 53d48459b903387d3f8492d2e2f277bfe2e444ab Mon Sep 17 00:00:00 2001 From: Yusinto Ngadiman Date: Wed, 3 Apr 2024 03:14:17 -0700 Subject: [PATCH 09/15] chore: Improve error logging. --- packages/shared/common/src/utils/timeout.ts | 14 ++++++++++---- .../sdk-client/src/LDClientImpl.timeout.test.ts | 2 +- packages/shared/sdk-client/src/LDClientImpl.ts | 10 ++-------- 3 files changed, 13 insertions(+), 13 deletions(-) diff --git a/packages/shared/common/src/utils/timeout.ts b/packages/shared/common/src/utils/timeout.ts index ed46dda90c..8224cfa1ee 100644 --- a/packages/shared/common/src/utils/timeout.ts +++ b/packages/shared/common/src/utils/timeout.ts @@ -1,13 +1,19 @@ +import { LDLogger } from '../api'; + /** * Returns a promise which errors after t seconds. * * @param t Timeout in seconds. - * @param error Throw this error when when time is up. Should be a meaningful - * description of what task has timed out to aid debugging. + * @param taskName Name of task being timed for logging and error reporting. + * @param logger {@link LDLogger} object. */ -const timeout = (t: number, error?: string) => +const timeout = (t: number, taskName: string, logger?: LDLogger) => new Promise((_res, reject) => { - setTimeout(() => reject(new Error(error ?? `Timed out after ${t} seconds.`)), t * 1000); + setTimeout(() => { + const e = `${taskName} timed out after ${t} seconds.`; + logger?.error(e); + reject(new Error(e)); + }, t * 1000); }); export default timeout; diff --git a/packages/shared/sdk-client/src/LDClientImpl.timeout.test.ts b/packages/shared/sdk-client/src/LDClientImpl.timeout.test.ts index f41938e006..4c450a7c9a 100644 --- a/packages/shared/sdk-client/src/LDClientImpl.timeout.test.ts +++ b/packages/shared/sdk-client/src/LDClientImpl.timeout.test.ts @@ -54,7 +54,7 @@ describe('sdk-client identify timeout', () => { test('rejects with default timeout of 5s', async () => { jest.advanceTimersByTimeAsync(ldc.defaultIdentifyTimeout * 1000).then(); await expect(ldc.identify(carContext)).rejects.toThrow(/identify timed out/); - expect(logger.error).toHaveBeenCalledWith(new Error('identify timed out.')); + expect(logger.error).toHaveBeenCalledWith(expect.stringMatching(/identify timed out/)); }); // streamer is setup to error in beforeEach to cause a timeout diff --git a/packages/shared/sdk-client/src/LDClientImpl.ts b/packages/shared/sdk-client/src/LDClientImpl.ts index 5fd841f0a7..c34b5394df 100644 --- a/packages/shared/sdk-client/src/LDClientImpl.ts +++ b/packages/shared/sdk-client/src/LDClientImpl.ts @@ -267,14 +267,8 @@ export default class LDClientImpl implements LDClient { this.emitter.on('error', this.identifyErrorListener); }); - const timeoutError = 'identify timed out.'; - const timed = timeout(timeoutSeconds, timeoutError); - const raced = Promise.race([timed, slow]).catch((reason: Error) => { - if (reason.message === timeoutError) { - this.logger.error(reason); - } - throw reason; - }); + const timed = timeout(timeoutSeconds, 'identify', this.logger); + const raced = Promise.race([timed, slow]); return { identifyPromise: raced, identifyResolve: res }; } From 409f340b8a7c4439a5c7c5ccfab40bb9e2a3d3a7 Mon Sep 17 00:00:00 2001 From: Yusinto Ngadiman Date: Wed, 3 Apr 2024 10:34:50 -0700 Subject: [PATCH 10/15] chore: Improve high timeout warning. --- packages/shared/sdk-client/src/LDClientImpl.ts | 4 +--- 1 file changed, 1 insertion(+), 3 deletions(-) diff --git a/packages/shared/sdk-client/src/LDClientImpl.ts b/packages/shared/sdk-client/src/LDClientImpl.ts index c34b5394df..8d0e992919 100644 --- a/packages/shared/sdk-client/src/LDClientImpl.ts +++ b/packages/shared/sdk-client/src/LDClientImpl.ts @@ -289,9 +289,7 @@ export default class LDClientImpl implements LDClient { async identify(pristineContext: LDContext, identifyOptions?: LDIdentifyOptions): Promise { const identifyTimeout = identifyOptions?.timeoutSeconds ?? this.defaultIdentifyTimeout; if (identifyTimeout > this.defaultIdentifyTimeout) { - this.logger.warn( - `** Your timeout is too high **. We recommend a max of ${this.defaultIdentifyTimeout}s to prevent blocking your application in the event of network delays.`, - ); + this.logger.warn('identify called with high timeout parameter.'); } let context = await ensureKey(pristineContext, this.platform); From d9c0469a3d5bc3e68da525734118e4868d4d6702 Mon Sep 17 00:00:00 2001 From: Yusinto Ngadiman Date: Wed, 3 Apr 2024 10:42:16 -0700 Subject: [PATCH 11/15] chore: Renamed timeoutSeconds to timeout. Renamed timeout function to timedPromise. --- .../sdk/react-native/example/src/welcome.tsx | 2 +- packages/shared/common/src/utils/index.ts | 4 ++-- .../src/utils/{timeout.ts => timedPromise.ts} | 4 ++-- .../src/LDClientImpl.timeout.test.ts | 18 ++++++++---------- packages/shared/sdk-client/src/LDClientImpl.ts | 8 ++++---- .../sdk-client/src/api/LDIdentifyOptions.ts | 4 ++-- 6 files changed, 19 insertions(+), 21 deletions(-) rename packages/shared/common/src/utils/{timeout.ts => timedPromise.ts} (81%) diff --git a/packages/sdk/react-native/example/src/welcome.tsx b/packages/sdk/react-native/example/src/welcome.tsx index f5447f65df..028c0dffb6 100644 --- a/packages/sdk/react-native/example/src/welcome.tsx +++ b/packages/sdk/react-native/example/src/welcome.tsx @@ -12,7 +12,7 @@ export default function Welcome() { const onIdentify = () => { ldc - .identify({ kind: 'user', key: userKey }) + .identify({ kind: 'user', key: userKey }, { timeout: 5 }) .catch((e: any) => console.error(`error identifying ${userKey}: ${e}`)); }; diff --git a/packages/shared/common/src/utils/index.ts b/packages/shared/common/src/utils/index.ts index fbbb63bed7..a6f49e0770 100644 --- a/packages/shared/common/src/utils/index.ts +++ b/packages/shared/common/src/utils/index.ts @@ -6,7 +6,7 @@ import fastDeepEqual from './fast-deep-equal'; import { base64UrlEncode, defaultHeaders, httpErrorMessage, LDHeaders, shouldRetry } from './http'; import noop from './noop'; import sleep from './sleep'; -import timeout from './timeout'; +import timedPromise from './timedPromise'; import { VoidFunction } from './VoidFunction'; export { @@ -22,6 +22,6 @@ export { shouldRetry, secondsToMillis, sleep, - timeout, + timedPromise, VoidFunction, }; diff --git a/packages/shared/common/src/utils/timeout.ts b/packages/shared/common/src/utils/timedPromise.ts similarity index 81% rename from packages/shared/common/src/utils/timeout.ts rename to packages/shared/common/src/utils/timedPromise.ts index 8224cfa1ee..6aa101d3bc 100644 --- a/packages/shared/common/src/utils/timeout.ts +++ b/packages/shared/common/src/utils/timedPromise.ts @@ -7,7 +7,7 @@ import { LDLogger } from '../api'; * @param taskName Name of task being timed for logging and error reporting. * @param logger {@link LDLogger} object. */ -const timeout = (t: number, taskName: string, logger?: LDLogger) => +const timedPromise = (t: number, taskName: string, logger?: LDLogger) => new Promise((_res, reject) => { setTimeout(() => { const e = `${taskName} timed out after ${t} seconds.`; @@ -16,4 +16,4 @@ const timeout = (t: number, taskName: string, logger?: LDLogger) => }, t * 1000); }); -export default timeout; +export default timedPromise; diff --git a/packages/shared/sdk-client/src/LDClientImpl.timeout.test.ts b/packages/shared/sdk-client/src/LDClientImpl.timeout.test.ts index 4c450a7c9a..bfd7dd198c 100644 --- a/packages/shared/sdk-client/src/LDClientImpl.timeout.test.ts +++ b/packages/shared/sdk-client/src/LDClientImpl.timeout.test.ts @@ -59,12 +59,10 @@ describe('sdk-client identify timeout', () => { // streamer is setup to error in beforeEach to cause a timeout test('rejects with custom timeout', async () => { - const timeoutSeconds = 15; - jest.advanceTimersByTimeAsync(timeoutSeconds * 1000).then(); + const timeout = 15; + jest.advanceTimersByTimeAsync(timeout * 1000).then(); - await expect(ldc.identify(carContext, { timeoutSeconds })).rejects.toThrow( - /identify timed out/, - ); + await expect(ldc.identify(carContext, { timeout })).rejects.toThrow(/identify timed out/); }); test('resolves with default timeout', async () => { @@ -87,11 +85,11 @@ describe('sdk-client identify timeout', () => { }); test('resolves with custom timeout', async () => { - const timeoutSeconds = 15; + const timeout = 15; setupMockStreamingProcessor(false, defaultPutResponse); - jest.advanceTimersByTimeAsync(timeoutSeconds).then(); + jest.advanceTimersByTimeAsync(timeout).then(); - await expect(ldc.identify(carContext, { timeoutSeconds })).resolves.toBeUndefined(); + await expect(ldc.identify(carContext, { timeout })).resolves.toBeUndefined(); expect(ldc.getContext()).toEqual(expect.objectContaining(toMulti(carContext))); expect(ldc.allFlags()).toEqual({ @@ -134,7 +132,7 @@ describe('sdk-client identify timeout', () => { setupMockStreamingProcessor(false, defaultPutResponse); jest.advanceTimersByTimeAsync(defaultIdentifyTimeout * 1000).then(); - await ldc.identify(carContext, { timeoutSeconds: dangerousTimeout }); + await ldc.identify(carContext, { timeout: dangerousTimeout }); expect(logger.warn).toHaveBeenCalledWith(expect.stringMatching(/too high/)); }); @@ -145,7 +143,7 @@ describe('sdk-client identify timeout', () => { setupMockStreamingProcessor(false, defaultPutResponse); jest.advanceTimersByTimeAsync(defaultIdentifyTimeout * 1000).then(); - await ldc.identify(carContext, { timeoutSeconds: safeTimeout }); + await ldc.identify(carContext, { timeout: safeTimeout }); expect(logger.warn).not.toHaveBeenCalledWith(expect.stringMatching(/too high/)); }); diff --git a/packages/shared/sdk-client/src/LDClientImpl.ts b/packages/shared/sdk-client/src/LDClientImpl.ts index 8d0e992919..d69a0a071c 100644 --- a/packages/shared/sdk-client/src/LDClientImpl.ts +++ b/packages/shared/sdk-client/src/LDClientImpl.ts @@ -14,7 +14,7 @@ import { Platform, ProcessStreamResponse, EventName as StreamEventName, - timeout, + timedPromise, TypeValidators, } from '@launchdarkly/js-sdk-common'; @@ -243,7 +243,7 @@ export default class LDClientImpl implements LDClient { ); } - private createIdentifyPromise(timeoutSeconds: number) { + private createIdentifyPromise(timeout: number) { let res: any; const slow = new Promise((resolve, reject) => { res = resolve; @@ -267,7 +267,7 @@ export default class LDClientImpl implements LDClient { this.emitter.on('error', this.identifyErrorListener); }); - const timed = timeout(timeoutSeconds, 'identify', this.logger); + const timed = timedPromise(timeout, 'identify', this.logger); const raced = Promise.race([timed, slow]); return { identifyPromise: raced, identifyResolve: res }; @@ -287,7 +287,7 @@ export default class LDClientImpl implements LDClient { * @returns {Promise}. */ async identify(pristineContext: LDContext, identifyOptions?: LDIdentifyOptions): Promise { - const identifyTimeout = identifyOptions?.timeoutSeconds ?? this.defaultIdentifyTimeout; + const identifyTimeout = identifyOptions?.timeout ?? this.defaultIdentifyTimeout; if (identifyTimeout > this.defaultIdentifyTimeout) { this.logger.warn('identify called with high timeout parameter.'); } diff --git a/packages/shared/sdk-client/src/api/LDIdentifyOptions.ts b/packages/shared/sdk-client/src/api/LDIdentifyOptions.ts index 128088042f..355c0e93ec 100644 --- a/packages/shared/sdk-client/src/api/LDIdentifyOptions.ts +++ b/packages/shared/sdk-client/src/api/LDIdentifyOptions.ts @@ -1,11 +1,11 @@ export interface LDIdentifyOptions { /** - * Determines when the identify promise resolves if no flags have been + * In seconds. Determines when the identify promise resolves if no flags have been * returned from the network. If you use a large timeout and await it, then * any network delays will cause your application to wait a long time before * continuing execution. * * Defaults to 5 seconds. */ - timeoutSeconds: number; + timeout: number; } From 381feee3bab36221d2f599dabfb15607da99299c Mon Sep 17 00:00:00 2001 From: Yusinto Ngadiman Date: Wed, 3 Apr 2024 10:44:09 -0700 Subject: [PATCH 12/15] chore: Fixed broken tests due to warning string. --- packages/shared/sdk-client/src/LDClientImpl.timeout.test.ts | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/packages/shared/sdk-client/src/LDClientImpl.timeout.test.ts b/packages/shared/sdk-client/src/LDClientImpl.timeout.test.ts index bfd7dd198c..0fdc3e118d 100644 --- a/packages/shared/sdk-client/src/LDClientImpl.timeout.test.ts +++ b/packages/shared/sdk-client/src/LDClientImpl.timeout.test.ts @@ -123,7 +123,7 @@ describe('sdk-client identify timeout', () => { await ldc.identify(carContext); expect(defaultIdentifyTimeout).toEqual(sdkTimeout); - expect(logger.warn).not.toHaveBeenCalledWith(expect.stringMatching(/too high/)); + expect(logger.warn).not.toHaveBeenCalledWith(expect.stringMatching(/high timeout/)); }); test('warning when timeout is too high', async () => { @@ -134,7 +134,7 @@ describe('sdk-client identify timeout', () => { await ldc.identify(carContext, { timeout: dangerousTimeout }); - expect(logger.warn).toHaveBeenCalledWith(expect.stringMatching(/too high/)); + expect(logger.warn).toHaveBeenCalledWith(expect.stringMatching(/high timeout/)); }); test('safe timeout should not warn', async () => { @@ -145,6 +145,6 @@ describe('sdk-client identify timeout', () => { await ldc.identify(carContext, { timeout: safeTimeout }); - expect(logger.warn).not.toHaveBeenCalledWith(expect.stringMatching(/too high/)); + expect(logger.warn).not.toHaveBeenCalledWith(expect.stringMatching(/high timeout/)); }); }); From c45a8671972f15d1386d910e53533f78bdfe4216 Mon Sep 17 00:00:00 2001 From: Yusinto Ngadiman Date: Wed, 3 Apr 2024 11:08:33 -0700 Subject: [PATCH 13/15] chore: Minor comment improvement. --- .../shared/common/src/internal/events/LDInternalOptions.ts | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/packages/shared/common/src/internal/events/LDInternalOptions.ts b/packages/shared/common/src/internal/events/LDInternalOptions.ts index 69fa9786d0..b86bed297c 100644 --- a/packages/shared/common/src/internal/events/LDInternalOptions.ts +++ b/packages/shared/common/src/internal/events/LDInternalOptions.ts @@ -15,7 +15,8 @@ export type LDInternalOptions = { /** * Sets the default identify timeout in seconds. - * If not specified, this is set to 5 seconds in LDClientImpl.constructor. + * If unspecified for sdk-client, this is set to 5 seconds in LDClientImpl.constructor. + * * Mobile sdks should set this to 15 seconds. */ defaultIdentifyTimeout?: number; From d64edae5e0f68e87925a116eae6f3b0ac3a751e8 Mon Sep 17 00:00:00 2001 From: Yusinto Ngadiman Date: Wed, 3 Apr 2024 12:33:56 -0700 Subject: [PATCH 14/15] chore: Introduce customizable highTimeoutThreshold to replace defaultIdentifyTimeout. --- .../src/ReactNativeLDClient.test.ts | 2 +- .../react-native/src/ReactNativeLDClient.ts | 2 +- .../src/internal/events/LDInternalOptions.ts | 9 +++--- .../src/LDClientImpl.timeout.test.ts | 30 +++++++++---------- .../shared/sdk-client/src/LDClientImpl.ts | 18 ++++++----- 5 files changed, 32 insertions(+), 29 deletions(-) diff --git a/packages/sdk/react-native/src/ReactNativeLDClient.test.ts b/packages/sdk/react-native/src/ReactNativeLDClient.test.ts index 4f97461a18..aafe5b1553 100644 --- a/packages/sdk/react-native/src/ReactNativeLDClient.test.ts +++ b/packages/sdk/react-native/src/ReactNativeLDClient.test.ts @@ -10,7 +10,7 @@ describe('ReactNativeLDClient', () => { }); test('constructing a new client', () => { - expect(ldc.defaultIdentifyTimeout).toEqual(15); + expect(ldc.highTimeoutThreshold).toEqual(15); expect(ldc.sdkKey).toEqual('mobile-key'); expect(ldc.config.serviceEndpoints).toEqual({ analyticsEventPath: '/mobile', diff --git a/packages/sdk/react-native/src/ReactNativeLDClient.ts b/packages/sdk/react-native/src/ReactNativeLDClient.ts index d3bfad59bf..9ccff1f392 100644 --- a/packages/sdk/react-native/src/ReactNativeLDClient.ts +++ b/packages/sdk/react-native/src/ReactNativeLDClient.ts @@ -47,7 +47,7 @@ export default class ReactNativeLDClient extends LDClientImpl { const internalOptions: internal.LDInternalOptions = { analyticsEventPath: `/mobile`, diagnosticEventPath: `/mobile/events/diagnostic`, - defaultIdentifyTimeout: 15, + highTimeoutThreshold: 15, }; super( diff --git a/packages/shared/common/src/internal/events/LDInternalOptions.ts b/packages/shared/common/src/internal/events/LDInternalOptions.ts index b86bed297c..b852990a3e 100644 --- a/packages/shared/common/src/internal/events/LDInternalOptions.ts +++ b/packages/shared/common/src/internal/events/LDInternalOptions.ts @@ -14,10 +14,11 @@ export type LDInternalOptions = { includeAuthorizationHeader?: boolean; /** - * Sets the default identify timeout in seconds. - * If unspecified for sdk-client, this is set to 5 seconds in LDClientImpl.constructor. + * In seconds. Log a warning if identifyTimeout is greater than this value. * - * Mobile sdks should set this to 15 seconds. + * Mobile - 15s. + * Browser - 5s. + * Server - 60s. */ - defaultIdentifyTimeout?: number; + highTimeoutThreshold?: number; }; diff --git a/packages/shared/sdk-client/src/LDClientImpl.timeout.test.ts b/packages/shared/sdk-client/src/LDClientImpl.timeout.test.ts index 0fdc3e118d..06c190d264 100644 --- a/packages/shared/sdk-client/src/LDClientImpl.timeout.test.ts +++ b/packages/shared/sdk-client/src/LDClientImpl.timeout.test.ts @@ -52,7 +52,7 @@ describe('sdk-client identify timeout', () => { // streamer is setup to error in beforeEach to cause a timeout test('rejects with default timeout of 5s', async () => { - jest.advanceTimersByTimeAsync(ldc.defaultIdentifyTimeout * 1000).then(); + jest.advanceTimersByTimeAsync(ldc.identifyTimeout * 1000).then(); await expect(ldc.identify(carContext)).rejects.toThrow(/identify timed out/); expect(logger.error).toHaveBeenCalledWith(expect.stringMatching(/identify timed out/)); }); @@ -67,7 +67,7 @@ describe('sdk-client identify timeout', () => { test('resolves with default timeout', async () => { setupMockStreamingProcessor(false, defaultPutResponse); - jest.advanceTimersByTimeAsync(ldc.defaultIdentifyTimeout * 1000).then(); + jest.advanceTimersByTimeAsync(ldc.identifyTimeout * 1000).then(); await expect(ldc.identify(carContext)).resolves.toBeUndefined(); @@ -104,8 +104,8 @@ describe('sdk-client identify timeout', () => { }); }); - test('setting default timeout with internalOptions', async () => { - const sdkTimeout = 20; + test('setting high timeout threshold with internalOptions', async () => { + const highTimeoutThreshold = 20; setupMockStreamingProcessor(false, defaultPutResponse); ldc = new LDClientImpl( testSdkKey, @@ -115,22 +115,22 @@ describe('sdk-client identify timeout', () => { logger, sendEvents: false, }, - { defaultIdentifyTimeout: sdkTimeout }, + { highTimeoutThreshold }, ); - const { defaultIdentifyTimeout } = ldc; - jest.advanceTimersByTimeAsync(defaultIdentifyTimeout * 1000).then(); + const { identifyTimeout } = ldc; + jest.advanceTimersByTimeAsync(identifyTimeout * 1000).then(); - await ldc.identify(carContext); + await ldc.identify(carContext, { timeout: 10 }); - expect(defaultIdentifyTimeout).toEqual(sdkTimeout); + expect(identifyTimeout).toEqual(10); expect(logger.warn).not.toHaveBeenCalledWith(expect.stringMatching(/high timeout/)); }); test('warning when timeout is too high', async () => { - const { defaultIdentifyTimeout } = ldc; - const dangerousTimeout = defaultIdentifyTimeout * 2; + const { identifyTimeout } = ldc; + const dangerousTimeout = identifyTimeout * 2; setupMockStreamingProcessor(false, defaultPutResponse); - jest.advanceTimersByTimeAsync(defaultIdentifyTimeout * 1000).then(); + jest.advanceTimersByTimeAsync(identifyTimeout * 1000).then(); await ldc.identify(carContext, { timeout: dangerousTimeout }); @@ -138,10 +138,10 @@ describe('sdk-client identify timeout', () => { }); test('safe timeout should not warn', async () => { - const { defaultIdentifyTimeout } = ldc; - const safeTimeout = defaultIdentifyTimeout; + const { identifyTimeout } = ldc; + const safeTimeout = identifyTimeout; setupMockStreamingProcessor(false, defaultPutResponse); - jest.advanceTimersByTimeAsync(defaultIdentifyTimeout * 1000).then(); + jest.advanceTimersByTimeAsync(identifyTimeout * 1000).then(); await ldc.identify(carContext, { timeout: safeTimeout }); diff --git a/packages/shared/sdk-client/src/LDClientImpl.ts b/packages/shared/sdk-client/src/LDClientImpl.ts index d69a0a071c..7aa983d3f5 100644 --- a/packages/shared/sdk-client/src/LDClientImpl.ts +++ b/packages/shared/sdk-client/src/LDClientImpl.ts @@ -36,10 +36,11 @@ export default class LDClientImpl implements LDClient { context?: LDContext; diagnosticsManager?: internal.DiagnosticsManager; eventProcessor?: internal.EventProcessor; + identifyTimeout: number = 5; logger: LDLogger; streamer?: internal.StreamingProcessor; - readonly defaultIdentifyTimeout: number; + readonly highTimeoutThreshold: number = 15; private eventFactoryDefault = new EventFactory(false); private eventFactoryWithReasons = new EventFactory(true); @@ -47,6 +48,7 @@ export default class LDClientImpl implements LDClient { private flags: Flags = {}; private identifyChangeListener?: (c: LDContext, changedKeys: string[]) => void; private identifyErrorListener?: (c: LDContext, err: any) => void; + private readonly clientContext: ClientContext; /** @@ -79,9 +81,6 @@ export default class LDClientImpl implements LDClient { !this.isOffline(), ); this.emitter = new LDEmitter(); - - // defaults to 5 seconds - this.defaultIdentifyTimeout = internalOptions?.defaultIdentifyTimeout ?? 5; } /** @@ -106,7 +105,7 @@ export default class LDClientImpl implements LDClient { if (this.context) { // identify will start streamer - return this.identify(this.context); + return this.identify(this.context, { timeout: this.identifyTimeout }); } break; default: @@ -287,8 +286,11 @@ export default class LDClientImpl implements LDClient { * @returns {Promise}. */ async identify(pristineContext: LDContext, identifyOptions?: LDIdentifyOptions): Promise { - const identifyTimeout = identifyOptions?.timeout ?? this.defaultIdentifyTimeout; - if (identifyTimeout > this.defaultIdentifyTimeout) { + if (identifyOptions?.timeout) { + this.identifyTimeout = identifyOptions.timeout; + } + + if (this.identifyTimeout > this.highTimeoutThreshold) { this.logger.warn('identify called with high timeout parameter.'); } @@ -307,7 +309,7 @@ export default class LDClientImpl implements LDClient { } this.eventProcessor?.sendEvent(this.eventFactoryDefault.identifyEvent(checkedContext)); - const { identifyPromise, identifyResolve } = this.createIdentifyPromise(identifyTimeout); + const { identifyPromise, identifyResolve } = this.createIdentifyPromise(this.identifyTimeout); this.logger.debug(`Identifying ${JSON.stringify(context)}`); const flagsStorage = await this.getFlagsFromStorage(checkedContext.canonicalKey); From ebb5d864be2e626be7af87b8432b69f1b6732caf Mon Sep 17 00:00:00 2001 From: Yusinto Ngadiman Date: Wed, 3 Apr 2024 12:39:02 -0700 Subject: [PATCH 15/15] chore: Fixed broken unit tests. --- .../sdk-client/src/LDClientImpl.timeout.test.ts | 16 +++++++--------- 1 file changed, 7 insertions(+), 9 deletions(-) diff --git a/packages/shared/sdk-client/src/LDClientImpl.timeout.test.ts b/packages/shared/sdk-client/src/LDClientImpl.timeout.test.ts index 06c190d264..fbb1c53092 100644 --- a/packages/shared/sdk-client/src/LDClientImpl.timeout.test.ts +++ b/packages/shared/sdk-client/src/LDClientImpl.timeout.test.ts @@ -117,22 +117,20 @@ describe('sdk-client identify timeout', () => { }, { highTimeoutThreshold }, ); - const { identifyTimeout } = ldc; - jest.advanceTimersByTimeAsync(identifyTimeout * 1000).then(); - - await ldc.identify(carContext, { timeout: 10 }); + const customTimeout = 10; + jest.advanceTimersByTimeAsync(customTimeout * 1000).then(); + await ldc.identify(carContext, { timeout: customTimeout }); - expect(identifyTimeout).toEqual(10); + expect(ldc.identifyTimeout).toEqual(10); expect(logger.warn).not.toHaveBeenCalledWith(expect.stringMatching(/high timeout/)); }); test('warning when timeout is too high', async () => { - const { identifyTimeout } = ldc; - const dangerousTimeout = identifyTimeout * 2; + const highTimeout = 60; setupMockStreamingProcessor(false, defaultPutResponse); - jest.advanceTimersByTimeAsync(identifyTimeout * 1000).then(); + jest.advanceTimersByTimeAsync(highTimeout * 1000).then(); - await ldc.identify(carContext, { timeout: dangerousTimeout }); + await ldc.identify(carContext, { timeout: highTimeout }); expect(logger.warn).toHaveBeenCalledWith(expect.stringMatching(/high timeout/)); });