From 2736cfd90f535b7c3b05335eab26a01c4b4b9799 Mon Sep 17 00:00:00 2001 From: Ryan Lamb <4955475+kinyoklion@users.noreply.github.com> Date: Wed, 5 Feb 2025 14:59:29 -0800 Subject: [PATCH] fix: Ensure streaming connection is closed on SDK close. --- .../__tests__/BrowserDataManager.test.ts | 32 +++++++++++++- .../sdk/browser/src/BrowserDataManager.ts | 5 +++ .../__tests__/MobileDataManager.test.ts | 44 ++++++++++++++++++- .../sdk/react-native/src/MobileDataManager.ts | 9 ++++ .../sdk-client/__tests__/LDClientImpl.test.ts | 17 +++++++ packages/shared/sdk-client/src/DataManager.ts | 11 +++++ .../shared/sdk-client/src/LDClientImpl.ts | 4 +- 7 files changed, 117 insertions(+), 5 deletions(-) diff --git a/packages/sdk/browser/__tests__/BrowserDataManager.test.ts b/packages/sdk/browser/__tests__/BrowserDataManager.test.ts index 628602acf7..c467ef623a 100644 --- a/packages/sdk/browser/__tests__/BrowserDataManager.test.ts +++ b/packages/sdk/browser/__tests__/BrowserDataManager.test.ts @@ -63,6 +63,8 @@ describe('given a BrowserDataManager with mocked dependencies', () => { let diagnosticsManager: jest.Mocked; let dataManager: BrowserDataManager; let logger: LDLogger; + let eventSourceCloseMethod: jest.Mock; + beforeEach(() => { logger = { error: jest.fn(), @@ -70,6 +72,7 @@ describe('given a BrowserDataManager with mocked dependencies', () => { info: jest.fn(), debug: jest.fn(), }; + eventSourceCloseMethod = jest.fn(); config = { logger, maxCachedContexts: 5, @@ -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(), @@ -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.', + ); + }); }); diff --git a/packages/sdk/browser/src/BrowserDataManager.ts b/packages/sdk/browser/src/BrowserDataManager.ts index 00f777f5e5..c25b3d6a49 100644 --- a/packages/sdk/browser/src/BrowserDataManager.ts +++ b/packages/sdk/browser/src/BrowserDataManager.ts @@ -74,6 +74,11 @@ export default class BrowserDataManager extends BaseDataManager { context: Context, identifyOptions?: LDIdentifyOptions, ): Promise { + 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) { diff --git a/packages/sdk/react-native/__tests__/MobileDataManager.test.ts b/packages/sdk/react-native/__tests__/MobileDataManager.test.ts index 27ac0de488..c1f056a68f 100644 --- a/packages/sdk/react-native/__tests__/MobileDataManager.test.ts +++ b/packages/sdk/react-native/__tests__/MobileDataManager.test.ts @@ -54,6 +54,8 @@ describe('given a MobileDataManager with mocked dependencies', () => { let diagnosticsManager: jest.Mocked; let mobileDataManager: MobileDataManager; let logger: LDLogger; + let eventSourceCloseMethod: jest.Mock; + beforeEach(() => { logger = { error: jest.fn(), @@ -61,6 +63,8 @@ describe('given a MobileDataManager with mocked dependencies', () => { info: jest.fn(), debug: jest.fn(), }; + eventSourceCloseMethod = jest.fn(); + config = { logger, maxCachedContexts: 5, @@ -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(), @@ -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 }; @@ -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.', + ); + }); }); diff --git a/packages/sdk/react-native/src/MobileDataManager.ts b/packages/sdk/react-native/src/MobileDataManager.ts index bf50fdea00..1f1d0357b6 100644 --- a/packages/sdk/react-native/src/MobileDataManager.ts +++ b/packages/sdk/react-native/src/MobileDataManager.ts @@ -58,6 +58,10 @@ export default class MobileDataManager extends BaseDataManager { context: Context, identifyOptions?: LDIdentifyOptions, ): Promise { + 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. @@ -140,6 +144,11 @@ export default class MobileDataManager extends BaseDataManager { } async setConnectionMode(mode: ConnectionMode): Promise { + 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; diff --git a/packages/shared/sdk-client/__tests__/LDClientImpl.test.ts b/packages/shared/sdk-client/__tests__/LDClientImpl.test.ts index b760e94104..2241c81c00 100644 --- a/packages/shared/sdk-client/__tests__/LDClientImpl.test.ts +++ b/packages/shared/sdk-client/__tests__/LDClientImpl.test.ts @@ -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); + }); }); diff --git a/packages/shared/sdk-client/src/DataManager.ts b/packages/shared/sdk-client/src/DataManager.ts index d3b33de1bc..bca06a6830 100644 --- a/packages/shared/sdk-client/src/DataManager.ts +++ b/packages/shared/sdk-client/src/DataManager.ts @@ -43,6 +43,11 @@ export interface DataManager { context: Context, identifyOptions?: LDIdentifyOptions, ): Promise; + + /** + * Closes the data manager. Any active connections are closed. + */ + close(): void; } /** @@ -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, @@ -221,4 +227,9 @@ export abstract class BaseDataManager implements DataManager { }, }; } + + public close() { + this.updateProcessor?.close(); + this.closed = true; + } } diff --git a/packages/shared/sdk-client/src/LDClientImpl.ts b/packages/shared/sdk-client/src/LDClientImpl.ts index b61d0c5c49..df59fb7c44 100644 --- a/packages/shared/sdk-client/src/LDClientImpl.ts +++ b/packages/shared/sdk-client/src/LDClientImpl.ts @@ -11,7 +11,6 @@ import { LDHeaders, LDLogger, Platform, - subsystem, timedPromise, TypeValidators, } from '@launchdarkly/js-sdk-common'; @@ -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; @@ -153,7 +151,7 @@ export default class LDClientImpl implements LDClient { async close(): Promise { await this.flush(); this._eventProcessor?.close(); - this._updateProcessor?.close(); + this.dataManager.close(); this.logger.debug('Closed event processor and data source.'); }