diff --git a/src/lib/routes/admin-api/api-token.ts b/src/lib/routes/admin-api/api-token.ts index 8c6349ac15..ad282b6660 100644 --- a/src/lib/routes/admin-api/api-token.ts +++ b/src/lib/routes/admin-api/api-token.ts @@ -200,7 +200,7 @@ export class ApiTokenController extends Controller { const { token } = req.params; await this.apiTokenService.delete(token, extractUsername(req)); - this.proxyService.deleteClientForProxyToken(token); + await this.proxyService.deleteClientForProxyToken(token); res.status(200).end(); } diff --git a/src/lib/routes/admin-api/project/api-token.ts b/src/lib/routes/admin-api/project/api-token.ts index 7afe8242d1..2c8e007ec4 100644 --- a/src/lib/routes/admin-api/project/api-token.ts +++ b/src/lib/routes/admin-api/project/api-token.ts @@ -187,7 +187,7 @@ export class ProjectApiTokenController extends Controller { storedToken.project[0] === projectId)) ) { await this.apiTokenService.delete(token, extractUsername(req)); - this.proxyService.deleteClientForProxyToken(token); + await this.proxyService.deleteClientForProxyToken(token); res.status(200).end(); } } diff --git a/src/lib/services/proxy-service.ts b/src/lib/services/proxy-service.ts index 1490ed15b6..b8965fd0f0 100644 --- a/src/lib/services/proxy-service.ts +++ b/src/lib/services/proxy-service.ts @@ -42,7 +42,13 @@ export class ProxyService { private readonly services: Services; - private readonly clients: Map = new Map(); + /** + * This is intentionally a Promise becasue we want to be able to await + * until the client (which might be being created by a different request) is ready + * Check this test that fails if we don't use a Promise: src/test/e2e/api/proxy/proxy.concurrency.e2e.test.ts + */ + private readonly clients: Map> = + new Map(); private cachedFrontendSettings?: FrontendSettings; @@ -99,14 +105,13 @@ export class ProxyService { private async clientForProxyToken(token: ApiUser): Promise { ProxyService.assertExpectedTokenType(token); - if (!this.clients.has(token.secret)) { - this.clients.set( - token.secret, - await this.createClientForProxyToken(token), - ); + let client = this.clients.get(token.secret); + if (!client) { + client = this.createClientForProxyToken(token); + this.clients.set(token.secret, client); } - return this.clients.get(token.secret); + return client; } private async createClientForProxyToken(token: ApiUser): Promise { @@ -134,13 +139,17 @@ export class ProxyService { return client; } - deleteClientForProxyToken(secret: string): void { - this.clients.get(secret)?.destroy(); - this.clients.delete(secret); + async deleteClientForProxyToken(secret: string): Promise { + let clientPromise = this.clients.get(secret); + if (clientPromise) { + const client = await clientPromise; + client.destroy(); + this.clients.delete(secret); + } } stopAll(): void { - this.clients.forEach((client) => client.destroy()); + this.clients.forEach((promise) => promise.then((c) => c.destroy())); } private static assertExpectedTokenType({ type }: ApiUser) { diff --git a/src/test/e2e/api/proxy/proxy.concurrency.e2e.test.ts b/src/test/e2e/api/proxy/proxy.concurrency.e2e.test.ts new file mode 100644 index 0000000000..bc89dd722a --- /dev/null +++ b/src/test/e2e/api/proxy/proxy.concurrency.e2e.test.ts @@ -0,0 +1,64 @@ +import { IUnleashTest, setupAppWithAuth } from '../../helpers/test-helper'; +import dbInit, { ITestDb } from '../../helpers/database-init'; +import getLogger from '../../../fixtures/no-logger'; +import { randomId } from '../../../../lib/util'; +import { ApiTokenType } from '../../../../lib/types/models/api-token'; + +let app: IUnleashTest; +let db: ITestDb; +let appErrorLogs: string[] = []; + +beforeAll(async () => { + db = await dbInit(`proxy_concurrency`, getLogger); + const baseLogger = getLogger(); + const appLogger = { + ...baseLogger, + error: (msg: string, ...args: any[]) => { + appErrorLogs.push(msg); + baseLogger.error(msg, ...args); + }, + }; + app = await setupAppWithAuth(db.stores, { + frontendApiOrigins: ['https://example.com'], + getLogger: () => appLogger, + }); +}); + +afterEach(() => { + app.services.proxyService.stopAll(); + jest.clearAllMocks(); +}); + +afterAll(async () => { + await app.destroy(); + await db.destroy(); +}); + +beforeEach(async () => { + appErrorLogs = []; +}); + +/** + * This test needs to run on a new instance of the application and a clean DB + * which is why it should be the only test of this file + */ +test('multiple parallel calls to api/frontend should not create multiple instances', async () => { + const frontendTokenDefault = + await app.services.apiTokenService.createApiTokenWithProjects({ + type: ApiTokenType.FRONTEND, + projects: ['default'], + environment: 'default', + username: `test-token-${randomId()}`, + }); + + await Promise.all( + Array.from(Array(15).keys()).map(() => + app.request + .get('/api/frontend') + .set('Authorization', frontendTokenDefault.secret) + .expect('Content-Type', /json/) + .expect(200), + ), + ); + expect(appErrorLogs).toHaveLength(0); +});