Closed Bug 1481699 Opened 3 years ago Closed 3 years ago

Send 'host:kill' to adb server to stop the server

Categories

(DevTools :: about:debugging, enhancement)

enhancement
Not set
normal

Tracking

(firefox63 fixed)

RESOLVED FIXED
Firefox 63
Tracking Status
firefox63 --- fixed

People

(Reporter: hiro, Assigned: hiro)

References

Details

Attachments

(5 files)

Currently when we stop the adb server, we do spawn a new adb process with 'kill-server' argument, but I happened to discover there is a command to stop the server, that is 'host:kill' [1].  With this command, we don't need to spawn a new process and wait for the process finishes so that we can make our code simpler.

[1] http://androidxref.com/4.0.4/xref/system/core/adb/SERVICES.TXT#14
Since it's actually a child of SocketServer.BaseRequestHandler.
Because the genuine adb binary does it [1].  And doing it before shutdown
thread is created to make sure it's sent to the client.

[1] https://android.googlesource.com/platform/system/core/+/4039051d6ddb02324204930fb86d9d0fe405b3fb/adb/adb.cpp#1055

Depends on D3167
In the genuine adb binary, when it reveices 'host:kill' command, it exits the
server process soon [1].  Whereas, in our mock adb.py which is based on
SocketServer.TCPServer in python, when we calls
SocketServer.TCPServer.shutdown() in the case of 'kill-server' command, the
server process doesn't exit soon since the shutdown() function just sets a
flag [2] and serve_forever() polls it [3] every |poll_interval| seconds,
|poll_interval| is 0.5 seconds by default.  Thus it's possible that new incoming
requests are processed during polling.

[1] https://android.googlesource.com/platform/system/core/+/4039051d6ddb02324204930fb86d9d0fe405b3fb/adb/adb.cpp#1049
[2] https://github.com/python/cpython/blob/65b5ef02ec1f44e3a19b689a1ecf73d01c82161b/Lib/socketserver.py#L248
[3] https://docs.python.org/2/library/socketserver.html#SocketServer.BaseServer.serve_forever

Depends on D3168
Comment on attachment 8999465 [details]
Bug 1481699 - Send host:kill command through TCP socket instead of spawning a new adb process with 'kill-server'. r=jdescottes

Julian Descottes [:jdescottes][:julian] has approved the revision.
Attachment #8999465 - Flags: review+
Comment on attachment 8999464 [details]
Bug 1481699 - Do not process any further data after we started shutting down the ADB server. r=jdescottes

Julian Descottes [:jdescottes][:julian] has approved the revision.
Attachment #8999464 - Flags: review+
Comment on attachment 8999463 [details]
Bug 1481699 - Send back 'OKAY' to the client when the adb server received 'kill-server' command. r=jdescottes

Julian Descottes [:jdescottes][:julian] has approved the revision.
Attachment #8999463 - Flags: review+
Comment on attachment 8999462 [details]
Bug 1481699 - Rename ADBServer to ADBRequestHandler. r=jdescottes

Julian Descottes [:jdescottes][:julian] has approved the revision.
Attachment #8999462 - Flags: review+
Pushed by hikezoe@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/671ad07b1645
Rename ADBServer to ADBRequestHandler. r=jdescottes
https://hg.mozilla.org/integration/mozilla-inbound/rev/147066d870a2
Send back 'OKAY' to the client when the adb server received 'kill-server' command. r=jdescottes
https://hg.mozilla.org/integration/mozilla-inbound/rev/4e70c4d1dfc3
Do not process any further data after we started shutting down the ADB server. r=jdescottes
https://hg.mozilla.org/integration/mozilla-inbound/rev/784bc6605eeb
Send host:kill command through TCP socket instead of spawning a new adb process with 'kill-server'. r=jdescottes
Attached patch A followup-fixSplinter Review
Gah, I did push a revision which didn't including changes addressed Julian's review comments. :/

Julian would you mind taking a look this?
Attachment #8999758 - Flags: review?(jdescottes)
Comment on attachment 8999758 [details] [diff] [review]
A followup-fix

Review of attachment 8999758 [details] [diff] [review]:
-----------------------------------------------------------------

Looks good, thanks for catching this!

::: devtools/shared/adb/adb.js
@@ +139,4 @@
>      try {
>        await this.runCommand("host:kill");
>      } catch (e) {
> +      dumpn("Failed to sending host:kill command");

nit: Failed to sending -> Failed to send
Attachment #8999758 - Flags: review?(jdescottes) → review+
Pushed by hikezoe@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/e4f165c26370
Merge two if statements for 'start-server' into a single one.  r=jdescottes
Thank for this additional review that wasn't necessary. :)
You need to log in before you can comment on or make changes to this bug.