Remove adb cleanup from snippet and JSON-RPC client finalizers - #1044
Open
xpconanfan wants to merge 1 commit into
Open
xpconanfan wants to merge 1 commit into
xpconanfan wants to merge 1 commit into
Conversation
`ClientBase.__del__` and `JsonRpcClientBase.__del__` ran
`close_connection()` / `disconnect()`, which issue adb commands
(`adb forward --list`, `adb forward --remove`). Doing blocking subprocess
work from a finalizer is unreliable and has caused real problems:
* It runs whenever the garbage collector decides, on whichever thread
triggered the collection, including thread-pool workers and the logcat
processor thread, and at interpreter shutdown ("Exception ignored in
ClientBase.__del__" noise).
* After a failed `stop()`, a stale event client could later remove the
port forward of a *new* client that reused the same host port.
* In the unit test suite, leaked clients fired real `adb` processes long
after the tests' adb mocks were gone. Where those landed depended on
allocation patterns, so an unrelated test-file removal (#1037) made the
Windows + Python 3.14 CI job hang.
Explicit cleanup is already the supported path: `AndroidDevice` teardown
calls `services.stop_all()`, which stops every snippet client, and
`unload_snippet` / `restore_server_connection` go through `stop()` /
`close_connection()` as well. The socket itself is closed by Python when
the object is collected, so the finalizers only ever added the adb calls.
Tests: update the one test that invoked `__del__` directly to call
`close_connection()`, and drop the now-unneeded double `side_effect`
workaround in `jsonrpc_client_base_test`. Real `adb` launches during a
full `pytest tests` run on master go from 18 to 0.
This branch has not been deployed
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.
Problem
ClientBase.__del__andJsonRpcClientBase.__del__runclose_connection()/disconnect(), which issue adb commands (adb forward --list,adb forward --remove). Doing blocking subprocess work from a finalizer is unreliable:Exception ignored in: ClientBase.__del__noise.stop(), a stale event client's finalizer can remove the port forward of a new client that reused the same host port.adbprocesses long after the tests' adb mocks are gone. Where those land depends on allocation patterns, so an unrelated test-file removal (Remove the deprecatedcallback_handler#1037) was enough to hang the Windows + Python 3.14 CI job.Fix
Delete the two
__del__methods. Explicit cleanup is already the supported path:AndroidDeviceteardown →services.stop_all()stops every snippet client, andunload_snippet/restore_server_connectiongo throughstop()/close_connection(). The socket itself is closed by Python when the object is collected; the finalizers only ever added the adb calls.Tests
__del__directly now callsclose_connection().side_effectworkaround injsonrpc_client_base_test.test_disconnect_raisesthat only existed to survive the finalizer.adblaunches during a fullpytest testsrun on master (measured with a temporarysubprocess.Popentracer): 18 → 0, 3/3 runs.Notes
callback_handler#1037; thehost_port = Nonetest cleanup added there stays in place (removing it before this lands would re-trigger the CI hang).stop()/close_connection(), whichAndroidDeviceteardown already does.