From 156dc83be200431a993a9b600e172e06ddfc8648 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Knut=20Olav=20L=C3=B8ite?= Date: Thu, 17 Sep 2026 13:13:42 +0200 Subject: [PATCH] perf(spanner): remove rest-parameter allocations in wrappedRequestFn Every RPC passes through wrappedRequestFn in prepareGapicRequest_. The function previously used rest parameters (...args) and sliced the arguments array on each call, creating two ephemeral arrays per RPC. Because reqOpts and gaxOpts are already bound to requestFn, wrappedRequestFn only receives an optional callback. This change: - Replaces ...args and args.slice with a single optional callback parameter. - Reuses the initial promise in non-callback mode instead of invoking requestFn a second time. - Preserves .cancel() when requestFn returns a CancellablePromise. - Adds try/catch in callback mode to inject the request ID if requestFn throws synchronously. --- handwritten/spanner/src/index.ts | 118 +++++++------ handwritten/spanner/test/index.ts | 258 +++++++++++++++++++++++++++- handwritten/spanner/test/spanner.ts | 39 +++++ 3 files changed, 363 insertions(+), 52 deletions(-) diff --git a/handwritten/spanner/src/index.ts b/handwritten/spanner/src/index.ts index 17287fba98bf..a0ca5999bd4f 100644 --- a/handwritten/spanner/src/index.ts +++ b/handwritten/spanner/src/index.ts @@ -534,7 +534,7 @@ class Spanner extends GrpcService { if (!this.clients_.has(clientName)) { this.clients_.set( clientName, - new v1[clientName](this.options as ClientOptions), + new v1.InstanceAdminClient(this.options as ClientOptions), ); } return this.clients_.get(clientName)! as v1.InstanceAdminClient; @@ -558,7 +558,7 @@ class Spanner extends GrpcService { if (!this.clients_.has(clientName)) { this.clients_.set( clientName, - new v1[clientName](this.options as ClientOptions), + new v1.DatabaseAdminClient(this.options as ClientOptions), ); } return this.clients_.get(clientName)! as v1.DatabaseAdminClient; @@ -615,13 +615,12 @@ class Spanner extends GrpcService { if (callback) { // process.nextTick prevents Unhandled Promise Rejections if callback throws - res.then( - () => process.nextTick(() => callback(null)), - err => process.nextTick(() => callback(err)), - ); - } else { - return res; + res + .then(() => process.nextTick(() => callback(null))) + .catch(err => process.nextTick(() => callback(err))); + return; } + return res; } /** @@ -1727,7 +1726,11 @@ class Spanner extends GrpcService { const clientName = config.client; try { if (!this.clients_.has(clientName)) { - this.clients_.set(clientName, new v1[clientName](this.options)); + const v1Clients: {[key: string]: any} = v1; + this.clients_.set( + clientName, + new v1Clients[clientName](this.options), + ); } } catch (err) { callback(err, null); @@ -1791,52 +1794,70 @@ class Spanner extends GrpcService { }), ); - // Wrap requestFn to inject the spanner request id into every returned error. - const wrappedRequestFn = (...args) => { - const hasCallback = - args && - args.length > 0 && - typeof args[args.length - 1] === 'function'; - - switch (hasCallback) { - case true: { - const cb = args[args.length - 1]; - const priorArgs = args.slice(0, args.length - 1); - requestFn(...priorArgs, (...results) => { - if (results && results.length > 0) { - const err = results[0] as Error; - injectRequestIDIntoError(config, err); + // Extract a lightweight config reference containing only headers so error + // enrichment is decoupled from the caller's mutable config object. + const errorConfig = {headers: config?.headers}; + + // Wrap requestFn to inject the Spanner request ID (x-goog-spanner-request-id) + // into any error returned via callback, rejected promise, stream event, or + // synchronous exception. + // + // Because reqOpts and gaxOpts are already pre-bound to requestFn, wrappedRequestFn + // receives at most one argument: an optional callback. + const wrappedRequestFn = (callback?: Function) => { + // Callback mode: invoke requestFn with an intercepted callback to enrich + // the error parameter before delegating to the caller's callback. + if (typeof callback === 'function') { + try { + requestFn((...results: unknown[]) => { + if (results[0]) { + injectRequestIDIntoError(errorConfig, results[0] as Error); } - - cb(...results); + callback(...results); }); - return; + } catch (err) { + injectRequestIDIntoError(errorConfig, err as Error); + throw err; } + return; + } - case false: { - const res = requestFn(...args); - const stream = res as EventEmitter; - if (stream) { - stream.on('error', err => { - injectRequestIDIntoError(config, err as Error); - }); - } - - const originallyPromise = res instanceof Promise; - if (!originallyPromise) { - return res; - } + // Non-callback mode: invoke requestFn() for Promise or Stream callers. + let res; + try { + res = requestFn(); + } catch (err) { + injectRequestIDIntoError(errorConfig, err as Error); + throw err; + } - return new Promise((resolve, reject) => { - requestFn(...args) - .then(resolve) - .catch(err => { - injectRequestIDIntoError(config, err as Error); - reject(err); - }); - }); + // Handle Promise / Thenable return values (e.g. unary requests). + // Attach a rejection handler to inject the request ID into rejected errors. + // If the promise is cancellable (e.g. google-gax CancellablePromise), preserve + // its .cancel() method so callers can cancel the underlying operation. + if (res && typeof (res as PromiseLike).then === 'function') { + const chained = (res as PromiseLike).then(null, err => { + injectRequestIDIntoError(errorConfig, err as Error); + throw err; + }); + if (typeof (res as {cancel?: Function}).cancel === 'function') { + (chained as {cancel?: Function}).cancel = ( + res as {cancel: Function} + ).cancel.bind(res); } + return chained; } + + // Handle Stream return values (e.g. streaming reads or queries). + // Listen for 'error' events to enrich the emitted error with the request ID. + const stream = res as EventEmitter; + if (stream && typeof stream.on === 'function') { + stream.on('error', err => { + injectRequestIDIntoError(errorConfig, err as Error); + }); + } + + return res; }; callback(null, wrappedRequestFn); @@ -1896,6 +1917,7 @@ class Spanner extends GrpcService { .then(val => { metricsTracer?.recordOperationCompletion(); resolve(val); + return val; }) .catch(error => { metricsTracer?.recordOperationCompletion(); diff --git a/handwritten/spanner/test/index.ts b/handwritten/spanner/test/index.ts index 6652d25fec63..30ef40d2c056 100644 --- a/handwritten/spanner/test/index.ts +++ b/handwritten/spanner/test/index.ts @@ -38,6 +38,7 @@ import { GetInstancesOptions, } from '../src'; import {Duplex} from 'stream'; +import {EventEmitter} from 'events'; import {CLOUD_RESOURCE_HEADER, AFE_SERVER_TIMING_HEADER} from '../src/common'; import {MetricsTracerFactory} from '../src/metrics/metrics-tracer-factory'; import IsolationLevel = protos.google.spanner.v1.TransactionOptions.IsolationLevel; @@ -2223,6 +2224,256 @@ describe('Spanner', () => { requestFn(done); // (FAKE_GAPIC_CLIENT[CONFIG.method]) }); }); + + it('should invoke gapic method with exact arguments and attach requestID on callback error', done => { + replaceProjectIdTokenOverride = reqOpts => reqOpts; + const apiError = new Error('Callback failure') as Error & { + requestID?: string; + }; + const expectedResponse = {result: 'ok'}; + const expectedApiResponse = {metadata: 'meta'}; + + const configWithRequestId = Object.assign({}, CONFIG, { + headers: Object.assign({}, CONFIG.headers, { + 'x-goog-spanner-request-id': 'req-callback-123', + }), + }); + + FAKE_GAPIC_CLIENT[CONFIG.method] = function ( + reqOpts: unknown, + gaxOpts: unknown, + callback: Function, + ) { + assert.strictEqual(arguments.length, 3); + assert.strictEqual(typeof callback, 'function'); + callback(apiError, expectedResponse, expectedApiResponse); + }; + + spanner.prepareGapicRequest_(configWithRequestId, (err, requestFn) => { + assert.ifError(err); + requestFn( + ( + error: Error & {requestID?: string}, + response: unknown, + apiResponse: unknown, + ) => { + assert.strictEqual(error, apiError); + assert.strictEqual(error.requestID, 'req-callback-123'); + assert.strictEqual(response, expectedResponse); + assert.strictEqual(apiResponse, expectedApiResponse); + done(); + }, + ); + }); + }); + + it('should invoke gapic method once with exact arguments and attach requestID on rejected promise', async () => { + replaceProjectIdTokenOverride = reqOpts => reqOpts; + const promiseError = new Error('Promise failure') as Error & { + requestID?: string; + }; + let invocationCount = 0; + + const configWithRequestId = Object.assign({}, CONFIG, { + headers: Object.assign({}, CONFIG.headers, { + 'x-goog-spanner-request-id': 'req-promise-456', + }), + }); + + FAKE_GAPIC_CLIENT[CONFIG.method] = function () { + invocationCount++; + assert.strictEqual(arguments.length, 2); + return Promise.reject(promiseError); + }; + + const requestFn = await new Promise((resolve, reject) => { + spanner.prepareGapicRequest_(configWithRequestId, (err, fn) => { + if (err) { + reject(err); + } else { + resolve(fn); + } + }); + }); + + await assert.rejects( + async () => { + await requestFn(); + }, + (error: Error & {requestID?: string}) => { + assert.strictEqual(invocationCount, 1); + assert.strictEqual(error, promiseError); + assert.strictEqual(error.requestID, 'req-promise-456'); + return true; + }, + ); + }); + + it('should attach requestID to error when gapic method throws synchronously', done => { + replaceProjectIdTokenOverride = reqOpts => reqOpts; + const syncError = new Error('Sync failure') as Error & { + requestID?: string; + }; + + const configWithRequestId = Object.assign({}, CONFIG, { + headers: Object.assign({}, CONFIG.headers, { + 'x-goog-spanner-request-id': 'req-sync-789', + }), + }); + + FAKE_GAPIC_CLIENT[CONFIG.method] = function () { + assert.strictEqual(arguments.length, 2); + throw syncError; + }; + + spanner.prepareGapicRequest_(configWithRequestId, (err, requestFn) => { + assert.ifError(err); + assert.throws( + () => { + requestFn(); + }, + (error: Error & {requestID?: string}) => { + assert.strictEqual(error, syncError); + assert.strictEqual(error.requestID, 'req-sync-789'); + return true; + }, + ); + done(); + }); + }); + + it('should attach requestID to stream error events', done => { + replaceProjectIdTokenOverride = reqOpts => reqOpts; + const streamError = new Error('Stream failure') as Error & { + requestID?: string; + }; + const fakeStream = new EventEmitter(); + + const configWithRequestId = Object.assign({}, CONFIG, { + headers: Object.assign({}, CONFIG.headers, { + 'x-goog-spanner-request-id': 'req-stream-012', + }), + }); + + FAKE_GAPIC_CLIENT[CONFIG.method] = function () { + assert.strictEqual(arguments.length, 2); + return fakeStream; + }; + + spanner.prepareGapicRequest_(configWithRequestId, (err, requestFn) => { + assert.ifError(err); + const stream = requestFn(); + stream.on('error', (error: Error & {requestID?: string}) => { + assert.strictEqual(error, streamError); + assert.strictEqual(error.requestID, 'req-stream-012'); + done(); + }); + fakeStream.emit('error', streamError); + }); + }); + + it('should attach requestID to error when gapic method throws synchronously in callback mode', done => { + replaceProjectIdTokenOverride = reqOpts => reqOpts; + const syncError = new Error('Sync callback failure') as Error & { + requestID?: string; + }; + + const configWithRequestId = Object.assign({}, CONFIG, { + headers: Object.assign({}, CONFIG.headers, { + 'x-goog-spanner-request-id': 'req-sync-callback-123', + }), + }); + + FAKE_GAPIC_CLIENT[CONFIG.method] = function () { + assert.strictEqual(arguments.length, 3); + throw syncError; + }; + + spanner.prepareGapicRequest_(configWithRequestId, (err, requestFn) => { + assert.ifError(err); + assert.throws( + () => { + requestFn(assert.ifError); + }, + (error: Error & {requestID?: string}) => { + assert.strictEqual(error, syncError); + assert.strictEqual(error.requestID, 'req-sync-callback-123'); + return true; + }, + ); + done(); + }); + }); + + it('should preserve and forward cancel method on cancellable promises', done => { + replaceProjectIdTokenOverride = reqOpts => reqOpts; + let cancelInvoked = false; + const cancellablePromise = Promise.resolve('ok') as Promise & { + cancel: () => void; + }; + cancellablePromise.cancel = () => { + cancelInvoked = true; + }; + + FAKE_GAPIC_CLIENT[CONFIG.method] = function () { + return cancellablePromise; + }; + + spanner.prepareGapicRequest_(CONFIG, (err, requestFn) => { + assert.ifError(err); + const result = requestFn(); + assert.strictEqual(typeof result.cancel, 'function'); + result.cancel(); + assert.strictEqual(cancelInvoked, true); + done(); + }); + }); + + it('should recognize duck-typed thenable and attach requestID on rejection', async () => { + replaceProjectIdTokenOverride = reqOpts => reqOpts; + const thenableError = new Error('Thenable error') as Error & { + requestID?: string; + }; + const customThenable = { + then: ( + _onFulfilled?: Function | null, + onRejected?: (error: unknown) => unknown, + ) => { + return Promise.reject(thenableError).then(null, onRejected); + }, + }; + + const configWithRequestId = Object.assign({}, CONFIG, { + headers: Object.assign({}, CONFIG.headers, { + 'x-goog-spanner-request-id': 'req-thenable-789', + }), + }); + + FAKE_GAPIC_CLIENT[CONFIG.method] = function () { + return customThenable; + }; + + const requestFn = await new Promise((resolve, reject) => { + spanner.prepareGapicRequest_(configWithRequestId, (err, fn) => { + if (err) { + reject(err); + } else { + resolve(fn); + } + }); + }); + + await assert.rejects( + async () => { + await requestFn(); + }, + (error: Error & {requestID?: string}) => { + assert.strictEqual(error, thenableError); + assert.strictEqual(error.requestID, 'req-thenable-789'); + return true; + }, + ); + }); }); describe('request', () => { @@ -2301,7 +2552,7 @@ describe('Spanner', () => { }); }); - it('should resolve the promise with the request fn', () => { + it('should resolve the promise with the request fn', async () => { const gapicRequestFnResult = {}; function gapicRequestFn() { @@ -2312,9 +2563,8 @@ describe('Spanner', () => { callback(null, gapicRequestFn); }; - return spanner.request(CONFIG).then(result => { - assert.strictEqual(result, gapicRequestFnResult); - }); + const result = await spanner.request(CONFIG); + assert.strictEqual(result, gapicRequestFnResult); }); }); }); diff --git a/handwritten/spanner/test/spanner.ts b/handwritten/spanner/test/spanner.ts index 926aeb8ca63b..041b71188ff0 100644 --- a/handwritten/spanner/test/spanner.ts +++ b/handwritten/spanner/test/spanner.ts @@ -386,6 +386,45 @@ describe('Spanner with mock server', () => { assert.notStrictEqual(dbWithDefaultOptions, dbWithWriteSessions); }); + it('should invoke promise-based GAPIC request exactly once against mock server', async () => { + const databaseName = + 'projects/test-project/instances/instance/databases/gapic-test-db'; + await new Promise((resolve, reject) => { + // eslint-disable-next-line @typescript-eslint/no-explicit-any + (spanner as any).prepareGapicRequest_( + { + client: 'SpannerClient', + method: 'createSession', + reqOpts: { + database: databaseName, + }, + headers: { + 'x-goog-spanner-request-id': `1.${randIdForProcess}.1.1.1.1`, + }, + }, + async (err: Error | null, requestFn: Function) => { + if (err) { + reject(err); + return; + } + try { + const session = await requestFn(); + assert.ok(session); + resolve(); + } catch (e) { + reject(e); + } + }, + ); + }); + const createSessionRequests = spannerMock + .getRequests() + .filter( + req => (req as v1.CreateSessionRequest).database === databaseName, + ); + assert.strictEqual(createSessionRequests.length, 1); + }); + it('should execute query', async () => { // The query to execute const query = {