From 01b47aeb7f1fe3124a30611f17b5f3f2d0ca8f2c Mon Sep 17 00:00:00 2001 From: pgautier404 Date: Fri, 19 Apr 2024 19:05:25 -0500 Subject: [PATCH] fix: instantiate middleware after cred provider (#1241) * fix: instantiate middleware after cred provider This works around an issue that left background logging tasks from the middleware running, preventing jest from exiting. This is a workaround until a better solution for notifying the middlewares that they should start logging after successful client instantaiation can be implemented. * chore: explicitly close middlewares in catch block * chore: init middleware logging from client constructors * chore: map -> forEach * chore: comments and error handling for logging middleware --- .../client-sdk-nodejs/src/cache-client.ts | 43 +++++++++++++------ ...imental-active-request-count-middleware.ts | 3 ++ ...experimental-event-loop-perf-middleware.ts | 3 ++ .../src/config/middleware/middleware.ts | 1 + .../src/preview-leaderboard-client.ts | 12 ++++++ 5 files changed, 49 insertions(+), 13 deletions(-) diff --git a/packages/client-sdk-nodejs/src/cache-client.ts b/packages/client-sdk-nodejs/src/cache-client.ts index 5064a823a..88765af80 100644 --- a/packages/client-sdk-nodejs/src/cache-client.ts +++ b/packages/client-sdk-nodejs/src/cache-client.ts @@ -79,6 +79,18 @@ export class CacheClient extends AbstractCacheClient implements ICacheClient { this.logger = configuration.getLoggerFactory().getLogger(this); this.logger.debug('Creating Momento CacheClient'); + + // Initialize middlewares that have init methods. These currently start + // background tasks for logging that will execute until they are explicitly + // stopped. This is usually handled by the client's close method, but if + // there is ever a chance that this client constructor may fail after these + // methods are called, it is up to you to catch the exception and call close + // on each of these manually. + this._configuration.getMiddlewares().forEach(m => { + if (m.init) { + m.init(); + } + }); } public close() { @@ -87,7 +99,7 @@ export class CacheClient extends AbstractCacheClient implements ICacheClient { } this.controlClient.close(); this.dataClients.map(dc => dc.close()); - this._configuration.getMiddlewares().map(m => { + this._configuration.getMiddlewares().forEach(m => { if (m.close) { m.close(); } @@ -102,19 +114,24 @@ export class CacheClient extends AbstractCacheClient implements ICacheClient { */ static async create(props: EagerCacheClientProps): Promise { const client = new CacheClient(props); - const timeout = - props.eagerConnectTimeout !== undefined - ? props.eagerConnectTimeout - : EAGER_CONNECTION_DEFAULT_TIMEOUT_SECONDS; - // eslint-disable-next-line @typescript-eslint/no-unsafe-call - validateTimeout(timeout); - // client need to explicitly set the value as 0 to disable eager connection. - if (props.eagerConnectTimeout !== 0) { - await Promise.all( - client.dataClients.map(dc => (dc as CacheDataClient).connect(timeout)) - ); + try { + const timeout = + props.eagerConnectTimeout !== undefined + ? props.eagerConnectTimeout + : EAGER_CONNECTION_DEFAULT_TIMEOUT_SECONDS; + // eslint-disable-next-line @typescript-eslint/no-unsafe-call + validateTimeout(timeout); + // client need to explicitly set the value as 0 to disable eager connection. + if (props.eagerConnectTimeout !== 0) { + await Promise.all( + client.dataClients.map(dc => (dc as CacheDataClient).connect(timeout)) + ); + } + return client; + } catch (e) { + client.close(); + throw e; } - return client; } /** diff --git a/packages/client-sdk-nodejs/src/config/middleware/experimental-active-request-count-middleware.ts b/packages/client-sdk-nodejs/src/config/middleware/experimental-active-request-count-middleware.ts index 5cfec3b3c..b76ed08c5 100644 --- a/packages/client-sdk-nodejs/src/config/middleware/experimental-active-request-count-middleware.ts +++ b/packages/client-sdk-nodejs/src/config/middleware/experimental-active-request-count-middleware.ts @@ -49,6 +49,9 @@ export class ExperimentalActiveRequestCountLoggingMiddleware extends Experimenta c ) ); + } + + init() { if (!this.isLoggingStarted) { this.isLoggingStarted = true; this.startLogging(); diff --git a/packages/client-sdk-nodejs/src/config/middleware/experimental-event-loop-perf-middleware.ts b/packages/client-sdk-nodejs/src/config/middleware/experimental-event-loop-perf-middleware.ts index beff65481..112cd03bc 100644 --- a/packages/client-sdk-nodejs/src/config/middleware/experimental-event-loop-perf-middleware.ts +++ b/packages/client-sdk-nodejs/src/config/middleware/experimental-event-loop-perf-middleware.ts @@ -116,6 +116,9 @@ export class ExperimentalEventLoopPerformanceMetricsMiddleware constructor(loggerFactory: MomentoLoggerFactory) { this.logger = loggerFactory.getLogger(this); + } + + init() { if (!this.isLoggingStarted) { this.isLoggingStarted = true; this.startLogging(); diff --git a/packages/client-sdk-nodejs/src/config/middleware/middleware.ts b/packages/client-sdk-nodejs/src/config/middleware/middleware.ts index 6dcbd0cd3..b993cfbb5 100644 --- a/packages/client-sdk-nodejs/src/config/middleware/middleware.ts +++ b/packages/client-sdk-nodejs/src/config/middleware/middleware.ts @@ -62,5 +62,6 @@ export interface Middleware { onNewRequest( context?: MiddlewareRequestHandlerContext ): MiddlewareRequestHandler; + init?(): void; close?(): void; } diff --git a/packages/client-sdk-nodejs/src/preview-leaderboard-client.ts b/packages/client-sdk-nodejs/src/preview-leaderboard-client.ts index 81d006156..4dab8f84c 100644 --- a/packages/client-sdk-nodejs/src/preview-leaderboard-client.ts +++ b/packages/client-sdk-nodejs/src/preview-leaderboard-client.ts @@ -36,6 +36,18 @@ export class PreviewLeaderboardClient implements ILeaderboardClient { this.logger = configuration.getLoggerFactory().getLogger(this); this.logger.debug('Creating Momento LeaderboardClient'); this.dataClient = new LeaderboardDataClient(propsWithConfig, '0'); // only creating one leaderboard client + + // Initialize middlewares that have init methods. These currently start + // background tasks for logging that will execute until they are explicitly + // stopped. This is usually handled by the client's close method, but if + // there is ever a chance that this client constructor may fail after these + // methods are called, it is up to you to catch the exception and call close + // on each of these manually. + this.configuration.getMiddlewares().forEach(m => { + if (m.init) { + m.init(); + } + }); } public close() {