Skip to content

Commit 2cc7483

Browse files
thomaspoignantthomaspoignant
authored
fix(go-feature-flag-web): avoid infinite loop in waitWebsocketFinalSt… (#1104)
Signed-off-by: Thomas Poignant <[email protected]>
1 parent 8069e41 commit 2cc7483

File tree

2 files changed

+112
-54
lines changed

2 files changed

+112
-54
lines changed

‎libs/providers/go-feature-flag-web/src/lib/go-feature-flag-web-provider.spec.ts‎

Lines changed: 93 additions & 46 deletions
Original file line numberDiff line numberDiff line change
@@ -89,11 +89,11 @@ describe('GoFeatureFlagWebProvider', () => {
8989
const logger = new TestLogger();
9090

9191
beforeEach(async () => {
92-
await WS.clean();
92+
WS.clean();
9393
await OpenFeature.close();
9494
fetchMock.mockClear();
9595
fetchMock.mockReset();
96-
await jest.resetAllMocks();
96+
jest.resetAllMocks();
9797
websocketMockServer = new WS(websocketEndpoint, { jsonProtocol: true });
9898
fetchMock.post(allFlagsEndpoint, defaultAllFlagResponse);
9999
fetchMock.post(dataCollectorEndpoint, 200);
@@ -109,14 +109,14 @@ describe('GoFeatureFlagWebProvider', () => {
109109
});
110110

111111
afterEach(async () => {
112-
await WS.clean();
112+
WS.clean();
113113
websocketMockServer.close();
114114
await OpenFeature.close();
115-
await OpenFeature.clearHooks();
115+
OpenFeature.clearHooks();
116116
fetchMock.mockClear();
117117
fetchMock.mockReset();
118118
await defaultProvider?.onClose();
119-
await jest.resetAllMocks();
119+
jest.resetAllMocks();
120120
readyHandler.mockReset();
121121
errorHandler.mockReset();
122122
configurationChangedHandler.mockReset();
@@ -145,8 +145,8 @@ describe('GoFeatureFlagWebProvider', () => {
145145
describe('flag evaluation', () => {
146146
it('should change evaluation value if context has changed', async () => {
147147
await OpenFeature.setContext(defaultContext);
148-
OpenFeature.setProvider('test-provider', defaultProvider);
149-
const client = await OpenFeature.getClient('test-provider');
148+
await OpenFeature.setProviderAndWait('test-provider', defaultProvider);
149+
const client = OpenFeature.getClient('test-provider');
150150
await websocketMockServer.connected;
151151
await new Promise((resolve) => setTimeout(resolve, 5));
152152

@@ -168,11 +168,11 @@ describe('GoFeatureFlagWebProvider', () => {
168168
await OpenFeature.setContext(defaultContext);
169169
const providerName = expect.getState().currentTestName || 'test';
170170
OpenFeature.setProvider(providerName, newDefaultProvider());
171-
const client = await OpenFeature.getClient(providerName);
171+
const client = OpenFeature.getClient(providerName);
172172
await websocketMockServer.connected;
173173
// Need to wait before using the mock
174174
await new Promise((resolve) => setTimeout(resolve, 5));
175-
await websocketMockServer.close();
175+
websocketMockServer.close();
176176

177177
const got = client.getBooleanDetails('bool_flag', false);
178178
expect(got.reason).toEqual(StandardResolutionReasons.CACHED);
@@ -183,11 +183,11 @@ describe('GoFeatureFlagWebProvider', () => {
183183
const providerName = expect.getState().currentTestName || 'test';
184184
await OpenFeature.setContext(defaultContext);
185185
OpenFeature.setProvider(providerName, newDefaultProvider());
186-
const client = await OpenFeature.getClient(providerName);
186+
const client = OpenFeature.getClient(providerName);
187187
client.addHandler(ProviderEvents.Error, errorHandler);
188188
// wait the event to be triggered
189189
await new Promise((resolve) => setTimeout(resolve, 5));
190-
expect(errorHandler).toBeCalled();
190+
expect(errorHandler).toHaveBeenCalled();
191191
expect(logger.inMemoryLogger['error'][0]).toEqual(
192192
'GoFeatureFlagWebProvider: invalid token used to contact GO Feature Flag instance: Error: Request failed with status code 401',
193193
);
@@ -196,15 +196,15 @@ describe('GoFeatureFlagWebProvider', () => {
196196
it('should emit an error if we receive a 404 from GO Feature Flag', async () => {
197197
fetchMock.post(allFlagsEndpoint, 404, { overwriteRoutes: true });
198198
await OpenFeature.setContext(defaultContext);
199-
OpenFeature.setProvider('test-provider', defaultProvider);
200-
const client = await OpenFeature.getClient('test-provider');
199+
await OpenFeature.setProviderAndWait('test-provider', defaultProvider);
200+
const client = OpenFeature.getClient('test-provider');
201201
client.addHandler(ProviderEvents.Ready, readyHandler);
202202
client.addHandler(ProviderEvents.Error, errorHandler);
203203
client.addHandler(ProviderEvents.Stale, staleHandler);
204204
client.addHandler(ProviderEvents.ConfigurationChanged, configurationChangedHandler);
205205
// wait the event to be triggered
206206
await new Promise((resolve) => setTimeout(resolve, 5));
207-
expect(errorHandler).toBeCalled();
207+
expect(errorHandler).toHaveBeenCalled();
208208
expect(logger.inMemoryLogger['error'][0]).toEqual(
209209
'GoFeatureFlagWebProvider: impossible to call go-feature-flag relay proxy Error: Request failed with status code 404',
210210
);
@@ -214,7 +214,7 @@ describe('GoFeatureFlagWebProvider', () => {
214214
const flagKey = 'bool_flag';
215215
await OpenFeature.setContext(defaultContext);
216216
await OpenFeature.setProviderAndWait('test-provider', defaultProvider);
217-
const client = await OpenFeature.getClient('test-provider');
217+
const client = OpenFeature.getClient('test-provider');
218218
await websocketMockServer.connected;
219219
const got = client.getBooleanDetails(flagKey, false);
220220
const want: EvaluationDetails<boolean> = {
@@ -233,7 +233,7 @@ describe('GoFeatureFlagWebProvider', () => {
233233
const flagKey = 'string_flag';
234234
await OpenFeature.setContext(defaultContext);
235235
await OpenFeature.setProviderAndWait('test-provider', defaultProvider);
236-
const client = await OpenFeature.getClient('test-provider');
236+
const client = OpenFeature.getClient('test-provider');
237237
await websocketMockServer.connected;
238238
const got = client.getStringDetails(flagKey, 'false');
239239
const want: EvaluationDetails<string> = {
@@ -252,7 +252,7 @@ describe('GoFeatureFlagWebProvider', () => {
252252
const flagKey = 'number_flag';
253253
await OpenFeature.setContext(defaultContext);
254254
await OpenFeature.setProviderAndWait('test-provider', defaultProvider);
255-
const client = await OpenFeature.getClient('test-provider');
255+
const client = OpenFeature.getClient('test-provider');
256256
await websocketMockServer.connected;
257257
const got = client.getNumberDetails(flagKey, 456);
258258
const want: EvaluationDetails<number> = {
@@ -271,7 +271,7 @@ describe('GoFeatureFlagWebProvider', () => {
271271
const flagKey = 'object_flag';
272272
await OpenFeature.setContext(defaultContext);
273273
await OpenFeature.setProviderAndWait('test-provider', defaultProvider);
274-
const client = await OpenFeature.getClient('test-provider');
274+
const client = OpenFeature.getClient('test-provider');
275275
await websocketMockServer.connected;
276276
const got = client.getObjectDetails(flagKey, { error: true });
277277
const want: EvaluationDetails<JsonValue> = {
@@ -290,7 +290,7 @@ describe('GoFeatureFlagWebProvider', () => {
290290
const flagKey = 'bool_flag';
291291
await OpenFeature.setContext(defaultContext);
292292
await OpenFeature.setProviderAndWait('test-provider', defaultProvider);
293-
const client = await OpenFeature.getClient('test-provider');
293+
const client = OpenFeature.getClient('test-provider');
294294
await websocketMockServer.connected;
295295
const got = client.getStringDetails(flagKey, 'false');
296296
const want: EvaluationDetails<string> = {
@@ -308,7 +308,7 @@ describe('GoFeatureFlagWebProvider', () => {
308308
const flagKey = 'not-exist';
309309
await OpenFeature.setContext(defaultContext);
310310
await OpenFeature.setProviderAndWait('test-provider', defaultProvider);
311-
const client = await OpenFeature.getClient('test-provider');
311+
const client = OpenFeature.getClient('test-provider');
312312
await websocketMockServer.connected;
313313
const got = client.getBooleanDetails(flagKey, false);
314314
const want: EvaluationDetails<boolean> = {
@@ -354,8 +354,8 @@ describe('GoFeatureFlagWebProvider', () => {
354354
describe('eventing', () => {
355355
it('should call client handler with ProviderEvents.Ready when websocket is connected', async () => {
356356
// await OpenFeature.setContext(defaultContext); // we deactivate this call because the context is already set, and we want to avoid calling contextChanged function
357-
OpenFeature.setProvider('test-provider', defaultProvider);
358-
const client = await OpenFeature.getClient('test-provider');
357+
await OpenFeature.setProviderAndWait('test-provider', defaultProvider);
358+
const client = OpenFeature.getClient('test-provider');
359359
client.addHandler(ProviderEvents.Ready, readyHandler);
360360
client.addHandler(ProviderEvents.Error, errorHandler);
361361
client.addHandler(ProviderEvents.Stale, staleHandler);
@@ -365,16 +365,16 @@ describe('GoFeatureFlagWebProvider', () => {
365365
await websocketMockServer.connected;
366366
await new Promise((resolve) => setTimeout(resolve, 5));
367367

368-
expect(readyHandler).toBeCalled();
369-
expect(errorHandler).not.toBeCalled();
370-
expect(configurationChangedHandler).not.toBeCalled();
371-
expect(staleHandler).not.toBeCalled();
368+
expect(readyHandler).toHaveBeenCalled();
369+
expect(errorHandler).not.toHaveBeenCalled();
370+
expect(configurationChangedHandler).not.toHaveBeenCalled();
371+
expect(staleHandler).not.toHaveBeenCalled();
372372
});
373373

374374
it('should call client handler with ProviderEvents.ConfigurationChanged when websocket is sending update', async () => {
375375
// await OpenFeature.setContext(defaultContext); // we deactivate this call because the context is already set, and we want to avoid calling contextChanged function
376-
OpenFeature.setProvider('test-provider', defaultProvider);
377-
const client = await OpenFeature.getClient('test-provider');
376+
await OpenFeature.setProviderAndWait('test-provider', defaultProvider);
377+
const client = OpenFeature.getClient('test-provider');
378378

379379
client.addHandler(ProviderEvents.Ready, readyHandler);
380380
client.addHandler(ProviderEvents.Error, errorHandler);
@@ -403,10 +403,10 @@ describe('GoFeatureFlagWebProvider', () => {
403403
// waiting the call to the API to be successful
404404
await new Promise((resolve) => setTimeout(resolve, 50));
405405

406-
expect(readyHandler).toBeCalled();
407-
expect(errorHandler).not.toBeCalled();
408-
expect(configurationChangedHandler).toBeCalled();
409-
expect(staleHandler).not.toBeCalled();
406+
expect(readyHandler).toHaveBeenCalled();
407+
expect(errorHandler).not.toHaveBeenCalled();
408+
expect(configurationChangedHandler).toHaveBeenCalled();
409+
expect(staleHandler).not.toHaveBeenCalled();
410410
expect(configurationChangedHandler.mock.calls[0][0]).toEqual({
411411
clientName: 'test-provider',
412412
domain: 'test-provider',
@@ -433,8 +433,8 @@ describe('GoFeatureFlagWebProvider', () => {
433433
},
434434
logger,
435435
);
436-
OpenFeature.setProvider('test-provider', provider);
437-
const client = await OpenFeature.getClient('test-provider');
436+
await OpenFeature.setProviderAndWait('test-provider', provider);
437+
const client = OpenFeature.getClient('test-provider');
438438
client.addHandler(ProviderEvents.Ready, readyHandler);
439439
client.addHandler(ProviderEvents.Error, errorHandler);
440440
client.addHandler(ProviderEvents.Stale, staleHandler);
@@ -444,14 +444,14 @@ describe('GoFeatureFlagWebProvider', () => {
444444
await websocketMockServer.connected;
445445

446446
// Need to wait before using the mock
447-
await new Promise((resolve) => setTimeout(resolve, 5));
448-
await websocketMockServer.close();
447+
await new Promise((resolve) => setTimeout(resolve, 50));
448+
websocketMockServer.close();
449449
await new Promise((resolve) => setTimeout(resolve, 300));
450450

451-
expect(readyHandler).toBeCalled();
452-
expect(errorHandler).not.toBeCalled();
453-
expect(configurationChangedHandler).not.toBeCalled();
454-
expect(staleHandler).toBeCalled();
451+
expect(readyHandler).toHaveBeenCalled();
452+
expect(errorHandler).not.toHaveBeenCalled();
453+
expect(configurationChangedHandler).not.toHaveBeenCalled();
454+
expect(staleHandler).toHaveBeenCalled();
455455
});
456456
});
457457

@@ -470,7 +470,7 @@ describe('GoFeatureFlagWebProvider', () => {
470470
logger,
471471
);
472472

473-
OpenFeature.setProvider(clientName, p);
473+
await OpenFeature.setProviderAndWait(clientName, p);
474474
const client = OpenFeature.getClient(clientName);
475475
await websocketMockServer.connected;
476476
await new Promise((resolve) => setTimeout(resolve, 5));
@@ -501,7 +501,7 @@ describe('GoFeatureFlagWebProvider', () => {
501501
logger,
502502
);
503503

504-
OpenFeature.setProvider(clientName, p);
504+
await OpenFeature.setProviderAndWait(clientName, p);
505505
const client = OpenFeature.getClient(clientName);
506506
await websocketMockServer.connected;
507507
await new Promise((resolve) => setTimeout(resolve, 5));
@@ -531,7 +531,7 @@ describe('GoFeatureFlagWebProvider', () => {
531531
logger,
532532
);
533533

534-
OpenFeature.setProvider(clientName, p);
534+
await OpenFeature.setProviderAndWait(clientName, p);
535535
const client = OpenFeature.getClient(clientName);
536536
await websocketMockServer.connected;
537537
await new Promise((resolve) => setTimeout(resolve, 5));
@@ -559,7 +559,7 @@ describe('GoFeatureFlagWebProvider', () => {
559559
logger,
560560
);
561561

562-
OpenFeature.setProvider(clientName, p);
562+
await OpenFeature.setProviderAndWait(clientName, p);
563563
const client = OpenFeature.getClient(clientName);
564564
await websocketMockServer.connected;
565565
await new Promise((resolve) => setTimeout(resolve, 5));
@@ -574,7 +574,7 @@ describe('GoFeatureFlagWebProvider', () => {
574574
it('should have a log when data collector is not available', async () => {
575575
const clientName = expect.getState().currentTestName ?? 'test-provider';
576576
fetchMock.post(dataCollectorEndpoint, 500, { overwriteRoutes: true });
577-
OpenFeature.setContext(defaultContext);
577+
await OpenFeature.setContext(defaultContext);
578578
const p = new GoFeatureFlagWebProvider(
579579
{
580580
endpoint: endpoint,
@@ -585,7 +585,7 @@ describe('GoFeatureFlagWebProvider', () => {
585585
logger,
586586
);
587587

588-
OpenFeature.setProvider(clientName, p);
588+
await OpenFeature.setProviderAndWait(clientName, p);
589589
const client = OpenFeature.getClient(clientName);
590590
await websocketMockServer.connected;
591591
await new Promise((resolve) => setTimeout(resolve, 5));
@@ -606,4 +606,51 @@ describe('GoFeatureFlagWebProvider', () => {
606606
await OpenFeature.close();
607607
});
608608
});
609+
it('should resolve when WebSocket is open', async () => {
610+
const provider = new GoFeatureFlagWebProvider({ endpoint: 'http://localhost:1031', apiTimeout: 1000 });
611+
await provider.initialize({ targetingKey: 'user-key' });
612+
const websocket = new WebSocket(websocketEndpoint);
613+
await websocketMockServer.connected;
614+
await expect(provider.waitWebsocketFinalStatus(websocket)).resolves.toBeUndefined();
615+
});
616+
617+
// how can I mock a websocket server to stay in CONNECTING state
618+
it('should timeout if websocket stay in CONNECTING state', async () => {
619+
const provider = new GoFeatureFlagWebProvider({ endpoint: 'http://localhost:1031', apiTimeout: 1000 });
620+
await provider.initialize({ targetingKey: 'user-key' });
621+
const websocket = new MockWebSocketConnectingState(websocketEndpoint);
622+
623+
// Now you can test the behavior when the WebSocket is in CONNECTING state
624+
await expect(provider.waitWebsocketFinalStatus(websocket)).rejects.toBe(
625+
'timeout of 1000 ms reached when initializing the websocket',
626+
);
627+
});
609628
});
629+
630+
class MockWebSocketConnectingState extends WebSocket {
631+
constructor(url: string, protocols?: string | string[]) {
632+
super(url, protocols);
633+
}
634+
635+
get readyState() {
636+
return WebSocket.CONNECTING;
637+
}
638+
639+
set onopen(_: { (this: WebSocket, event: Event): void; (): void }) {
640+
// Do nothing to prevent setting the onopen handler
641+
}
642+
643+
set onclose(_: { (): Promise<void>; (): void }) {
644+
// Do nothing to prevent setting the onclose handler
645+
}
646+
647+
addEventListener(
648+
type: string,
649+
listener: EventListenerOrEventListenerObject,
650+
options?: boolean | AddEventListenerOptions,
651+
): void {
652+
if (type !== 'open' && type !== 'close') {
653+
super.addEventListener(type, listener, options);
654+
}
655+
}
656+
}

‎libs/providers/go-feature-flag-web/src/lib/go-feature-flag-web-provider.ts‎

Lines changed: 19 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -106,7 +106,9 @@ export class GoFeatureFlagWebProvider implements Provider {
106106
this._logger?.debug(`${GoFeatureFlagWebProvider.name}: Trying to connect the websocket at ${wsURL}`);
107107

108108
this._websocket = new WebSocket(wsURL);
109-
await this.waitWebsocketFinalStatus(this._websocket);
109+
await this.waitWebsocketFinalStatus(this._websocket).catch((reason) => {
110+
throw new Error(`impossible to connect to the websocket: ${reason}`);
111+
});
110112

111113
this._websocket.onopen = (event) => {
112114
this._logger?.info(`${GoFeatureFlagWebProvider.name}: Websocket to go-feature-flag open: ${event}`);
@@ -133,15 +135,24 @@ export class GoFeatureFlagWebProvider implements Provider {
133135
* @param socket - the websocket you are waiting for
134136
*/
135137
waitWebsocketFinalStatus(socket: WebSocket): Promise<void> {
136-
return new Promise((resolve) => {
137-
const checkConnection = () => {
138-
if (socket.readyState === WebSocket.OPEN || socket.readyState === WebSocket.CLOSED) {
139-
return resolve();
138+
return new Promise((resolve, reject) => {
139+
// wait until the socket is in a stable state or until the timeout is reached
140+
const websocketTimeout = this._apiTimeout !== 0 ? this._apiTimeout : 5000;
141+
const timeout = setTimeout(() => {
142+
if (socket.readyState !== WebSocket.OPEN && socket.readyState !== WebSocket.CLOSED) {
143+
reject(`timeout of ${websocketTimeout} ms reached when initializing the websocket`);
140144
}
141-
// Wait 5 milliseconds before checking again
142-
setTimeout(checkConnection, 5);
145+
}, websocketTimeout);
146+
147+
socket.onopen = () => {
148+
clearTimeout(timeout);
149+
resolve();
150+
};
151+
152+
socket.onclose = () => {
153+
clearTimeout(timeout);
154+
resolve();
143155
};
144-
checkConnection();
145156
});
146157
}
147158

0 commit comments

Comments
 (0)