From 8c3584ba4df8cec1d0721fedc41ecfbef2850d2e Mon Sep 17 00:00:00 2001 From: Thiago Morales Date: Sat, 4 Jun 2022 01:25:31 -0300 Subject: [PATCH] feat: add retry config customization in app init --- etc/firebase-admin.api.md | 11 +++++++ etc/firebase-admin.app.api.md | 11 +++++++ src/app/core.ts | 11 +++++++ src/app/index.ts | 1 + src/auth/auth-api-request.ts | 2 +- src/firebase-namespace-api.ts | 1 + src/messaging/batch-request-internal.ts | 4 +-- .../messaging-api-request-internal.ts | 2 +- ...project-management-api-request-internal.ts | 4 +-- src/utils/api-request.ts | 13 +++++--- test/resources/mocks.ts | 12 ++++++++ .../machine-learning/machine-learning.spec.ts | 8 ++--- test/unit/utils/api-request.spec.ts | 30 +++++++++++++++++-- 13 files changed, 94 insertions(+), 16 deletions(-) diff --git a/etc/firebase-admin.api.md b/etc/firebase-admin.api.md index 33c8664d29..c18106518f 100644 --- a/etc/firebase-admin.api.md +++ b/etc/firebase-admin.api.md @@ -72,8 +72,10 @@ export interface AppOptions { credential?: Credential; databaseAuthVariableOverride?: object | null; databaseURL?: string; + disableRetry?: boolean; httpAgent?: Agent; projectId?: string; + retryConfig?: RetryConfig; serviceAccountId?: string; storageBucket?: string; } @@ -462,6 +464,15 @@ export namespace remoteConfig { export type Version = Version; } +// @public +export interface RetryConfig { + backOffFactor?: number; + ioErrorCodes?: string[]; + maxDelayInMillis: number; + maxRetries: number; + statusCodes?: number[]; +} + // @public (undocumented) export const SDK_VERSION: string; diff --git a/etc/firebase-admin.app.api.md b/etc/firebase-admin.app.api.md index a6ad65cd57..97eb5095a9 100644 --- a/etc/firebase-admin.app.api.md +++ b/etc/firebase-admin.app.api.md @@ -22,8 +22,10 @@ export interface AppOptions { credential?: Credential; databaseAuthVariableOverride?: object | null; databaseURL?: string; + disableRetry?: boolean; httpAgent?: Agent; projectId?: string; + retryConfig?: RetryConfig; serviceAccountId?: string; storageBucket?: string; } @@ -73,6 +75,15 @@ export function initializeApp(options?: AppOptions, appName?: string): App; // @public export function refreshToken(refreshTokenPathOrObject: string | object, httpAgent?: Agent): Credential; +// @public +export interface RetryConfig { + backOffFactor?: number; + ioErrorCodes?: string[]; + maxDelayInMillis: number; + maxRetries: number; + statusCodes?: number[]; +} + // @public (undocumented) export const SDK_VERSION: string; diff --git a/src/app/core.ts b/src/app/core.ts index f37e3c112c..1056e2b289 100644 --- a/src/app/core.ts +++ b/src/app/core.ts @@ -18,6 +18,7 @@ import { Agent } from 'http'; import { Credential } from './credential'; +import { RetryConfig } from '.' /** * Available options to pass to {@link firebase-admin.app#initializeApp}. @@ -83,6 +84,16 @@ export interface AppOptions { * specifying an HTTP Agent in the corresponding factory methods. */ httpAgent?: Agent; + + /** + * The retry configuration that will be used in HttpClient for API Requests. + */ + retryConfig?: RetryConfig; + + /** + * Completely disable the retry strategy in HttpClient. + */ + disableRetry?: boolean; } /** diff --git a/src/app/index.ts b/src/app/index.ts index 02a8509653..e5d3b27c7d 100644 --- a/src/app/index.ts +++ b/src/app/index.ts @@ -28,5 +28,6 @@ export { initializeApp, getApp, getApps, deleteApp } from './lifecycle'; export { Credential, ServiceAccount, GoogleOAuthAccessToken } from './credential'; export { applicationDefault, cert, refreshToken } from './credential-factory'; +export { RetryConfig } from '../utils/api-request' export const SDK_VERSION = getSdkVersion(); diff --git a/src/auth/auth-api-request.ts b/src/auth/auth-api-request.ts index a962a4f719..bd4900d75d 100644 --- a/src/auth/auth-api-request.ts +++ b/src/auth/auth-api-request.ts @@ -48,7 +48,7 @@ const FIREBASE_AUTH_HEADER = { 'X-Client-Version': `Node/Admin/${utils.getSdkVersion()}`, }; /** Firebase Auth request timeout duration in milliseconds. */ -const FIREBASE_AUTH_TIMEOUT = 25000; +const FIREBASE_AUTH_TIMEOUT = parseInt(process.env.FIREBASE_AUTH_TIMEOUT || '25000', 10); /** List of reserved claims which cannot be provided when creating a custom token. */ diff --git a/src/firebase-namespace-api.ts b/src/firebase-namespace-api.ts index 5358e5bd2c..6a4a3f9385 100644 --- a/src/firebase-namespace-api.ts +++ b/src/firebase-namespace-api.ts @@ -91,6 +91,7 @@ export { projectManagement } from './project-management/project-management-names export { remoteConfig } from './remote-config/remote-config-namespace'; export { securityRules } from './security-rules/security-rules-namespace'; export { storage } from './storage/storage-namespace'; +export { RetryConfig } from './utils/api-request' // Declare other top-level members of the admin namespace below. Unfortunately, there's no // compile-time mechanism to ensure that the FirebaseNamespace class actually provides these diff --git a/src/messaging/batch-request-internal.ts b/src/messaging/batch-request-internal.ts index 8244edf215..5ba80e0581 100644 --- a/src/messaging/batch-request-internal.ts +++ b/src/messaging/batch-request-internal.ts @@ -20,7 +20,7 @@ import { import { FirebaseAppError, AppErrorCodes } from '../utils/error'; const PART_BOUNDARY = '__END_OF_PART__'; -const TEN_SECONDS_IN_MILLIS = 10000; +const FIREBASE_MESSAGING_BATCH_TIMEOUT = parseInt(process.env.FIREBASE_MESSAGING_BATCH_TIMEOUT || '10000', 10); /** * Represents a request that can be sent as part of an HTTP batch request. @@ -72,7 +72,7 @@ export class BatchRequestClient { url: this.batchUrl, data: this.getMultipartPayload(requests), headers: Object.assign({}, this.commonHeaders, requestHeaders), - timeout: TEN_SECONDS_IN_MILLIS, + timeout: FIREBASE_MESSAGING_BATCH_TIMEOUT, }; return this.httpClient.send(request).then((response) => { if (!response.multipart) { diff --git a/src/messaging/messaging-api-request-internal.ts b/src/messaging/messaging-api-request-internal.ts index b24154ba0c..0c685c9548 100644 --- a/src/messaging/messaging-api-request-internal.ts +++ b/src/messaging/messaging-api-request-internal.ts @@ -27,7 +27,7 @@ import { SendResponse, BatchResponse } from './messaging-api'; // FCM backend constants -const FIREBASE_MESSAGING_TIMEOUT = 10000; +const FIREBASE_MESSAGING_TIMEOUT = parseInt(process.env.FIREBASE_MESSAGING_TIMEOUT || '10000', 10); const FIREBASE_MESSAGING_BATCH_URL = 'https://fcm.googleapis.com/batch'; const FIREBASE_MESSAGING_HTTP_METHOD: HttpMethod = 'POST'; const FIREBASE_MESSAGING_HEADERS = { diff --git a/src/project-management/project-management-api-request-internal.ts b/src/project-management/project-management-api-request-internal.ts index d7e597d50d..4f019a5421 100644 --- a/src/project-management/project-management-api-request-internal.ts +++ b/src/project-management/project-management-api-request-internal.ts @@ -36,7 +36,7 @@ const PROJECT_MANAGEMENT_HEADERS = { 'X-Client-Version': `Node/Admin/${getSdkVersion()}`, }; /** Project management request timeout duration in milliseconds. */ -const PROJECT_MANAGEMENT_TIMEOUT_MILLIS = 10000; +const FIREBASE_PROJECT_MANAGEMENT_TIMEOUT = parseInt(process.env.FIREBASE_PROJECT_MANAGEMENT_TIMEOUT || '10000', 10); const LIST_APPS_MAX_PAGE_SIZE = 100; @@ -315,7 +315,7 @@ export class ProjectManagementRequestHandler { url: `${baseUrlToUse}${path}`, headers: PROJECT_MANAGEMENT_HEADERS, data: requestData, - timeout: PROJECT_MANAGEMENT_TIMEOUT_MILLIS, + timeout: FIREBASE_PROJECT_MANAGEMENT_TIMEOUT, }; return this.httpClient.send(request) diff --git a/src/utils/api-request.ts b/src/utils/api-request.ts index 9132fcaeb8..b26e294dfc 100644 --- a/src/utils/api-request.ts +++ b/src/utils/api-request.ts @@ -243,8 +243,13 @@ function validateRetryConfig(retry: RetryConfig): void { export class HttpClient { - constructor(private readonly retry: RetryConfig | null = defaultRetryConfig()) { - if (this.retry) { + constructor( + private readonly retry: RetryConfig | null = null, + private readonly disableRetry: boolean = false + ) { + if (!this.disableRetry && !this.retry) { + this.retry = defaultRetryConfig(); + } else if (!this.disableRetry && this.retry) { validateRetryConfig(this.retry); } } @@ -345,7 +350,7 @@ export class HttpClient { } private isRetryEligible(retryAttempts: number, err: LowLevelError): boolean { - if (!this.retry) { + if (this.disableRetry || !this.retry) { return false; } @@ -811,7 +816,7 @@ class HttpRequestConfigImpl implements HttpRequestConfig { export class AuthorizedHttpClient extends HttpClient { constructor(private readonly app: FirebaseApp) { - super(); + super(app.options.retryConfig, app.options.disableRetry); } public send(request: HttpRequestConfig): Promise { diff --git a/test/resources/mocks.ts b/test/resources/mocks.ts index f55fd9052b..74ab9e5e70 100644 --- a/test/resources/mocks.ts +++ b/test/resources/mocks.ts @@ -53,11 +53,19 @@ export const databaseAuthVariableOverride = { 'some#string': 'some#val' }; export const storageBucket = 'bucketName.appspot.com'; export const credential = cert(path.resolve(__dirname, './mock.key.json')); +export const retryConfig = { + maxRetries: 15, + statusCodes: [507], + ioErrorCodes: ['ECONNRESET', 'ETIMEDOUT'], + backOffFactor: 0.8, + maxDelayInMillis: 190000, +} export const appOptions: AppOptions = { credential, databaseURL, storageBucket, + retryConfig, }; export const appOptionsWithOverride: AppOptions = { @@ -66,19 +74,23 @@ export const appOptionsWithOverride: AppOptions = { databaseURL, storageBucket, projectId, + retryConfig, }; export const appOptionsNoAuth: AppOptions = { databaseURL, + retryConfig, }; export const appOptionsNoDatabaseUrl: AppOptions = { credential, + retryConfig, }; export const appOptionsAuthDB: AppOptions = { credential, databaseURL, + retryConfig, }; export class MockCredential implements Credential { diff --git a/test/unit/machine-learning/machine-learning.spec.ts b/test/unit/machine-learning/machine-learning.spec.ts index 8791ca9c87..e51264fc41 100644 --- a/test/unit/machine-learning/machine-learning.spec.ts +++ b/test/unit/machine-learning/machine-learning.spec.ts @@ -582,7 +582,7 @@ describe('MachineLearning', () => { stubs.push(stub); return machineLearning.createModel(MODEL_OPTIONS_WITH_GCS) .should.eventually.be.rejected.and.have.property( - 'message', 'Cannot read property \'done\' of null'); + 'message', 'Cannot read properties of null (reading \'done\')'); }); it('should reject when API response does not contain a name', () => { @@ -699,7 +699,7 @@ describe('MachineLearning', () => { stubs.push(stub); return machineLearning.updateModel(MODEL_ID, MODEL_OPTIONS_WITH_GCS) .should.eventually.be.rejected.and.have.property( - 'message', 'Cannot read property \'done\' of null'); + 'message', 'Cannot read properties of null (reading \'done\')'); }); it('should reject when API response does not contain a name', () => { @@ -803,7 +803,7 @@ describe('MachineLearning', () => { stubs.push(stub); return machineLearning.publishModel(MODEL_ID) .should.eventually.be.rejected.and.have.property( - 'message', 'Cannot read property \'done\' of null'); + 'message', 'Cannot read properties of null (reading \'done\')'); }); it('should reject when API response does not contain a name', () => { @@ -907,7 +907,7 @@ describe('MachineLearning', () => { stubs.push(stub); return machineLearning.unpublishModel(MODEL_ID) .should.eventually.be.rejected.and.have.property( - 'message', 'Cannot read property \'done\' of null'); + 'message', 'Cannot read properties of null (reading \'done\')'); }); it('should reject when API response does not contain a name', () => { diff --git a/test/unit/utils/api-request.spec.ts b/test/unit/utils/api-request.spec.ts index fd92a9c191..f0cce3eefd 100644 --- a/test/unit/utils/api-request.spec.ts +++ b/test/unit/utils/api-request.spec.ts @@ -786,9 +786,35 @@ describe('HttpClient', () => { }); }); - it('should not retry when RetryConfig is explicitly null', () => { + it('should succeed, with custom retry config after 1 retry, on a single internal server error response', () => { + mockedRequests.push(mockRequestWithHttpError(500)); + const customRetryConfig = { + maxRetries: 4, + statusCodes: [500], + ioErrorCodes: [], + backOffFactor: 0.5, + maxDelayInMillis: 60 * 1000, + }; + const respData = { foo: 'bar' }; + const scope = nock('https://' + mockHost) + .get(mockPath) + .reply(200, respData, { + 'content-type': 'application/json', + }); + mockedRequests.push(scope); + const client = new HttpClient(customRetryConfig); + return client.send({ + method: 'GET', + url: mockUrl, + }).then((resp) => { + expect(resp.status).to.equal(200); + expect(resp.data).to.deep.equal(respData); + }); + }); + + it('should not retry when retry is explicitly disabled', () => { mockedRequests.push(mockRequestWithError({ message: 'connection reset 1', code: 'ECONNRESET' })); - const client = new HttpClient(null); + const client = new HttpClient(null, true); const err = 'Error while making request: connection reset 1'; return client.send({ method: 'GET',