Skip to content

Remove adb cleanup from snippet and JSON-RPC client finalizers - #1044

Open
xpconanfan wants to merge 1 commit into
masterfrom
remove-client-finalizers
Open

xpconanfan wants to merge 1 commit into
masterfrom
remove-client-finalizers

Conversation

@xpconanfan

Copy link
Copy Markdown
Collaborator

Problem

ClientBase.__del__ and JsonRpcClientBase.__del__ run close_connection() / disconnect(), which issue adb commands (adb forward --list, adb forward --remove). Doing blocking subprocess work from a finalizer is unreliable:

  • It runs whenever the garbage collector decides, on whichever thread triggered the collection (thread-pool workers, the logcat processor thread), and at interpreter shutdown — the source of the Exception ignored in: ClientBase.__del__ noise.
  • After a failed stop(), a stale event client's finalizer can remove the port forward of a new client that reused the same host port.
  • In the unit test suite, leaked clients fire real adb processes long after the tests' adb mocks are gone. Where those land depends on allocation patterns, so an unrelated test-file removal (Remove the deprecated callback_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: AndroidDevice teardown → services.stop_all() stops every snippet client, and unload_snippet / restore_server_connection go through stop() / close_connection(). The socket itself is closed by Python when the object is collected; the finalizers only ever added the adb calls.

Tests

  • The one test that invoked __del__ directly now calls close_connection().
  • Dropped the double-side_effect workaround in jsonrpc_client_base_test.test_disconnect_raises that only existed to survive the finalizer.
  • Real adb launches during a full pytest tests run on master (measured with a temporary subprocess.Popen tracer): 18 → 0, 3/3 runs.

Notes

  • Independent of Remove the deprecated callback_handler #1037; the host_port = None test cleanup added there stays in place (removing it before this lands would re-trigger the CI hang).
  • Release note: snippet / JSON-RPC clients no longer attempt adb cleanup on garbage collection. Call stop() / close_connection(), which AndroidDevice teardown already does.

`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.
@xpconanfan xpconanfan self-assigned this Oct 2, 2026

This branch has not been deployed

No deployments
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant