fix(bigquery): close GAPIC storage transport and auth sessions to prevent socket leaks#17508
Open
chalmerlowe wants to merge 2 commits into
Open
fix(bigquery): close GAPIC storage transport and auth sessions to prevent socket leaks#17508chalmerlowe wants to merge 2 commits into
chalmerlowe wants to merge 2 commits into
Conversation
Contributor
There was a problem hiding this comment.
Code Review
This pull request updates the BigQuery client to close the transport directly via _transport.close() instead of _transport.grpc_channel.close(), updates corresponding unit and system tests, and adds a new TestPropertyGraphReference test class. The review feedback suggests improving the patch_tracked_requests context manager in system tests to avoid closing pre-existing sessions, and refactoring manual debug prints to use idiomatic assertion messages instead.
77adf99 to
f2ec799
Compare
f2ec799 to
43e6eec
Compare
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
This PR resolves resource leaks (specifically open sockets left in the
ESTABLISHEDstate) that occur during client lifecycle operations and credential refreshing in system/unit tests.The Problem
Transport Lifecycle: When closing the BigQuery Storage client, calling
_transport.grpc_channel.close()was insufficient for releasing all network resources. The full_transport.close()method needs to be invoked to tear down the underlying transport channel correctly.Dynamic Auth Sessions: Under certain authentication environments (like Workload Identity/GCE Metadata server inside Kokoro CI), the
google-authlibrary dynamically instantiates helper HTTP sessions to fetch access tokens. These sessions are not owned by the BigQuery client and are not closed automatically, leading to leaked sockets.Flaky Test Assertions: Socket count assertions in system tests were flaky because Python's garbage collection is non-deterministic, meaning sockets remained open in the operating system even after client close calls until a garbage collection cycle swept them.
Changes
Client & Transport Lifecycle:
_transport.close()instead of_transport.grpc_channel.close().Testing Improvements:
patch_tracked_requestsinterceptor to system/unit tests to track and explicitly close all dynamically spawned credential-refreshing HTTP sessions when the test context exits.gc.collect()calls to socket leak verification tests to force synchronous sweeping of unreferenced socket objects before asserting final socket counts.Code Coverage:
# pragma: NO COVERto Python version checks in__init__.pyfor code paths that render a deprecation warning code if attempted to run on Python runtimes <3.10 test matrix (these paths do not run in our CI/CD since we never execute code with the older runtimes).