GeezerVison is showing github.com — fix(bigtable): close accelerator daemon and unregister instance on Table GC by mutianf · Pull Request #19023 · googleapis/google-cloud-python
Conversation
When an unclosed Table, TableAsync, or AcceleratorDaemon is garbage-collected, subprocess.Popen.__del__ runs while proc.stdin is still open and the Go daemon's monitorStdin watchdog is blocked waiting for EOF. Popen.__del__ sees returncode is None and appends proc to subprocess._active, resurrecting proc and its open stdin pipe so the Go daemon subprocess, /tmp/bt-accel-* directory, and client._instance_owners entry are leaked for the lifetime of the Python process.
Register weakref.finalize callbacks on AcceleratorDaemon and _DataApiTarget/_DataApiTargetAsync (detached on explicit close()) so garbage collection closes stdin, reaps the daemon subprocess before Popen.__del__ runs, removes the UDS tempdir, and unregisters the instance from the client.
Sorry, something went wrong.
Uh oh!
There was an error while loading. Please reload this page.
gemini-code-assist[bot] reacted with eyes emoji
…ble GC
Register weakref.finalize callbacks on AcceleratorDaemon and _DataApiTarget/_DataApiTargetAsync so garbage-collected tables and daemons close stdin, reap the Go daemon subprocess before Popen.__del__ runs, remove the UDS tempdir, and unregister the instance from the client.
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.
Code Review
This pull request implements automatic resource cleanup on garbage collection using weakref.finalize for AcceleratorDaemon, TableAsync, and Table instances, ensuring that subprocesses are reaped and registrations are cleaned up even if close() is not explicitly called. The review feedback highlights a critical reference cycle issue: passing self._register_instance_future directly to weakref.finalize prevents the table instances from being garbage-collected. To resolve this, the reviewer suggests passing a weak reference to the future, updating the finalizer to resolve it, and using a custom mock subclass in tests to avoid TypeError since standard mocks do not support weak references.
Sorry, something went wrong.
Uh oh!
There was an error while loading. Please reload this page.
Comment on lines
+1423
to
+1431
|
|
self._instance_finalizer = weakref.finalize(
|
|
|
self,
|
|
|
self._finalize_instance_registration,
|
|
|
self.client,
|
|
|
self.instance_id,
|
|
|
self.app_profile_id,
|
|
|
id(self),
|
|
|
self._register_instance_future,
|
|
|
)
|
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.
Passing self._register_instance_future directly to weakref.finalize creates a reference cycle that prevents the TableAsync instance from ever being garbage collected.
Why this happens:
weakref.finalize objects are stored in a global registry (weakref.finalize._registry) to keep them alive.- This creates a strong reference path:
global registry -> finalize object -> args -> self._register_instance_future.
- Since
self._register_instance_future is a Task/Future wrapping a coroutine of self, it holds a strong reference to self.
- Therefore,
self is reachable from a global root, meaning its reference count will never drop to 0, and the cyclic garbage collector will never collect it.
Solution:
Pass a weak reference to the future (weakref.ref(self._register_instance_future)) instead of the future itself. This breaks the reference cycle and allows the instance to be garbage collected as expected.
|
self._instance_finalizer = weakref.finalize( |
|
self, |
|
self._finalize_instance_registration, |
|
self.client, |
|
self.instance_id, |
|
self.app_profile_id, |
|
id(self), |
|
self._register_instance_future, |
|
) |
|
self._instance_finalizer = weakref.finalize( |
|
self, |
|
self._finalize_instance_registration, |
|
self.client, |
|
self.instance_id, |
|
self.app_profile_id, |
|
id(self), |
|
weakref.ref(self._register_instance_future), |
|
) |
Sorry, something went wrong.
Uh oh!
There was an error while loading. Please reload this page.
Comment on lines
+2524
to
+2534
|
|
@staticmethod
|
|
|
def _finalize_instance_registration(
|
|
|
client: Any,
|
|
|
instance_id: str,
|
|
|
app_profile_id: str | None,
|
|
|
owner_id: int,
|
|
|
register_future: Any,
|
|
|
) -> None:
|
|
|
if register_future:
|
|
|
register_future.cancel()
|
|
|
client._remove_instance_registration(instance_id, app_profile_id, owner_id)
|
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.
Update _finalize_instance_registration to resolve the weak reference to the registration future before attempting to cancel it.
|
@staticmethod |
|
def _finalize_instance_registration( |
|
client: Any, |
|
instance_id: str, |
|
app_profile_id: str | None, |
|
owner_id: int, |
|
register_future: Any, |
|
) -> None: |
|
if register_future: |
|
register_future.cancel() |
|
client._remove_instance_registration(instance_id, app_profile_id, owner_id) |
|
@staticmethod |
|
def _finalize_instance_registration( |
|
client: Any, |
|
instance_id: str, |
|
app_profile_id: str | None, |
|
owner_id: int, |
|
register_future_ref: Any, |
|
) -> None: |
|
register_future = register_future_ref() if register_future_ref else None |
|
if register_future: |
|
register_future.cancel() |
|
client._remove_instance_registration(instance_id, app_profile_id, owner_id) |
Sorry, something went wrong.
Uh oh!
There was an error while loading. Please reload this page.
Comment on lines
+1124
to
+1132
|
|
self._instance_finalizer = weakref.finalize(
|
|
|
self,
|
|
|
self._finalize_instance_registration,
|
|
|
self.client,
|
|
|
self.instance_id,
|
|
|
self.app_profile_id,
|
|
|
id(self),
|
|
|
self._register_instance_future,
|
|
|
)
|
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.
Passing self._register_instance_future directly to weakref.finalize creates a reference cycle that prevents the Table instance from ever being garbage collected.
Why this happens:
weakref.finalize objects are stored in a global registry (weakref.finalize._registry) to keep them alive.- This creates a strong reference path:
global registry -> finalize object -> args -> self._register_instance_future.
- Since
self._register_instance_future is a Task/Future wrapping a coroutine of self, it holds a strong reference to self.
- Therefore,
self is reachable from a global root, meaning its reference count will never drop to 0, and the cyclic garbage collector will never collect it.
Solution:
Pass a weak reference to the future (weakref.ref(self._register_instance_future)) instead of the future itself. This breaks the reference cycle and allows the instance to be garbage collected as expected.
|
self._instance_finalizer = weakref.finalize( |
|
self, |
|
self._finalize_instance_registration, |
|
self.client, |
|
self.instance_id, |
|
self.app_profile_id, |
|
id(self), |
|
self._register_instance_future, |
|
) |
|
self._instance_finalizer = weakref.finalize( |
|
self, |
|
self._finalize_instance_registration, |
|
self.client, |
|
self.instance_id, |
|
self.app_profile_id, |
|
id(self), |
|
weakref.ref(self._register_instance_future), |
|
) |
Sorry, something went wrong.
Uh oh!
There was an error while loading. Please reload this page.
Comment on lines
+2076
to
+2086
|
|
@staticmethod
|
|
|
def _finalize_instance_registration(
|
|
|
client: Any,
|
|
|
instance_id: str,
|
|
|
app_profile_id: str | None,
|
|
|
owner_id: int,
|
|
|
register_future: Any,
|
|
|
) -> None:
|
|
|
if register_future:
|
|
|
register_future.cancel()
|
|
|
client._remove_instance_registration(instance_id, app_profile_id, owner_id)
|
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.
Update _finalize_instance_registration to resolve the weak reference to the registration future before attempting to cancel it.
|
@staticmethod |
|
def _finalize_instance_registration( |
|
client: Any, |
|
instance_id: str, |
|
app_profile_id: str | None, |
|
owner_id: int, |
|
register_future: Any, |
|
) -> None: |
|
if register_future: |
|
register_future.cancel() |
|
client._remove_instance_registration(instance_id, app_profile_id, owner_id) |
|
@staticmethod |
|
def _finalize_instance_registration( |
|
client: Any, |
|
instance_id: str, |
|
app_profile_id: str | None, |
|
owner_id: int, |
|
register_future_ref: Any, |
|
) -> None: |
|
register_future = register_future_ref() if register_future_ref else None |
|
if register_future: |
|
register_future.cancel() |
|
client._remove_instance_registration(instance_id, app_profile_id, owner_id) |
Sorry, something went wrong.
Uh oh!
There was an error while loading. Please reload this page.
|
|
)
|
|
|
fake_health = mock.Mock(close=mock.AsyncMock() if is_async else mock.Mock())
|
|
|
fake_client = mock.Mock(close=mock.AsyncMock() if is_async else mock.Mock())
|
|
|
fake_future = mock.Mock()
|
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.
Since mock.Mock objects do not support weak references by default, passing a standard mock to weakref.ref will raise a TypeError. Use a custom subclass of mock.Mock that defines __slots__ with __weakref__ to allow weak referencing in tests.
class WeakableMock(mock.Mock):
__slots__ = ("__weakref__",)
fake_future = WeakableMock()
Sorry, something went wrong.
Uh oh!
There was an error while loading. Please reload this page.
This branch has not been deployed
No deployments
Reviewers
1 more reviewer
Reviewers whose approvals may not affect merge requirements
Uh oh!
There was an error while loading. Please reload this page.
Assignees
No one assigned
Milestone
No milestone
Development
Successfully merging this pull request may close these issues.
Uh oh!
There was an error while loading. Please reload this page.
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.
|