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)

@plamut plamut added the type: feature request ‘Nice-to-have’ improvement, new feature or different behavior or design. label Feb 18, 2020
@googlebot googlebot added the cla: yes This human has signed the Contributor License Agreement. label Feb 18, 2020
@plamut plamut requested a review from pradn February 18, 2020 15:45
@plamut plamut added the kokoro:force-run Add this label to force Kokoro to re-run the tests. label Feb 18, 2020
@yoshi-kokoro yoshi-kokoro removed the kokoro:force-run Add this label to force Kokoro to re-run the tests. label Feb 18, 2020
tests/system.py Outdated Show resolved Hide resolved
tests/system.py Show resolved Hide resolved
@plamut plamut merged commit b58d0d8 into googleapis:master Feb 20, 2020
@plamut plamut deleted the iss-4 branch February 20, 2020 07:52
@pradn
Copy link
Contributor

pradn commented Feb 20, 2020

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
cla: yes This human has signed the Contributor License Agreement. type: feature request ‘Nice-to-have’ improvement, new feature or different behavior or design.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

PubSub: gRPC channel memory leak/not closed.
5 participants