Skip to content

Commit

Permalink
fix: instantiate middleware after cred provider (#1241)
Browse files Browse the repository at this point in the history
* 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
  • Loading branch information
pgautier404 authored Apr 20, 2024
1 parent 6c9c5fe commit 01b47ae
Show file tree
Hide file tree
Showing 5 changed files with 49 additions and 13 deletions.
43 changes: 30 additions & 13 deletions packages/client-sdk-nodejs/src/cache-client.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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() {
Expand All @@ -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();
}
Expand All @@ -102,19 +114,24 @@ export class CacheClient extends AbstractCacheClient implements ICacheClient {
*/
static async create(props: EagerCacheClientProps): Promise<CacheClient> {
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;
}

/**
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -49,6 +49,9 @@ export class ExperimentalActiveRequestCountLoggingMiddleware extends Experimenta
c
)
);
}

init() {
if (!this.isLoggingStarted) {
this.isLoggingStarted = true;
this.startLogging();
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -116,6 +116,9 @@ export class ExperimentalEventLoopPerformanceMetricsMiddleware

constructor(loggerFactory: MomentoLoggerFactory) {
this.logger = loggerFactory.getLogger(this);
}

init() {
if (!this.isLoggingStarted) {
this.isLoggingStarted = true;
this.startLogging();
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -62,5 +62,6 @@ export interface Middleware {
onNewRequest(
context?: MiddlewareRequestHandlerContext
): MiddlewareRequestHandler;
init?(): void;
close?(): void;
}
12 changes: 12 additions & 0 deletions packages/client-sdk-nodejs/src/preview-leaderboard-client.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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() {
Expand Down

0 comments on commit 01b47ae

Please sign in to comment.