Skip to content

Commit

Permalink
fix: Ensure streaming connection is closed on SDK close.
Browse files Browse the repository at this point in the history
  • Loading branch information
kinyoklion committed Feb 6, 2025
1 parent 6058249 commit 37502fc
Show file tree
Hide file tree
Showing 7 changed files with 117 additions and 5 deletions.
32 changes: 31 additions & 1 deletion packages/sdk/browser/__tests__/BrowserDataManager.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -63,13 +63,16 @@ describe('given a BrowserDataManager with mocked dependencies', () => {
let diagnosticsManager: jest.Mocked<internal.DiagnosticsManager>;
let dataManager: BrowserDataManager;
let logger: LDLogger;
let eventSourceCloseMethod: jest.Mock;

beforeEach(() => {
logger = {
error: jest.fn(),
warn: jest.fn(),
info: jest.fn(),
debug: jest.fn(),
};
eventSourceCloseMethod = jest.fn();
config = {
logger,
maxCachedContexts: 5,
Expand Down Expand Up @@ -106,7 +109,7 @@ describe('given a BrowserDataManager with mocked dependencies', () => {
options,
onclose: jest.fn(),
addEventListener: jest.fn(),
close: jest.fn(),
close: eventSourceCloseMethod,
})),
fetch: mockedFetch,
getEventSourceCapabilities: jest.fn(),
Expand Down Expand Up @@ -495,4 +498,31 @@ describe('given a BrowserDataManager with mocked dependencies', () => {

expect(platform.requests.createEventSource).toHaveBeenCalled();
});

it('closes the event source when the data manager is closed', async () => {
const context = Context.fromLDContext({ kind: 'user', key: 'test-user' });
const identifyOptions: BrowserIdentifyOptions = {};
const identifyResolve = jest.fn();
const identifyReject = jest.fn();

dataManager.setForcedStreaming(undefined);
dataManager.setAutomaticStreamingState(true);
expect(platform.requests.createEventSource).not.toHaveBeenCalled();

flagManager.loadCached.mockResolvedValue(false);

await dataManager.identify(identifyResolve, identifyReject, context, identifyOptions);

expect(platform.requests.createEventSource).toHaveBeenCalled();

dataManager.close();
expect(eventSourceCloseMethod).toHaveBeenCalled();
// Verify a subsequent identify doesn't create a new event source
await dataManager.identify(identifyResolve, identifyReject, context, {});
expect(platform.requests.createEventSource).toHaveBeenCalledTimes(1);

expect(logger.debug).toHaveBeenCalledWith(
'[BrowserDataManager] Identify called after data manager was closed.',
);
});
});
5 changes: 5 additions & 0 deletions packages/sdk/browser/src/BrowserDataManager.ts
Original file line number Diff line number Diff line change
Expand Up @@ -74,6 +74,11 @@ export default class BrowserDataManager extends BaseDataManager {
context: Context,
identifyOptions?: LDIdentifyOptions,
): Promise<void> {
if (this.closed) {
this._debugLog('Identify called after data manager was closed.');
return;
}

this.context = context;
const browserIdentifyOptions = identifyOptions as BrowserIdentifyOptions | undefined;
if (browserIdentifyOptions?.hash) {
Expand Down
44 changes: 43 additions & 1 deletion packages/sdk/react-native/__tests__/MobileDataManager.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -54,13 +54,17 @@ describe('given a MobileDataManager with mocked dependencies', () => {
let diagnosticsManager: jest.Mocked<internal.DiagnosticsManager>;
let mobileDataManager: MobileDataManager;
let logger: LDLogger;
let eventSourceCloseMethod: jest.Mock;

beforeEach(() => {
logger = {
error: jest.fn(),
warn: jest.fn(),
info: jest.fn(),
debug: jest.fn(),
};
eventSourceCloseMethod = jest.fn();

config = {
logger,
maxCachedContexts: 5,
Expand Down Expand Up @@ -94,7 +98,7 @@ describe('given a MobileDataManager with mocked dependencies', () => {
options,
onclose: jest.fn(),
addEventListener: jest.fn(),
close: jest.fn(),
close: eventSourceCloseMethod,
})),
fetch: mockedFetch,
getEventSourceCapabilities: jest.fn(),
Expand Down Expand Up @@ -223,6 +227,23 @@ describe('given a MobileDataManager with mocked dependencies', () => {
expect(platform.requests.fetch).not.toHaveBeenCalled();
});

it('makes no connection when closed', async () => {
mobileDataManager.close();

const context = Context.fromLDContext({ kind: 'user', key: 'test-user' });
const identifyOptions: LDIdentifyOptions = { waitForNetworkResults: false };
const identifyResolve = jest.fn();
const identifyReject = jest.fn();

await mobileDataManager.identify(identifyResolve, identifyReject, context, identifyOptions);

expect(platform.requests.createEventSource).not.toHaveBeenCalled();
expect(platform.requests.fetch).not.toHaveBeenCalled();
expect(logger.debug).toHaveBeenCalledWith(
'[MobileDataManager] Identify called after data manager was closed.',
);
});

it('should load cached flags and resolve the identify', async () => {
const context = Context.fromLDContext({ kind: 'user', key: 'test-user' });
const identifyOptions: LDIdentifyOptions = { waitForNetworkResults: false };
Expand Down Expand Up @@ -299,4 +320,25 @@ describe('given a MobileDataManager with mocked dependencies', () => {
expect(identifyResolve).toHaveBeenCalled();
expect(identifyReject).not.toHaveBeenCalled();
});

it('closes the event source when the data manager is closed', async () => {
const context = Context.fromLDContext({ kind: 'user', key: 'test-user' });
const identifyOptions: LDIdentifyOptions = { waitForNetworkResults: false };
const identifyResolve = jest.fn();
const identifyReject = jest.fn();

await mobileDataManager.identify(identifyResolve, identifyReject, context, identifyOptions);
expect(platform.requests.createEventSource).toHaveBeenCalled();

mobileDataManager.close();
expect(eventSourceCloseMethod).toHaveBeenCalled();

// Verify a subsequent identify doesn't create a new event source
await mobileDataManager.identify(identifyResolve, identifyReject, context, identifyOptions);
expect(platform.requests.createEventSource).toHaveBeenCalledTimes(1);

expect(logger.debug).toHaveBeenCalledWith(
'[MobileDataManager] Identify called after data manager was closed.',
);
});
});
9 changes: 9 additions & 0 deletions packages/sdk/react-native/src/MobileDataManager.ts
Original file line number Diff line number Diff line change
Expand Up @@ -58,6 +58,10 @@ export default class MobileDataManager extends BaseDataManager {
context: Context,
identifyOptions?: LDIdentifyOptions,
): Promise<void> {
if (this.closed) {
this._debugLog('Identify called after data manager was closed..');
return;
}
this.context = context;
const offline = this.connectionMode === 'offline';
// In offline mode we do not support waiting for results.
Expand Down Expand Up @@ -140,6 +144,11 @@ export default class MobileDataManager extends BaseDataManager {
}

async setConnectionMode(mode: ConnectionMode): Promise<void> {
if (this.closed) {
this._debugLog('setting connection mode after data manager was closed');
return;
}

if (this.connectionMode === mode) {
this._debugLog(`setConnectionMode ignored. Mode is already '${mode}'.`);
return;
Expand Down
17 changes: 17 additions & 0 deletions packages/shared/sdk-client/__tests__/LDClientImpl.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -413,4 +413,21 @@ describe('sdk-client object', () => {
}),
);
});

test('closes event source when client is closed', async () => {
const carContext: LDContext = { kind: 'car', key: 'test-car' };

const mockCreateEventSource = jest.fn((streamUri: string = '', options: any = {}) => {
mockEventSource = new MockEventSource(streamUri, options);
mockEventSource.simulateEvents('put', [{ data: JSON.stringify(defaultPutResponse) }]);
return mockEventSource;
});
mockPlatform.requests.createEventSource = mockCreateEventSource;

await ldc.identify(carContext);
expect(mockEventSource.closed).toBe(false);

await ldc.close();
expect(mockEventSource.closed).toBe(true);
});
});
11 changes: 11 additions & 0 deletions packages/shared/sdk-client/src/DataManager.ts
Original file line number Diff line number Diff line change
Expand Up @@ -43,6 +43,11 @@ export interface DataManager {
context: Context,
identifyOptions?: LDIdentifyOptions,
): Promise<void>;

/**
* Closes the data manager. Any active connections are closed.
*/
close(): void;
}

/**
Expand All @@ -69,6 +74,7 @@ export abstract class BaseDataManager implements DataManager {
private _connectionParams?: ConnectionParams;
protected readonly dataSourceStatusManager: DataSourceStatusManager;
private readonly _dataSourceEventHandler: DataSourceEventHandler;
protected closed = false;

constructor(
protected readonly platform: Platform,
Expand Down Expand Up @@ -221,4 +227,9 @@ export abstract class BaseDataManager implements DataManager {
},
};
}

public close() {
this.updateProcessor?.close();
this.closed = true;
}
}
4 changes: 1 addition & 3 deletions packages/shared/sdk-client/src/LDClientImpl.ts
Original file line number Diff line number Diff line change
Expand Up @@ -11,7 +11,6 @@ import {
LDHeaders,
LDLogger,
Platform,
subsystem,
timedPromise,
TypeValidators,
} from '@launchdarkly/js-sdk-common';
Expand Down Expand Up @@ -48,7 +47,6 @@ export default class LDClientImpl implements LDClient {
private readonly _diagnosticsManager?: internal.DiagnosticsManager;
private _eventProcessor?: internal.EventProcessor;
readonly logger: LDLogger;
private _updateProcessor?: subsystem.LDStreamProcessor;

private readonly _highTimeoutThreshold: number = 15;

Expand Down Expand Up @@ -153,7 +151,7 @@ export default class LDClientImpl implements LDClient {
async close(): Promise<void> {
await this.flush();
this._eventProcessor?.close();
this._updateProcessor?.close();
this.dataManager.close();
this.logger.debug('Closed event processor and data source.');
}

Expand Down

0 comments on commit 37502fc

Please sign in to comment.