Skip to content
This repository has been archived by the owner on Jan 10, 2023. It is now read-only.

More robust handling of duplicate CLOSE commands from device. #151

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

embray
Copy link

@embray embray commented Mar 17, 2019

For some reason some devices will send a CLSE command multiple times
for the sme connection. It's not clear whether this is a bug in the
device or some poorly-documented "feature" of the adb protocol, or
a bug elsewhere in python-adb (possible, but I can't find it if so).

Nevertheless, the protocol specs state:

> A CLOSE message containing a remote-id which does not map to an
> open stream on the recipient's side is ignored.  The stream may have
> already been closed by the recipient while this message was
> in-flight.

Thus, when opening a new connection, we should ignore any CLSE commands
that may still be on the wire for some reason. The current architecture
does not support multiple interleaved connections, and thus does not
keep track of the remote-ids of previous connections. We could do that,
but another approach is to send a monotonically increasing value for the
local-id of each connection, rather than repeatedly reusing 1. This
allows us to distinguish stray CLSE messages for a previous connection
from CLSE or OKAY messages in response to the current connection by
checking the remote-id (our local-id) of the command from the device.

I observed that an emulated device running Android 8.1 sometimes sent
multiple (2 or even 3) duplicate CLSE commands. Hence the while loop
until we get a command intended for the correct recipient.

@googlebot
Copy link

Thanks for your pull request. It looks like this may be your first contribution to a Google open source project (if not, look below for help). Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA).

📝 Please visit https://cla.developers.google.com/ to sign.

Once you've signed (or fixed any issues), please reply here (e.g. I signed it!) and we'll verify it.


What to do if you already signed the CLA

Individual signers
Corporate signers

ℹ️ Googlers: Go here for more info.

@embray
Copy link
Author

embray commented Mar 17, 2019

I signed it!

@embray
Copy link
Author

embray commented Mar 17, 2019

To be clear, this is a problem I had with connecting to an emulator over TCP. I have not tested whether I can reproduce the similar problem with any of my physical devices over USB. Also I can confirm via packet inspection that nothing else is talking to the emulated device over that TCP port.

Prior to this fix, I would get the crash described in #136 after running 2 or 3 shell commands over adb. With this fix I can do something like:

In [1]: from adb import adb_commands

In [2]: dev = adb_commands.AdbCommands()

In [3]: dev.ConnectDevice(serial='127.0.0.1:5555')
Out[3]: <adb.adb_commands.AdbCommands at 0x6fffdef7c50>

In [4]: while True: print(dev.Shell("cd /vendor && ls -l"))

and it will run forever without incident (until I Ctrl-C, at which point the connection is left in an inconsistent state and does not recover well without making a new connection, but I consider this a separate problem).

@embray
Copy link
Author

embray commented Mar 17, 2019

Some of the tests failed because they didn't expect the increasing connection id value.

For some reason some devices will send a CLSE command multiple times
for the sme connection. It's not clear whether this is a bug in the
device or some poorly-documented "feature" of the adb protocol, or
a bug elsewhere in python-adb (possible, but I can't find it if so).

Nevertheless, the [protocol specs](https://android.googlesource.com/platform/system/core/+/master/adb/protocol.txt) state:

    > A CLOSE message containing a remote-id which does not map to an
    > open stream on the recipient's side is ignored.  The stream may have
    > already been closed by the recipient while this message was
    > in-flight.

Thus, when opening a new connection, we should ignore any CLSE commands
that may still be on the wire for some reason.  The current architecture
does not support multiple interleaved connections, and thus does not
keep track of the remote-ids of previous connections.  We could do that,
but another approach is to send a monotonically increasing value for the
local-id of each connection, rather than repeatedly reusing 1.  This
allows us to distinguish stray CLSE messages for a previous connection
from CLSE or OKAY messages in response to the current connection by
checking the remote-id (our local-id) of the command from the device.

I observed that an emulated device running Android 8.1 sometimes sent
multiple (2 or even 3) duplicate CLSE commands.  Hence the while loop
until we get a command intended for the correct recipient.
@embray embray force-pushed the multiple-close-bug branch from 6cc4d25 to 76ed032 Compare March 17, 2019 15:49
@googlebot
Copy link

CLAs look good, thanks!

ℹ️ Googlers: Go here for more info.

embray added a commit to embray/python-adb that referenced this pull request Mar 17, 2019
@embray embray force-pushed the multiple-close-bug branch from 0ab2ad9 to 063cab1 Compare March 17, 2019 15:57
@coveralls
Copy link

coveralls commented Mar 17, 2019

Coverage Status

Coverage increased (+0.3%) to 43.826% when pulling 063cab1 on embray:multiple-close-bug into d9b94b2 on google:master.

@JeffLIrion
Copy link
Contributor

@embray I tried a fork of this pull request with my 2nd generation Fire TV stick and it works just fine. That said, I wasn't experiencing the issues #59 or #136 prior to incorporating your changes.

I know it's not the most relevant feedback, but at least it didn't break anything!

@embray
Copy link
Author

embray commented May 30, 2019

@JeffLIrion Thanks for checking it! Indeed I've only encountered the double-CLOSE issue myself on one device running Android 8.0, and an emulated Android 8.1. I haven't seen it on devices running older Android versions, though I've only tested a handful.

@JeffLIrion
Copy link
Contributor

@embray here's some feedback from a user with an Android Shield:

https://community.home-assistant.io/t/community-hass-io-add-on-adb-android-debug-bridge/96375/248

Done, the only thing I saw in logs was

Data_length 3787 does not match actual number of bytes read: 2872
Data_length 4096 does not match actual number of bytes read: 1424

Not sure how well it worked since I’m not at home to monitor, but that seemed to be okay.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants