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

fix(bigquery): add close() method to client for releasing open sockets #9894

Merged
merged 5 commits into from
Nov 27, 2019

Conversation

plamut
Copy link
Contributor

@plamut plamut commented Nov 26, 2019

Fixes #9790.

This PR fixes the client leaking open sockets by adding a close() method to the client for cleaning up after itself. It also fixes a similar leak in IPython magics - even if bqstorage client is used - when cell magic is run.

How to test

Run both code samples from the issue description, i.e. the one using magics, and the other one using a regular BigQuery client. In both cases the number of open connections at the end should be the same as at the beginning, meaning that no connections (sockets) are leaked.

Misc.

The magics._cell_magic() needs refactoring, it's very long and several helper methods should be extracted from it. But that's out of this PR's scope.

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 api: bigquery Issues related to the BigQuery API. label Nov 26, 2019
@plamut plamut requested review from tswast and a team November 26, 2019 23:22
@googlebot googlebot added the cla: yes This human has signed the Contributor License Agreement. label Nov 26, 2019
Copy link
Contributor

@tswast tswast left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Tests look great! A few suggestions.

bigquery/google/cloud/bigquery/magics.py Show resolved Hide resolved
bigquery/setup.py Outdated Show resolved Hide resolved
A single common cleanup point at the end makes it much less likely
to accidentally re-introduce an open socket leak.
@plamut plamut requested a review from tswast November 27, 2019 13:36
@plamut
Copy link
Contributor Author

plamut commented Nov 27, 2019

@tswast Another thought - since the cleanup only happens if the user calls .close(), we should probably update our code samples to include the client.close() line at the end. Otherwise I presume that many users will simply forget or not know about this.

@tswast
Copy link
Contributor

tswast commented Nov 27, 2019

I'm working on a proposal to add close() to all clients and make clients act as a context manager. Once this is done, we'll likely need to update our sample rubric to account for this.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
api: bigquery Issues related to the BigQuery API. cla: yes This human has signed the Contributor License Agreement.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

BigQuery: resource leaks in client and %%bigquery magics
3 participants