fix: Closing the client should work even if the Gapic clients have not been initialized yet

src/index.ts

@@ -379,6 +379,7 @@ export interface BigtableOptions extends gax.GoogleAuthOptions {
  * ```
  */
 export class Bigtable {
+  closed = false; // Make private
   customEndpoint?: string;
   options: BigtableOptions;
   api: {
@@ -786,7 +787,7 @@ export class Bigtable {
     const prepareGaxRequest = (
       callback: (err: Error | null, fn?: Function) => void
     ) => {
-      this.getProjectId_((err, projectId) => {
+      this.getProjectId_(async (err, projectId) => {
         if (err) {
           callback(err);
           return;
@@ -797,6 +798,19 @@ export class Bigtable {
           const clientOptions = this.options[config.client]!;
           gaxClient = new v2[config.client](clientOptions);
           this.api[config.client] = gaxClient;
+          if (this.closed) {
+            /**
+             * If close has already been called on the handwritten client then
+             * we still want to make sure the Gapic client is closed before it
+             * services the API call. In order for the close function to work,
+             * the initialize function needs to be called to initialize the
+             * stub so that it can be closed.
+             */
+            console.log('initializing and closing');
+            await gaxClient.initialize();
+            await gaxClient.close();
+            console.log('done initializing and closing');
+          }
         }
         let reqOpts = extend(true, {}, config.reqOpts);
         if (this.shouldReplaceProjectIdToken && projectId !== '{{projectId}}') {
@@ -900,6 +914,7 @@ export class Bigtable {
    * kill connections with pending requests.
    */
   close(): Promise<void[]> {
+    this.closed = true;
     const combined = Object.keys(this.api).map(clientType =>
       this.api[clientType].close()
     );

system-test/close-client-after-initialization.ts

@@ -0,0 +1,147 @@
+// Copyright 2024 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
+//
+//     https://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, before, after} from 'mocha';
+import Q from 'p-queue';
+
+import {Bigtable, Instance} from '../src';
+import {generateId, PREFIX} from './common';
+import * as assert from 'assert';
+import {ServiceError} from 'google-gax';
+
+const v2 = Symbol.for('v2');
+
+describe('CloseClientAfterInitialization', () => {
+  const instanceId = generateId('instance');
+  const bigtable = new Bigtable();
+  const INSTANCE = bigtable.instance(instanceId);
+  const TABLE = INSTANCE.table(generateId('table'));
+  const APP_PROFILE_ID = generateId('appProfile');
+  const CLUSTER_ID = generateId('cluster');
+
+  async function reapBackups(instance: Instance) {
+    const [backups] = await instance.getBackups();
+    const q = new Q({concurrency: 5});
+    return Promise.all(
+      backups.map(backup => {
+        q.add(async () => {
+          try {
+            await backup.delete({timeout: 50 * 1000});
+          } catch (e) {
+            console.log(`Error deleting backup: ${backup.id}`);
+          }
+        });
+      })
+    );
+  }
+
+  async function reapInstances() {
+    const [instances] = await bigtable.getInstances();
+    const testInstances = instances
+      .filter(i => i.id.match(PREFIX))
+      .filter(i => {
+        const timeCreated = i.metadata!.labels!.time_created as {} as Date;
+        // Only delete stale resources.
+        const oneHourAgo = new Date(Date.now() - 3600000);
+        return !timeCreated || timeCreated <= oneHourAgo;
+      });
+    const q = new Q({concurrency: 5});
+    // need to delete backups first due to instance deletion precondition
+    await Promise.all(testInstances.map(instance => reapBackups(instance)));
+    await Promise.all(
+      testInstances.map(instance => {
+        q.add(async () => {
+          try {
+            await instance.delete();
+          } catch (e) {
+            console.log(`Error deleting instance: ${instance.id}`);
+          }
+        });
+      })
+    );
+  }
+
+  before(async () => {
+    await reapInstances();
+    const [, operation] = await INSTANCE.create(
+      createInstanceConfig(CLUSTER_ID, 'us-central1-c', 3, Date.now())
+    );
+
+    await operation.promise();
+    await TABLE.create({
+      families: ['follows', 'traits'],
+    });
+    await INSTANCE.createAppProfile(APP_PROFILE_ID, {
+      routing: 'any',
+      ignoreWarnings: true,
+    });
+  });
+
+  after(async () => {
+    const secondBigtable = new Bigtable();
+    const SECOND_INSTANCE = secondBigtable.instance(instanceId);
+    const q = new Q({concurrency: 5});
+    const instances = [SECOND_INSTANCE];
+
+    // need to delete backups first due to instance deletion precondition
+    await Promise.all(instances.map(instance => reapBackups(instance)));
+    await Promise.all(
+      instances.map(instance => {
+        q.add(async () => {
+          try {
+            await instance.delete();
+          } catch (e) {
+            console.log(`Error deleting instance: ${instance.id}`);
+          }
+        });
+      })
+    );
+  });
+
+  it('Calling close and then sampleRowKeys should tell us the client is closed after client initialization', async () => {
+    await TABLE.sampleRowKeys(); // Initialize the client.
+    // await (bigtable as any)[v2].close();
+    // await bigtable.api.BigtableClient.close();
+    await bigtable.close();
+    try {
+      await TABLE.sampleRowKeys();
+      assert.fail('This call should have resulted in a client closed error');
+    } catch (e) {
+      assert.strictEqual(
+        (e as ServiceError).message,
+        'The client has already been closed.'
+      );
+    }
+  });
+});
+
+function createInstanceConfig(
+  clusterId: string,
+  location: string,
+  nodes: number,
+  time_created: number
+) {
+  return {
+    clusters: [
+      {
+        id: clusterId,
+        location: location,
+        nodes: nodes,
+      },
+    ],
+    labels: {
+      time_created: time_created,
+    },
+  };
+}

system-test/close-client-before-initialization.ts

@@ -0,0 +1,144 @@
+// Copyright 2024 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
+//
+//     https://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, before, after} from 'mocha';
+import Q from 'p-queue';
+
+import {Bigtable, Instance} from '../src';
+import {generateId, PREFIX} from './common';
+import * as assert from 'assert';
+import {ServiceError} from 'google-gax';
+
+describe('CloseClientBeforeInitialization', () => {
+  const instanceId = generateId('instance');
+  const bigtable = new Bigtable();
+  const INSTANCE = bigtable.instance(instanceId);
+  const TABLE = INSTANCE.table(generateId('table'));
+  const APP_PROFILE_ID = generateId('appProfile');
+  const CLUSTER_ID = generateId('cluster');
+
+  async function reapBackups(instance: Instance) {
+    const [backups] = await instance.getBackups();
+    const q = new Q({concurrency: 5});
+    return Promise.all(
+      backups.map(backup => {
+        q.add(async () => {
+          try {
+            await backup.delete({timeout: 50 * 1000});
+          } catch (e) {
+            console.log(`Error deleting backup: ${backup.id}`);
+          }
+        });
+      })
+    );
+  }
+
+  async function reapInstances() {
+    const [instances] = await bigtable.getInstances();
+    const testInstances = instances
+      .filter(i => i.id.match(PREFIX))
+      .filter(i => {
+        const timeCreated = i.metadata!.labels!.time_created as {} as Date;
+        // Only delete stale resources.
+        const oneHourAgo = new Date(Date.now() - 3600000);
+        return !timeCreated || timeCreated <= oneHourAgo;
+      });
+    const q = new Q({concurrency: 5});
+    // need to delete backups first due to instance deletion precondition
+    await Promise.all(testInstances.map(instance => reapBackups(instance)));
+    await Promise.all(
+      testInstances.map(instance => {
+        q.add(async () => {
+          try {
+            await instance.delete();
+          } catch (e) {
+            console.log(`Error deleting instance: ${instance.id}`);
+          }
+        });
+      })
+    );
+  }
+
+  before(async () => {
+    await reapInstances();
+    const [, operation] = await INSTANCE.create(
+      createInstanceConfig(CLUSTER_ID, 'us-central1-c', 3, Date.now())
+    );
+
+    await operation.promise();
+    await TABLE.create({
+      families: ['follows', 'traits'],
+    });
+    await INSTANCE.createAppProfile(APP_PROFILE_ID, {
+      routing: 'any',
+      ignoreWarnings: true,
+    });
+  });
+
+  after(async () => {
+    const secondBigtable = new Bigtable();
+    const SECOND_INSTANCE = secondBigtable.instance(instanceId);
+    const q = new Q({concurrency: 5});
+    const instances = [SECOND_INSTANCE];
+
+    // need to delete backups first due to instance deletion precondition
+    await Promise.all(instances.map(instance => reapBackups(instance)));
+    await Promise.all(
+      instances.map(instance => {
+        q.add(async () => {
+          try {
+            await instance.delete();
+          } catch (e) {
+            console.log(`Error deleting instance: ${instance.id}`);
+          }
+        });
+      })
+    );
+  });
+
+  it('Calling close and then sampleRowKeys should tell us the client is closed before client initialization', async () => {
+    // await (bigtable as any)[v2].close();
+    // await bigtable.api.BigtableClient.close();
+    await bigtable.close();
+    try {
+      await TABLE.sampleRowKeys();
+      assert.fail('This call should have resulted in a client closed error');
+    } catch (e) {
+      assert.strictEqual(
+        (e as ServiceError).message,
+        'The client has already been closed.'
+      );
+    }
+  });
+});
+
+function createInstanceConfig(
+  clusterId: string,
+  location: string,
+  nodes: number,
+  time_created: number
+) {
+  return {
+    clusters: [
+      {
+        id: clusterId,
+        location: location,
+        nodes: nodes,
+      },
+    ],
+    labels: {
+      time_created: time_created,
+    },
+  };
+}

test/index.ts

@@ -26,6 +26,7 @@ import {PassThrough} from 'stream';
 import {RequestOptions} from '../src';
 import * as snapshot from 'snap-shot-it';
 import {createClusterOptionsList} from './constants/cluster';
+import {ServiceError} from 'google-gax';
 
 // eslint-disable-next-line @typescript-eslint/no-var-requires
 const v2 = require('../src/v2');
@@ -1215,18 +1216,27 @@ describe('Bigtable', () => {
     });
   });
   describe('close', () => {
-    it('should have failed request after close is called', done => {
-      bigtable.close().then(() => {
-        bigtable.getInstances((err: Error) => {
-          if (err) {
-            done();
-          } else {
-            assert.fail(
-              'The request did not fail, but it should have because the connection is closed'
-            );
-          }
+    it('should have failed request after close is called', function (done) {
+      this.timeout(1200000); // Closing and initializing the client takes a long time on kokoro.
+      console.log('before close');
+      bigtable
+        .close()
+        .then(() => {
+          console.log('after close');
+          bigtable.getInstances((err: Error) => {
+            console.log('after getInstances');
+            if (err) {
+              done();
+            } else {
+              done(
+                'The request did not fail, but it should have because the connection is closed'
+              );
+            }
+          });
+        })
+        .catch((e: ServiceError) => {
+          done(e);
         });
-      });
     });
   });
 });

testproxy/services/close-client.js

@@ -15,7 +15,7 @@
 
 const normalizeCallback = require('./utils/normalize-callback.js');
 
 const v2 = Symbol.for('v2');
 
 const closeClient = ({clientMap}) =>
   normalizeCallback(async rawRequest => {
@@ -24,7 +24,7 @@
     const bigtable = clientMap.get(clientId);
 
     if (bigtable) {
-      await bigtable[v2].close();
+      // await bigtable[v2].close();
       await bigtable.close();
       return {};
     }