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

feat: add context manager capability to subscriber #32

Merged
merged 2 commits into from Feb 20, 2020

Conversation

@plamut
Copy link
Contributor

@plamut plamut commented Feb 18, 2020

Fixes #4.

This PR gives users a more handy way of closing the subscriber's underlying channel.

How to test

  • Make sure that a topic has a few messages published to it to avoid the issue with a dubious DeadlineExceeded response.
  • List the currently open sockets by Python:
    $ lsof -i -n | grep python
    
  • Run the code snippet from the ticket description in debugger, but by using the subscriber client as a context manager:
    with subscriber:
        ...
    Make sure to place a breakpoint after the with block.
  • After the messages have been pulled and the context manager exited, list the open sockets again.

Actual result (before the fix):
Subscriber client cannot be used as a context manager. Using it without the context results in open sockets being leaked until the Python process terminates,

Expected result (after the fix):
Subscriber client releases the open sockes after exiting the context management block.

Things to consider/discuss

  • I experimented by adding a _close flag to the client and additional logic that would raise an error if any of its methods are used after the client has been closed. Since a lot of the methods are copied from the generated SubscriberClient GAPIC class, that extra logic would bloat the code without that much of a gain (the GRPC channel itself already raises an informative "channel closed" error).
  • I did not implement the __del__() method that would call close() automatically, as this could bring its own set of potential problems.
  • While already at it, should we add similar logic to the publisher client, too? The latter, too, opens two sockets through its underlying GRPC channel.
  • Update code samples in docs to demonstrate the context manager capability as a recommended practice?

PR checklist:

  • Make sure to open an issue as a bug/issue before writing your code! That way we can discuss the change, evaluate designs, and agree on the general idea
  • Ensure the tests and linter pass
  • Code coverage does not decrease (if any source code was changed)
  • Appropriate docs were updated (if necessary)
@tseaver tseaver self-requested a review Feb 18, 2020
tests/system.py Outdated Show resolved Hide resolved
Loading
tests/system.py Show resolved Hide resolved
Loading
@plamut plamut merged commit b58d0d8 into googleapis:master Feb 20, 2020
3 checks passed
Loading
@plamut plamut deleted the iss-4 branch Feb 20, 2020
@pradn
Copy link
Contributor

@pradn pradn commented Feb 20, 2020

I just had a chance to take a look at this. It looks good, thanks!

Loading

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

5 participants