Nodejs-pubsub: PubSub.close throws error

[mad-it]

This bug report is related to https://github.com/googleapis/nodejs-pubsub/pull/916.

Environment details

• OS: Linux
• Node.js version: v10.16.3
• npm version: 6.9.0
• @google-cloud/pubsub version: 1.7.0

Steps to reproduce

Here is a test written to test this newly introduced close method.

import { PubSub } from '@google-cloud/pubsub';

it('test pubsub close', async (): Promise<void> => {
  expect.assertions(1);
  const topicName = 'test-topic';

  const pubSub = new PubSub();

  const [topic] = await pubSub.topic(topicName).get({ autoCreate: true });

  await topic.publish(Buffer.from('test-message'));

  expect(await pubSub.topic(topicName).exists()).toStrictEqual([true]);

  await pubSub.close();
});

When running this simple test in Jest, I get the following error.

 FAIL  src/test.spec.ts (7.801s)
  ✕ test pubsub close (106ms)

  ● test pubsub close

    TypeError: gaxClient.close is not a function

      13 |   expect(await pubSub.topic(topicName).exists()).toStrictEqual([true]);
      14 | 
    > 15 |   await pubSub.close();
         |                ^
      16 | });
      17 | 

      at PubSub.closeAllClients_ (node_modules/@google-cloud/pubsub/src/pubsub.ts:983:31)
      at PubSub.close (node_modules/@google-cloud/pubsub/src/pubsub.ts:302:12)
      at PromiseCtor (node_modules/@google-cloud/promisify/build/src/index.js:69:28)
      at PubSub.wrapper (node_modules/@google-cloud/promisify/build/src/index.js:54:16)
      at Object.it (src/test.spec.ts:15:16)

[stephenplusplus]

// @feywind

[feywind]

@mad-it @stephenplusplus Thanks! I will take a look in a moment.

[feywind]

Well, this will teach me to not write a system test for a feature. :S The Pub/Sub client API objects are hand-written and not using a standard generated gax client, and this gets cast to a standard gax interface, which is really confusing. I'll work on getting this piped through tomorrow and add a system test based on the case above.

Thanks again for the report!

[feywind]

I'm not sure I like the issue auto-close. Anyway, this PR should fix the problem, can you try again?
https://github.com/googleapis/nodejs-pubsub/pull/941

Specifically the recently released 1.7.1 should contain it.

[merlinnot]

@feywind Were you able to run it yourself? I just tried, made sure I have the latest version, but I still see the same error.

[mad-it]

@feywind it looks like this issue has been resolved for the very specific test case provided, but the same issue occurs when a subscriber is created.

By adding await topic.subscription('subscriber-1').get({ autoCreate: true }); to the test case above we get the exact same error.

So the complete test case becomes:

import { PubSub } from '@google-cloud/pubsub';

it('test pubsub close', async (): Promise<void> => {
  expect.assertions(1);

  const topicName = 'test-topic';

  const pubSub = new PubSub();

  const [topic] = await pubSub.topic(topicName).get({ autoCreate: true });

  await topic.subscription('subscriber-1').get({ autoCreate: true });

  await topic.publish(Buffer.from('test-message'));

  expect(await pubSub.topic(topicName).exists()).toStrictEqual([true]);

  await pubSub.close();
});

[merlinnot]

Indeed. It looks like PublisherClient has the close method, but SubscriberClient does not.

[feywind]

Hmm, that's interesting - the subscriber version was already there before my work on the publisher, so I didn't spend much time with it. It's possible that there was some code rot in there from the underlying gapic generation, too. I'll take a look at that.

[feywind]

@merlinnot @mad-it
1.7.2 adds the close() method to the SubscriberClient also. Let me know if that works for you? I think that should be all of them, since there are only stubs for publisher and subscriber. There is another system-test that will validate that this one works going forward as well.

We're in the process of releasing 2.0.0 that will bring a lot of dependencies up to date, and also introduce TypeScript generated service stubs that have a close() method built in. I wanted to backport this to the existing 1.x branch though... it doesn't seem like a good idea to require a major version update for a bug fix. :)

[merlinnot]

Thanks for fixing it so quickly. It doesn't crash now, but it also doesn't seem to close gRPC connections. I'll try to create a minimal reproduction and report it in a separate issue, this one can be considered closed.

[merlinnot]

My bad, it works perfectly fine. Thank you! 🎉

[mad-it]

Thanks for the quick fix!