Skip to content
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

Publisher Client close() method #817

Closed
colmsnowplow opened this issue Nov 12, 2019 · 8 comments · Fixed by #916
Closed

Publisher Client close() method #817

colmsnowplow opened this issue Nov 12, 2019 · 8 comments · Fixed by #916
Assignees
Labels
api: pubsub Issues related to the googleapis/nodejs-pubsub API. type: feature request ‘Nice-to-have’ improvement, new feature or different behavior or design.

Comments

@colmsnowplow
Copy link

Is your feature request related to a problem? Please describe.

There's currently no method to close a client. I'm using the client in a persistent environment, which isn't guaranteed to be actively processing data at any given time.

If there's a delay between messages, there's a chance that the network connection fails to resolve, eg. if the connection is closed on the server side but not client side.

In order to cover this, I need to open a new connection and I have no way to close the stale one, so basically this eventually leads to a proliferation of open but unused sockets.

Describe the solution you'd like

A client.close() method which allows me to terminate a client connection client-side.

Describe alternatives you've considered

Manually closing sockets or terminating the entire environment every time we hit a stale connection are the only real options I can see - neither are desirable for obvious reasons.

@merlinnot
Copy link
Contributor

This is a great feature request, I'd benefit from it too.

There's a similar FR against Firestore: googleapis/nodejs-firestore#769
And a preliminary PR against API Client Generator for TypeScript: googleapis/gapic-generator-typescript#126

@yoshi-automation yoshi-automation added the triage me I really want to be triaged. label Nov 13, 2019
@bcoe bcoe added the type: feature request ‘Nice-to-have’ improvement, new feature or different behavior or design. label Nov 13, 2019
@yoshi-automation yoshi-automation removed the triage me I really want to be triaged. label Nov 13, 2019
@merlinnot
Copy link
Contributor

@bcoe @callmehiphop It was now implemented in the Firestore client and I got impressive gains thanks to it, see googleapis/nodejs-firestore#769 (comment). Is there a chance to prioritize this feature somehow? I have the exact same issue with PubSub, as I had with Firestore.

@bcoe
Copy link
Contributor

bcoe commented Dec 26, 2019

@merlinnot just added to the schedule for our next library group meeting; @feywind probably worth having this on your radar 👍

@colmsnowplow
Copy link
Author

Good to hear this will be discussed!

For the record/in case it's pertinent I read over the issue @merlinnot linked to for firestore, and we face exactly the same problem (although he described it better than I!)

@merlinnot
Copy link
Contributor

@bcoe Did the meeting happen already?

@feywind
Copy link
Collaborator

feywind commented Jan 31, 2020

@merlinnot @colmsnowplow It's been discussed a couple of times. I haven't heard a super definite "yay do it", but I think the consensus is that we're planning to do it. Basically that it probably shouldn't be necessary to use a close() method, but that it will still probably help people for now. It's behind another fire or two though :)

@merlinnot
Copy link
Contributor

@feywind Some edge cases were discussed in this thread: grpc/grpc-node#1083. It's quite lengthy, but it took quite some time to get to a common understanding. Bottom line: there are some scenarios where having a close method is strictly necessary and there's no way around it. That's why a patch was made to @grpc/grpc-js, then googleapis/gapic-generator-typescript and then googleapis/nodejs-firestore.

While I do acknowledge and understand that this is not necessary in most of the scenarios, power users do and will always require it for more advanced scenarios.

@feywind
Copy link
Collaborator

feywind commented Feb 25, 2020

Potentially related: #725

(for the subscriber side)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
api: pubsub Issues related to the googleapis/nodejs-pubsub API. type: feature request ‘Nice-to-have’ improvement, new feature or different behavior or design.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

5 participants