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

Status: RESOLVED FIXED

(DevTools :: about:debugging, enhancement)

Description

[Hiroyuki Ikezoe (:hiro)]

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

Comment 1

[Hiroyuki Ikezoe (:hiro)]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=49d59fdce3e1c736f4bbd2db425ac64fa9fcba5d

Comment 2

[Hiroyuki Ikezoe (:hiro)]

Attachment: Bug 1481699 - Rename ADBServer to ADBRequestHandler. r=jdescottes

Since it's actually a child of SocketServer.BaseRequestHandler.

Comment 3

[Hiroyuki Ikezoe (:hiro)]

Attachment: Bug 1481699 - Send back 'OKAY' to the client when the adb server received 'kill-server' command. r=jdescottes

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

Comment 4

[Hiroyuki Ikezoe (:hiro)]

Attachment: Bug 1481699 - Do not process any further data after we started shutting down the ADB server. r=jdescottes

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 5

[Hiroyuki Ikezoe (:hiro)]

Attachment: Bug 1481699 - Send host:kill command through TCP socket instead of spawning a new adb process with 'kill-server'. r=jdescottes

Depends on D3169

Comment 6

[Julian Descottes [:jdescottes]]

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.

Comment 7

[Julian Descottes [:jdescottes]]

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.

Comment 8

[Julian Descottes [:jdescottes]]

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.

Comment 9

[Julian Descottes [:jdescottes]]

Comment on attachment 8999462 [details]
Bug 1481699 - Rename ADBServer to ADBRequestHandler. r=jdescottes

Julian Descottes [:jdescottes][:julian] has approved the revision.

Comment 10

[Hiroyuki Ikezoe (:hiro)]

Thank you Julian for the review!

This will be a final try;
https://treeherder.mozilla.org/#/jobs?repo=try&revision=3235c122a9b42e9fcee5ebfcb2332e529f3ee96c

Comment 11

[Pulsebot]

Pushed by [e-mail]:
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

Comment 12

[Natalia Csoregi [:nataliaCs]]

https://hg.mozilla.org/mozilla-central/rev/671ad07b1645
https://hg.mozilla.org/mozilla-central/rev/147066d870a2
https://hg.mozilla.org/mozilla-central/rev/4e70c4d1dfc3
https://hg.mozilla.org/mozilla-central/rev/784bc6605eeb

Comment 13

[Hiroyuki Ikezoe (:hiro)]

Attachment: A followup-fix

Gah, I did push a revision which didn't including changes addressed Julian's review comments. :/

Julian would you mind taking a look this?

Comment 14

[Julian Descottes [: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

Comment 15

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/e4f165c26370
Merge two if statements for 'start-server' into a single one.  r=jdescottes

Comment 16

[Hiroyuki Ikezoe (:hiro)]

Thank for this additional review that wasn't necessary. :)

Comment 17

[Andreea Pavel [:apavel]]

https://hg.mozilla.org/mozilla-central/rev/e4f165c26370