-
Notifications
You must be signed in to change notification settings - Fork 225
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
PubSub.close throws error #937
Comments
// @feywind |
@mad-it @stephenplusplus Thanks! I will take a look in a moment. |
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! |
I'm not sure I like the issue auto-close. Anyway, this PR should fix the problem, can you try again? Specifically the recently released 1.7.1 should contain it. |
@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. |
@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 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();
}); |
Indeed. It looks like |
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. |
@merlinnot @mad-it 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. :) |
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. |
My bad, it works perfectly fine. Thank you! 🎉 |
Thanks for the quick fix! |
This bug report is related to #916.
Environment details
@google-cloud/pubsub
version: 1.7.0Steps to reproduce
Here is a test written to test this newly introduced
close
method.When running this simple test in Jest, I get the following error.
The text was updated successfully, but these errors were encountered: