Repository navigation
Conversation
…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.
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
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.
| 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.
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.finalizeobjects 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_futureis a Task/Future wrapping a coroutine ofself, it holds a strong reference toself. - Therefore,
selfis 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), | |
| ) |
| @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.
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) |
| 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.
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.finalizeobjects 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_futureis a Task/Future wrapping a coroutine ofself, it holds a strong reference toself. - Therefore,
selfis 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), | |
| ) |
| @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.
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) |
| ) | ||
| 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.
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()
When an unclosed
Table,TableAsync, orAcceleratorDaemonis garbage-collected,subprocess.Popen.__del__runs whileproc.stdinis still open and the Go daemon'smonitorStdinwatchdog is blocked waiting for EOF.Popen.__del__seesreturncode is Noneand appendsproctosubprocess._active, resurrectingprocand its openstdinpipe so the Go daemon subprocess,/tmp/bt-accel-*directory, andclient._instance_ownersentry are leaked for the lifetime of the Python process.Register
weakref.finalizecallbacks onAcceleratorDaemonand_DataApiTarget/_DataApiTargetAsync(detached on explicitclose()) so garbage collection closesstdin, reaps the daemon subprocess beforePopen.__del__runs, removes the UDS tempdir, and unregisters the instance from the client.