From 6c109acf753062a4b02fa3d016980bbcef61e38e Mon Sep 17 00:00:00 2001 From: Marcos Passos Date: Thu, 12 Sep 2024 08:31:30 -0600 Subject: [PATCH 1/3] Fix timeout logs --- src/contentFetcher.ts | 12 ++--- src/evaluator.ts | 115 +++++++++++++++++++++-------------------- test/evaluator.test.ts | 33 +++++++++--- 3 files changed, 90 insertions(+), 70 deletions(-) diff --git a/src/contentFetcher.ts b/src/contentFetcher.ts index 93334e3..0f21008 100644 --- a/src/contentFetcher.ts +++ b/src/contentFetcher.ts @@ -129,14 +129,14 @@ export class ContentFetcher { } // Types for Browser and Node environment - let timeoutReference: number | NodeJS.Timeout | undefined; return new Promise>((resolve, reject) => { const abortController = new AbortController(); const timeout = options.timeout ?? this.configuration.defaultTimeout; + let timer: number | NodeJS.Timeout | undefined; if (timeout !== undefined) { - timeoutReference = setTimeout( + timer = setTimeout( () => { const response: ErrorResponse = { title: `Content could not be loaded in time for slot '${slotId}'.`, @@ -156,6 +156,7 @@ export class ContentFetcher { } this.load(slotId, abortController.signal, options) + .finally(() => clearTimeout(timer)) .then(response => { const region = response.headers.get('X-Croct-Region'); const timing = response.headers.get('X-Croct-Timing'); @@ -192,12 +193,7 @@ export class ContentFetcher { ); } }); - }) - .finally(() => { - // Once the fetch completes, regardless of outcome, cancel the timeout - // to avoid logging an error that didn't happen. - clearTimeout(timeoutReference); - }); + }); } private load(slotId: string, signal: AbortSignal, options: FetchOptions): Promise { diff --git a/src/evaluator.ts b/src/evaluator.ts index a980de5..c54ee8b 100644 --- a/src/evaluator.ts +++ b/src/evaluator.ts @@ -167,6 +167,8 @@ export class Evaluator { const abortController = new AbortController(); const timeout = options.timeout ?? this.configuration.defaultTimeout; + let timer: number | NodeJS.Timeout | undefined; + if (timeout !== undefined) { setTimeout( () => { @@ -187,63 +189,64 @@ export class Evaluator { ); } - const promise = this.fetch(payload, abortController.signal, options); - - promise.then( - response => { - const region = response.headers.get('X-Croct-Region'); - const timing = response.headers.get('X-Croct-Timing'); - - this.logger.debug( - `Evaluation of the query "${reference}" processed by region ${region} in ${timing}.`, - ); - - return response.json() - .then(body => { - if (response.ok) { - return resolve(body); - } - - this.logHelp(response.status); - - const problem: ErrorResponse = body; - - switch (problem.type) { - case EvaluationErrorType.INVALID_QUERY: - case EvaluationErrorType.EVALUATION_FAILED: - case EvaluationErrorType.TOO_COMPLEX_QUERY: - reject(new QueryError(problem as QueryErrorResponse)); - - break; - - default: - reject(new EvaluationError(problem)); - - break; - } - }) - .catch(error => { - if (!response.ok) { - throw new Error(`Error ${response.status} - ${response.statusText}`); - } - - throw error; - }); - }, - ).catch( - error => { - if (!abortController.signal.aborted) { - reject( - new EvaluationError({ - title: formatMessage(error), - type: EvaluationErrorType.UNEXPECTED_ERROR, - detail: 'Please try again or contact Croct support if the error persists.', - status: 500, // Internal Server Error - }), + this.fetch(payload, abortController.signal, options) + .finally(() => clearTimeout(timer)) + .then( + response => { + const region = response.headers.get('X-Croct-Region'); + const timing = response.headers.get('X-Croct-Timing'); + + this.logger.debug( + `Evaluation of the query "${reference}" processed by region ${region} in ${timing}.`, ); - } - }, - ); + + return response.json() + .then(body => { + if (response.ok) { + return resolve(body); + } + + this.logHelp(response.status); + + const problem: ErrorResponse = body; + + switch (problem.type) { + case EvaluationErrorType.INVALID_QUERY: + case EvaluationErrorType.EVALUATION_FAILED: + case EvaluationErrorType.TOO_COMPLEX_QUERY: + reject(new QueryError(problem as QueryErrorResponse)); + + break; + + default: + reject(new EvaluationError(problem)); + + break; + } + }) + .catch(error => { + if (!response.ok) { + throw new Error(`Error ${response.status} - ${response.statusText}`); + } + + throw error; + }); + }, + ) + .catch( + error => { + if (!abortController.signal.aborted) { + reject( + new EvaluationError({ + title: formatMessage(error), + type: EvaluationErrorType.UNEXPECTED_ERROR, + detail: 'Please try again or contact Croct support if the error persists.', + status: 500, // Internal Server Error + }), + ); + } + }, + ); }); } diff --git a/test/evaluator.test.ts b/test/evaluator.test.ts index 7c33362..7d036fa 100644 --- a/test/evaluator.test.ts +++ b/test/evaluator.test.ts @@ -277,9 +277,7 @@ describe('An evaluator', () => { fetchMock.mock({ ...requestMatcher, delay: 20, - response: { - result: 'Carol', - }, + response: JSON.stringify('Carol'), }); const promise = evaluator.evaluate(query, { @@ -315,9 +313,7 @@ describe('An evaluator', () => { fetchMock.mock({ ...requestMatcher, delay: 20, - response: { - result: 'Carol', - }, + response: JSON.stringify('Carol'), }); const promise = evaluator.evaluate(query); @@ -333,6 +329,31 @@ describe('An evaluator', () => { }); }); + it('should not log a timeout error message when request completes before the timeout', async () => { + jest.useFakeTimers(); + + const logger: Logger = { + debug: jest.fn(), + info: jest.fn(), + warn: jest.fn(), + error: jest.fn(), + }; + + const evaluator = new Evaluator({ + appId: appId, + logger: logger, + }); + + fetchMock.mock({ + ...requestMatcher, + response: JSON.stringify('Carol'), + }); + + await expect(evaluator.evaluate(query, {timeout: 10})).resolves.toBe('Carol'); + + expect(logger.error).not.toHaveBeenCalled(); + }); + it('should evaluate queries using the provided context', async () => { const evaluator = new Evaluator({ appId: appId, From ec435be204eea50d43475e7fd6a197aa1c284c3c Mon Sep 17 00:00:00 2001 From: Marcos Passos Date: Thu, 12 Sep 2024 08:33:35 -0600 Subject: [PATCH 2/3] Update src/contentFetcher.ts --- src/contentFetcher.ts | 1 + 1 file changed, 1 insertion(+) diff --git a/src/contentFetcher.ts b/src/contentFetcher.ts index 0f21008..61defbf 100644 --- a/src/contentFetcher.ts +++ b/src/contentFetcher.ts @@ -133,6 +133,7 @@ export class ContentFetcher { return new Promise>((resolve, reject) => { const abortController = new AbortController(); const timeout = options.timeout ?? this.configuration.defaultTimeout; + let timer: number | NodeJS.Timeout | undefined; if (timeout !== undefined) { From d2e93c79d90d80f296c3d92d7e477d0e386f5713 Mon Sep 17 00:00:00 2001 From: Marcos Passos Date: Thu, 12 Sep 2024 08:51:26 -0600 Subject: [PATCH 3/3] Update src/contentFetcher.ts --- src/contentFetcher.ts | 1 - 1 file changed, 1 deletion(-) diff --git a/src/contentFetcher.ts b/src/contentFetcher.ts index 61defbf..cbba7fb 100644 --- a/src/contentFetcher.ts +++ b/src/contentFetcher.ts @@ -128,7 +128,6 @@ export class ContentFetcher { throw new Error('The API key must be provided to fetch static content.'); } - // Types for Browser and Node environment return new Promise>((resolve, reject) => { const abortController = new AbortController();