From 2141b0879cbccb1354f9821edcc917b6aa4ff0ab Mon Sep 17 00:00:00 2001 From: Sebastian Schmidt Date: Thu, 27 May 2021 08:59:41 -0600 Subject: [PATCH] fix: do not load google-gax at client startup (#1517) --- dev/src/bulk-writer.ts | 19 +++++++------ dev/src/bundle.ts | 6 +++-- dev/src/index.ts | 53 ++++++++++++++++--------------------- dev/src/recursive-delete.ts | 9 ++++--- dev/src/reference.ts | 8 +++--- dev/src/status-code.ts | 39 +++++++++++++++++++++++++++ dev/src/transaction.ts | 26 +++++++++--------- dev/src/util.ts | 37 +++++++++++++------------- dev/src/write-batch.ts | 5 ++-- dev/test/lazy-load.ts | 40 ++++++++++++++++++++++++++++ 10 files changed, 162 insertions(+), 80 deletions(-) create mode 100644 dev/src/status-code.ts create mode 100644 dev/test/lazy-load.ts diff --git a/dev/src/bulk-writer.ts b/dev/src/bulk-writer.ts index 787095107..9c7cd1579 100644 --- a/dev/src/bulk-writer.ts +++ b/dev/src/bulk-writer.ts @@ -16,6 +16,7 @@ import * as firestore from '@google-cloud/firestore'; import * as assert from 'assert'; +import {GoogleError} from 'google-gax'; import {google} from '../protos/firestore_v1_proto_api'; import {FieldPath, Firestore} from '.'; @@ -44,7 +45,7 @@ import { validateOptional, } from './validate'; import {logger} from './logger'; -import {GoogleError, Status} from 'google-gax'; +import {StatusCode} from './status-code'; // eslint-disable-next-line no-undef import GrpcStatus = FirebaseFirestore.GrpcStatus; @@ -104,7 +105,7 @@ const DEFAULT_MAXIMUM_PENDING_OPERATIONS_COUNT = 500; class BulkWriterOperation { private deferred = new Deferred(); private failedAttempts = 0; - private lastStatus?: Status; + private lastStatus?: StatusCode; private _backoffDuration = 0; /** @@ -157,7 +158,7 @@ class BulkWriterOperation { ); if (shouldRetry) { - this.lastStatus = error.code; + this.lastStatus = error.code as number; this.updateBackoffDuration(); this.sendFn(this); } else { @@ -169,7 +170,7 @@ class BulkWriterOperation { } private updateBackoffDuration(): void { - if (this.lastStatus === Status.RESOURCE_EXHAUSTED) { + if (this.lastStatus === StatusCode.RESOURCE_EXHAUSTED) { this._backoffDuration = DEFAULT_BACKOFF_MAX_DELAY_MS; } else if (this._backoffDuration === 0) { this._backoffDuration = DEFAULT_BACKOFF_INITIAL_DELAY_MS; @@ -241,14 +242,16 @@ class BulkCommitBatch extends WriteBatch { const DELETE_TIMESTAMP_SENTINEL = Timestamp.fromMillis(0); const status = (response.status || [])[i]; - if (status.code === Status.OK) { + if (status.code === StatusCode.OK) { const updateTime = Timestamp.fromProto( response.writeResults![i].updateTime || DELETE_TIMESTAMP_SENTINEL ); this.pendingOps[i].onSuccess(new WriteResult(updateTime)); } else { - const error = new GoogleError(status.message || undefined); - error.code = status.code as Status; + const error = new (require('google-gax').GoogleError)( + status.message || undefined + ); + error.code = status.code as number; this.pendingOps[i].onError(wrapError(error, stack)); } } @@ -389,7 +392,7 @@ export class BulkWriter { private _errorFn: (error: BulkWriterError) => boolean = error => { const isRetryableDeleteError = error.operationType === 'delete' && - (error.code as number) === Status.INTERNAL; + (error.code as number) === StatusCode.INTERNAL; const retryCodes = getRetryCodes('batchWrite'); return ( (retryCodes.includes(error.code) || isRetryableDeleteError) && diff --git a/dev/src/bundle.ts b/dev/src/bundle.ts index 4f134404f..374b453bd 100644 --- a/dev/src/bundle.ts +++ b/dev/src/bundle.ts @@ -25,7 +25,6 @@ import { } from './validate'; import api = google.firestore.v1; -import BundleElement = firestore.BundleElement; const BUNDLE_VERSION = 1; @@ -146,7 +145,10 @@ export class BundleBuilder { // Convert to a valid proto message object then take its JSON representation. // This take cares of stuff like converting internal byte array fields // to Base64 encodings. - const message = BundleElement.fromObject(bundleElement).toJSON(); + // We lazy-load the Proto file to reduce cold-start times. + const message = require('../protos/firestore_v1_proto_api') + .firestore.BundleElement.fromObject(bundleElement) + .toJSON(); const buffer = Buffer.from(JSON.stringify(message), 'utf-8'); const lengthBuffer = Buffer.from(buffer.length.toString()); return Buffer.concat([lengthBuffer, buffer]); diff --git a/dev/src/index.ts b/dev/src/index.ts index b97969d5e..60cd32358 100644 --- a/dev/src/index.ts +++ b/dev/src/index.ts @@ -16,7 +16,7 @@ import * as firestore from '@google-cloud/firestore'; -import {CallOptions, grpc, RetryOptions} from 'google-gax'; +import {CallOptions} from 'google-gax'; import {Duplex, PassThrough, Transform} from 'stream'; import {URL} from 'url'; @@ -100,7 +100,6 @@ export {GeoPoint} from './geo-point'; export {CollectionGroup}; export {QueryPartition} from './query-partition'; export {setLogFunction} from './logger'; -export {Status as GrpcStatus} from 'google-gax'; const libVersion = require('../../package.json').version; setLibVersion(libVersion); @@ -127,16 +126,6 @@ setLibVersion(libVersion); * @namespace google.firestore.admin.v1 */ -/*! - * @see v1 - */ -let v1: unknown; // Lazy-loaded in `_runRequest()` - -/*! - * @see v1beta1 - */ -let v1beta1: unknown; // Lazy-loaded upon access. - /*! * HTTP header for the resource prefix to improve routing and project isolation * by the backend. @@ -504,7 +493,7 @@ export class Firestore implements firestore.Firestore { let client: GapicClient; if (this._settings.ssl === false) { - const grpcModule = this._settings.grpc ?? grpc; + const grpcModule = this._settings.grpc ?? require('google-gax').grpc; const sslCreds = grpcModule.credentials.createInsecure(); client = new module.exports.v1({ @@ -1392,7 +1381,10 @@ export class Firestore implements firestore.Firestore { if (retryCodes) { const retryParams = getRetryParams(methodName); - callOptions.retry = new RetryOptions(retryCodes, retryParams); + callOptions.retry = new (require('google-gax').RetryOptions)( + retryCodes, + retryParams + ); } return callOptions; @@ -1740,18 +1732,12 @@ module.exports = Object.assign(module.exports, existingExports); * * @private * @name Firestore.v1beta1 - * @see v1beta1 * @type {function} */ Object.defineProperty(module.exports, 'v1beta1', { // The v1beta1 module is very large. To avoid pulling it in from static - // scope, we lazy-load and cache the module. - get: () => { - if (!v1beta1) { - v1beta1 = require('./v1beta1'); - } - return v1beta1; - }, + // scope, we lazy-load the module. + get: () => require('./v1beta1'), }); /** @@ -1759,16 +1745,23 @@ Object.defineProperty(module.exports, 'v1beta1', { * * @private * @name Firestore.v1 - * @see v1 * @type {function} */ Object.defineProperty(module.exports, 'v1', { // The v1 module is very large. To avoid pulling it in from static - // scope, we lazy-load and cache the module. - get: () => { - if (!v1) { - v1 = require('./v1'); - } - return v1; - }, + // scope, we lazy-load the module. + get: () => require('./v1'), +}); + +/** + * {@link Status} factory function. + * + * @private + * @name Firestore.GrpcStatus + * @type {function} + */ +Object.defineProperty(module.exports, 'GrpcStatus', { + // The gax module is very large. To avoid pulling it in from static + // scope, we lazy-load the module. + get: () => require('google-gax').Status, }); diff --git a/dev/src/recursive-delete.ts b/dev/src/recursive-delete.ts index 97071b7ac..3c66f49f5 100644 --- a/dev/src/recursive-delete.ts +++ b/dev/src/recursive-delete.ts @@ -26,9 +26,10 @@ import Firestore, { QueryDocumentSnapshot, } from '.'; import {Deferred, wrapError} from './util'; -import {GoogleError, Status} from 'google-gax'; +import {GoogleError} from 'google-gax'; import {BulkWriterError} from './bulk-writer'; import {QueryOptions} from './reference'; +import {StatusCode} from './status-code'; /** * Datastore allowed numeric IDs where Firestore only allows strings. Numeric @@ -185,7 +186,7 @@ export class RecursiveDelete { let streamedDocsCount = 0; stream .on('error', err => { - err.code = Status.UNAVAILABLE; + err.code = StatusCode.UNAVAILABLE; err.stack = 'Failed to fetch children documents: ' + err.stack; this.lastError = err; this.onQueryEnd(); @@ -278,13 +279,13 @@ export class RecursiveDelete { if (this.lastError === undefined) { this.completionDeferred.resolve(); } else { - let error = new GoogleError( + let error = new (require('google-gax').GoogleError)( `${this.errorCount} ` + `${this.errorCount !== 1 ? 'deletes' : 'delete'} ` + 'failed. The last delete failed with: ' ); if (this.lastError.code !== undefined) { - error.code = this.lastError.code as number as Status; + error.code = this.lastError.code as number; } error = wrapError(error, this.errorStack); diff --git a/dev/src/reference.ts b/dev/src/reference.ts index 2c62dde71..a33208fb2 100644 --- a/dev/src/reference.ts +++ b/dev/src/reference.ts @@ -505,8 +505,10 @@ export class DocumentReference validateFunction('onNext', onNext); validateFunction('onError', onError, {optional: true}); - const watch = new DocumentWatch(this.firestore, this); - + const watch: DocumentWatch = new (require('./watch').DocumentWatch)( + this.firestore, + this + ); return watch.onSnapshot((readTime, size, docs) => { for (const document of docs()) { if (document.ref.path === this.path) { @@ -2250,7 +2252,7 @@ export class Query implements firestore.Query { validateFunction('onNext', onNext); validateFunction('onError', onError, {optional: true}); - const watch = new QueryWatch( + const watch: QueryWatch = new (require('./watch').QueryWatch)( this.firestore, this, this._queryOptions.converter diff --git a/dev/src/status-code.ts b/dev/src/status-code.ts new file mode 100644 index 000000000..a93837d2d --- /dev/null +++ b/dev/src/status-code.ts @@ -0,0 +1,39 @@ +/** + * Copyright 2021 Google LLC + * + * Licensed under the Apache License, Version 2.0 (the "License"); + * you may not use this file except in compliance with the License. + * You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ + +/** + * Internal copy of GRPC status code. Copied to prevent loading of google-gax + * at SDK startup. + */ +export const enum StatusCode { + OK = 0, + CANCELLED = 1, + UNKNOWN = 2, + INVALID_ARGUMENT = 3, + DEADLINE_EXCEEDED = 4, + NOT_FOUND = 5, + ALREADY_EXISTS = 6, + PERMISSION_DENIED = 7, + RESOURCE_EXHAUSTED = 8, + FAILED_PRECONDITION = 9, + ABORTED = 10, + OUT_OF_RANGE = 11, + UNIMPLEMENTED = 12, + INTERNAL = 13, + UNAVAILABLE = 14, + DATA_LOSS = 15, + UNAUTHENTICATED = 16, +} diff --git a/dev/src/transaction.ts b/dev/src/transaction.ts index ccd50588f..e7c5766c5 100644 --- a/dev/src/transaction.ts +++ b/dev/src/transaction.ts @@ -16,8 +16,7 @@ import * as firestore from '@google-cloud/firestore'; -import {GoogleError, Status} from 'google-gax'; - +import {GoogleError} from 'google-gax'; import * as proto from '../protos/firestore_v1_proto_api'; import {ExponentialBackoff} from './backoff'; @@ -25,6 +24,7 @@ import {DocumentSnapshot} from './document'; import {Firestore, WriteBatch} from './index'; import {logger} from './logger'; import {FieldPath, validateFieldPath} from './path'; +import {StatusCode} from './status-code'; import { DocumentReference, Query, @@ -484,7 +484,7 @@ export class Transaction implements firestore.Transaction { * @return A Promise that resolves after the delay expired. */ private async maybeBackoff(error?: GoogleError): Promise { - if (error && error.code === Status.RESOURCE_EXHAUSTED) { + if ((error?.code as number | undefined) === StatusCode.RESOURCE_EXHAUSTED) { this._backoff.resetToMax(); } await this._backoff.backoffAndWait(); @@ -592,17 +592,17 @@ function validateReadOptions( function isRetryableTransactionError(error: GoogleError): boolean { if (error.code !== undefined) { // This list is based on https://github.com/firebase/firebase-js-sdk/blob/master/packages/firestore/src/core/transaction_runner.ts#L112 - switch (error.code) { - case Status.ABORTED: - case Status.CANCELLED: - case Status.UNKNOWN: - case Status.DEADLINE_EXCEEDED: - case Status.INTERNAL: - case Status.UNAVAILABLE: - case Status.UNAUTHENTICATED: - case Status.RESOURCE_EXHAUSTED: + switch (error.code as number) { + case StatusCode.ABORTED: + case StatusCode.CANCELLED: + case StatusCode.UNKNOWN: + case StatusCode.DEADLINE_EXCEEDED: + case StatusCode.INTERNAL: + case StatusCode.UNAVAILABLE: + case StatusCode.UNAUTHENTICATED: + case StatusCode.RESOURCE_EXHAUSTED: return true; - case Status.INVALID_ARGUMENT: + case StatusCode.INVALID_ARGUMENT: // The Firestore backend uses "INVALID_ARGUMENT" for transactions // IDs that have expired. While INVALID_ARGUMENT is generally not // retryable, we retry this specific case. diff --git a/dev/src/util.ts b/dev/src/util.ts index d61306558..920f7d502 100644 --- a/dev/src/util.ts +++ b/dev/src/util.ts @@ -17,24 +17,10 @@ import {DocumentData} from '@google-cloud/firestore'; import {randomBytes} from 'crypto'; -import { - CallSettings, - ClientConfig, - constructSettings, - createDefaultBackoffSettings, - GoogleError, - Status, -} from 'google-gax'; +import {CallSettings, ClientConfig, GoogleError} from 'google-gax'; import {BackoffSettings} from 'google-gax/build/src/gax'; import * as gapicConfig from './v1/firestore_client_config.json'; -const serviceConfig = constructSettings( - 'google.firestore.v1.Firestore', - gapicConfig as ClientConfig, - {}, - Status -) as {[k: string]: CallSettings}; - /** * A Promise implementation that supports deferred resolution. * @private @@ -158,13 +144,28 @@ export function isPermanentRpcError( } } +let serviceConfig: Record | undefined; + +/** Lazy-loads the service config when first accessed. */ +function getServiceConfig(methodName: string): CallSettings | undefined { + if (!serviceConfig) { + serviceConfig = require('google-gax').constructSettings( + 'google.firestore.v1.Firestore', + gapicConfig as ClientConfig, + {}, + require('google-gax').Status + ) as {[k: string]: CallSettings}; + } + return serviceConfig[methodName]; +} + /** * Returns the list of retryable error codes specified in the service * configuration. * @private */ export function getRetryCodes(methodName: string): number[] { - return serviceConfig[methodName]?.retry?.retryCodes ?? []; + return getServiceConfig(methodName)?.retry?.retryCodes ?? []; } /** @@ -173,8 +174,8 @@ export function getRetryCodes(methodName: string): number[] { */ export function getRetryParams(methodName: string): BackoffSettings { return ( - serviceConfig[methodName]?.retry?.backoffSettings ?? - createDefaultBackoffSettings() + getServiceConfig(methodName)?.retry?.backoffSettings ?? + require('google-gax').createDefaultBackoffSettings() ); } diff --git a/dev/src/write-batch.ts b/dev/src/write-batch.ts index d4adf0e51..b6e684b1e 100644 --- a/dev/src/write-batch.ts +++ b/dev/src/write-batch.ts @@ -45,7 +45,8 @@ import { validateMinNumberOfArguments, validateOptional, } from './validate'; -import {Status} from 'google-gax'; +import {StatusCode} from './status-code'; + import api = google.firestore.v1; /** @@ -544,7 +545,7 @@ export class WriteBatch implements firestore.WriteBatch { const stack = Error().stack!; // Commits should also be retried when they fail with status code ABORTED. - const retryCodes = [Status.ABORTED, ...getRetryCodes('commit')]; + const retryCodes = [StatusCode.ABORTED, ...getRetryCodes('commit')]; return this._commit({retryCodes}) .then(response => { diff --git a/dev/test/lazy-load.ts b/dev/test/lazy-load.ts new file mode 100644 index 000000000..7d4f1bf2a --- /dev/null +++ b/dev/test/lazy-load.ts @@ -0,0 +1,40 @@ +// Copyright 2021 Google LLC +// +// Licensed under the Apache License, Version 2.0 (the "License"); +// you may not use this file except in compliance with the License. +// You may obtain a copy of the License at +// +// http://www.apache.org/licenses/LICENSE-2.0 +// +// Unless required by applicable law or agreed to in writing, software +// distributed under the License is distributed on an "AS IS" BASIS, +// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +// See the License for the specific language governing permissions and +// limitations under the License. + +import {describe, it} from 'mocha'; +import {expect} from 'chai'; + +function isModuleLoaded(moduleName: string) { + return !!Object.keys(require.cache).find( + path => path.indexOf(`node_modules/${moduleName}`) !== -1 + ); +} + +describe('Index.js', () => { + (isModuleLoaded('google-gax') ? it.skip : it)( + 'does not load google-gax', + () => { + require('../src/index'); + expect(isModuleLoaded('google-gax')).to.be.false; + } + ); + + (isModuleLoaded('protobufjs') ? it.skip : it)( + 'does not load protobufjs', + () => { + require('../src/index'); + expect(isModuleLoaded('protobufjs')).to.be.false; + } + ); +});