-
Notifications
You must be signed in to change notification settings - Fork 6.4k
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
Pub/Sub: add SubscriberClient.close() to examples #3118
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM with SME approval from @hongalex or @feywind, tests are passing and style looks good. I noticed you're using different patterns in some snippets:
subscriber = pubsub_v1.SubScriberClient()
...
subscriber.close()
versus
subscriber = pubsub_v1.SubscriberClient()
with subscriber:
try:
streaming_pull_future.result(timeout=timeout)
except:
streaming_pull_future.cancel()
And just want to make sure you're intentionally doing this (it looks like the first pattern you're using in synchronous operations and the second for streaming).
Thanks for reviewing @gguuss! That is intended. They are interchangeable. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I admit I don't know much about the Python client libraries, but nothing looks scary in there.
New PR based on @pradn's #2959.
Close subscriber clients after use either explicitly by calling the
close()
method or implicitly by wrapping them in a context manager.