Closed
Bug 1481699
Opened 5 years ago
Closed 5 years ago
Send 'host:kill' to adb server to stop the server
Categories
(DevTools :: about:debugging, enhancement)
DevTools
about:debugging
Tracking
(firefox63 fixed)
RESOLVED
FIXED
Firefox 63
Tracking | Status | |
---|---|---|
firefox63 | --- | fixed |
People
(Reporter: hiro, Assigned: hiro)
References
Details
Attachments
(5 files)
46 bytes,
text/x-phabricator-request
|
jdescottes
:
review+
|
Details | Review |
46 bytes,
text/x-phabricator-request
|
jdescottes
:
review+
|
Details | Review |
46 bytes,
text/x-phabricator-request
|
jdescottes
:
review+
|
Details | Review |
46 bytes,
text/x-phabricator-request
|
jdescottes
:
review+
|
Details | Review |
1.82 KB,
patch
|
jdescottes
:
review+
|
Details | Diff | Splinter Review |
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
Assignee | ||
Comment 1•5 years ago
|
||
https://treeherder.mozilla.org/#/jobs?repo=try&revision=49d59fdce3e1c736f4bbd2db425ac64fa9fcba5d
Assignee: nobody → hikezoe
Status: NEW → ASSIGNED
Assignee | ||
Comment 2•5 years ago
|
||
Since it's actually a child of SocketServer.BaseRequestHandler.
Assignee | ||
Comment 3•5 years ago
|
||
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
Assignee | ||
Comment 4•5 years ago
|
||
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
Assignee | ||
Comment 5•5 years ago
|
||
Depends on D3169
Comment 6•5 years ago
|
||
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 7•5 years ago
|
||
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 8•5 years ago
|
||
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 9•5 years ago
|
||
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+
Assignee | ||
Comment 10•5 years ago
|
||
Thank you Julian for the review! This will be a final try; https://treeherder.mozilla.org/#/jobs?repo=try&revision=3235c122a9b42e9fcee5ebfcb2332e529f3ee96c
Comment 11•5 years ago
|
||
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
Comment 12•5 years ago
|
||
bugherder |
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
Status: ASSIGNED → RESOLVED
Closed: 5 years ago
status-firefox63:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 63
Assignee | ||
Comment 13•5 years ago
|
||
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 14•5 years ago
|
||
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+
Comment 15•5 years ago
|
||
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
Assignee | ||
Comment 16•5 years ago
|
||
Thank for this additional review that wasn't necessary. :)
Comment 17•5 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/e4f165c26370
You need to log in
before you can comment on or make changes to this bug.
Description
•