Skip to content
Merged
Show file tree
Hide file tree
Changes from 4 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
79 changes: 79 additions & 0 deletions packages/sdk/browser/__tests__/BrowserDataManager.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -527,4 +527,83 @@ describe('given a BrowserDataManager with mocked dependencies', () => {
'[BrowserDataManager] Identify called after data manager was closed.',
);
});

it('retries initial polling until it succeeds', async () => {
jest.useFakeTimers();
const context = Context.fromLDContext({ kind: 'user', key: 'test-user' });
flagManager.loadCached.mockResolvedValue(false);

// Mock fetch to fail twice with 500 error, then succeed
let callCount = 0;
const mockedFetch = jest.fn().mockImplementation(() => {
callCount += 1;
if (callCount <= 2) {
return mockResponse('', 500);
}
return mockResponse('{"flagA": true}', 200);
});

platform.requests.fetch = mockedFetch as typeof platform.requests.fetch;

const identifyResolve = jest.fn();
const identifyReject = jest.fn();

const identifyOptions: BrowserIdentifyOptions = { initialPollingRetries: 3 };
const identifyPromise = dataManager.identify(
identifyResolve,
identifyReject,
context,
identifyOptions,
);

// Fast-forward through the retry delays (2 retries * 1000ms each)
await jest.advanceTimersByTimeAsync(2000);

await identifyPromise;

expect(mockedFetch).toHaveBeenCalledTimes(3);
expect(identifyResolve).toHaveBeenCalled();
expect(identifyReject).not.toHaveBeenCalled();
expect(flagManager.init).toHaveBeenCalledWith(
expect.anything(),
expect.objectContaining({ flagA: { flag: true, version: undefined } }),
);

jest.useRealTimers();
});

it('throws an error when initial polling reaches max retry limit', async () => {
jest.useFakeTimers();
const context = Context.fromLDContext({ kind: 'user', key: 'test-user' });
flagManager.loadCached.mockResolvedValue(false);

// Mock fetch to always fail with 500 error
const mockedFetch = jest.fn().mockImplementation(() => mockResponse('', 500));

platform.requests.fetch = mockedFetch as typeof platform.requests.fetch;

const identifyResolve = jest.fn();
const identifyReject = jest.fn();

const identifyOptions: BrowserIdentifyOptions = { initialPollingRetries: 2 };
const identifyPromise = dataManager.identify(
identifyResolve,
identifyReject,
context,
identifyOptions,
);

// Fast-forward through the retry delays (2 retries * 1000ms each)
await jest.advanceTimersByTimeAsync(2000);

await identifyPromise;

// Should attempt initial request + 2 retries = 3 total attempts
expect(mockedFetch).toHaveBeenCalledTimes(3);
expect(identifyResolve).not.toHaveBeenCalled();
expect(identifyReject).toHaveBeenCalled();
expect(identifyReject).toHaveBeenCalledWith(expect.objectContaining({ status: 500 }));

jest.useRealTimers();
});
});
83 changes: 68 additions & 15 deletions packages/sdk/browser/src/BrowserDataManager.ts
Original file line number Diff line number Diff line change
Expand Up @@ -6,12 +6,15 @@ import {
DataSourcePaths,
DataSourceState,
FlagManager,
httpErrorMessage,
internal,
LDEmitter,
LDHeaders,
LDIdentifyOptions,
makeRequestor,
Platform,
shouldRetry,
sleep,
} from '@launchdarkly/js-client-sdk-common';

import { readFlagsFromBootstrap } from './bootstrap';
Expand Down Expand Up @@ -97,34 +100,84 @@ export default class BrowserDataManager extends BaseDataManager {
this._debugLog('Identify - Flags loaded from cache. Continuing to initialize via a poll.');
}

await this._finishIdentifyFromPoll(context, identifyResolve, identifyReject);
await this._finishIdentifyFromPoll(
context,
identifyResolve,
identifyReject,
browserIdentifyOptions?.initialPollingRetries,
);
}
this._updateStreamingState();
}

/**
* A helper function for the initial poll request. This is mainly here to facilitate
* the retry logic.
*
* @param context - LDContext to request payload for.
* @param maxRetries - Maximum number of retries to attempt. Defaults to 3.
* @returns Payload as a string.
*/
private async _requestPayload(context: Context, maxRetries: number = 3): Promise<string> {
const plainContextString = JSON.stringify(Context.toLDContext(context));
const pollingRequestor = makeRequestor(
plainContextString,
this.config.serviceEndpoints,
this.getPollingPaths(),
this.platform.requests,
this.platform.encoding!,
this.baseHeaders,
[],
this.config.withReasons,
this.config.useReport,
this._secureModeHash,
);

let lastError: any;
let validMaxRetries = maxRetries ?? 3;

if (Number.isNaN(validMaxRetries) || validMaxRetries < 0) {
this.logger.warn(
`initialPollingRetries is set to an invalid value: ${maxRetries}. Defaulting to 3 retries.`,
);
validMaxRetries = 3;
}

for (let attempt = 0; attempt <= validMaxRetries; attempt += 1) {
try {
// eslint-disable-next-line no-await-in-loop
return await pollingRequestor.requestPayload();
} catch (e: any) {
if (!shouldRetry(e)) {
throw e;
}
lastError = e;
// NOTE: current we are hardcoding the retry interval to 1 second.
// We can make this configurable in the future.
// TODO: Reviewer any thoughts on this? Probably the easiest thing is to make this configurable
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is a reasonable start, and then we should look at adding some backoff to it later.

With FDv2 we may consider things a little differently. For example, if streaming is enabled, then this failing just means we may end up initializing from streaming. So we don't want to spend too long on this step.

// however, we can also look into using the backoff logic to calculate the delay?
if (attempt < validMaxRetries) {
this._debugLog(httpErrorMessage(e, 'initial poll request', 'will retry'));
// eslint-disable-next-line no-await-in-loop
await sleep(1000);
}
}
}

throw lastError;
}

private async _finishIdentifyFromPoll(
context: Context,
identifyResolve: () => void,
identifyReject: (err: Error) => void,
initialPollingRetries: number = 3,
) {
try {
this.dataSourceStatusManager.requestStateUpdate(DataSourceState.Initializing);

const plainContextString = JSON.stringify(Context.toLDContext(context));
const pollingRequestor = makeRequestor(
plainContextString,
this.config.serviceEndpoints,
this.getPollingPaths(),
this.platform.requests,
this.platform.encoding!,
this.baseHeaders,
[],
this.config.withReasons,
this.config.useReport,
this._secureModeHash,
);
const payload = await this._requestPayload(context, initialPollingRetries);

const payload = await pollingRequestor.requestPayload();
try {
const listeners = this.createStreamListeners(context, identifyResolve);
const putListener = listeners.get('put');
Expand Down
7 changes: 7 additions & 0 deletions packages/sdk/browser/src/BrowserIdentifyOptions.ts
Original file line number Diff line number Diff line change
Expand Up @@ -28,4 +28,11 @@ export interface BrowserIdentifyOptions extends Omit<LDIdentifyOptions, 'waitFor
* For more information, see the [SDK Reference Guide](https://docs.launchdarkly.com/sdk/features/bootstrapping#javascript).
*/
bootstrap?: unknown;

/**
* The number of retries to attempt for the initial polling request.
*
* Defaults to 3.
*/
initialPollingRetries?: number;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is interesting. I had not considered having this at the identify options level, versus being a client-level option. I am tempted to say we just don't expose the lever at all right now. If we do, then we probably do need that back-off logic. Because we don't want to, for example, retry 50 times once per second.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yea, looking back at this, I agree with not exposing this for now. The original thought was that the identify() method is what really triggers the initialization here so it (kind of) made sense for initialization options to live in identifyOptions. I created another work item to align the initialization with what we have in flutter sdk so this implementation now makes less sense, I'll remove this option for now.

}