search

JeffLIrion / adb_shell

A Python implementation of ADB with shell and FileSync functionality.
Apache License 2.0
530 stars 60 forks source link

AdbDeviceTcp.shell() ignores timeouts specfied in AdbDeviceTcp constructor #138

Closed jvmk closed 3 years ago

jvmk commented 3 years ago

Description

Hi Jeff,

Great tool, thanks a ton for making it available!

I'm having trouble specifying custom timeouts. It seems that a call to AdbDeviceTcp.shell() uses/propagates the timeout values provided as arguments to AdbDeviceTcp.shell() even when these parameters are omitted. This results in timeout values provided to the AdbDeviceTcp constructor being overwritten. I'm not sure if this is intentional (I assume not), or if I am using the API wrong.

For example:

# Read ADB keys
pubkey_path = pathlib.Path(pubkey_path).expanduser()
privkey_path = pathlib.Path(privkey_path).expanduser()
with open(pubkey_path) as f:
    pubkey = f.read()
with open(privkey_path) as f:
    privkey = f.read()
adbkeypair = PythonRSASigner(pubkey, privkey)

# Connect to Fire TV, specifying custom timeout values
ftv = AdbDeviceTcp(firetv_ip, firetv_port, default_transport_timeout_s=60.0)
ftv.connect(rsa_keys=[adbkeypair], transport_timeout_s=60.0, auth_timeout_s=60.0, read_timeout_s=60.0)

# Perform a (long) sequence of ADB commands (calls to AdbDeviceTcp.shell()), e.g. let's uninstall all 3rd party apps (assuming you have A LOT installed):
pkgs_str = ftv.shell('pm list packages -3')
pkgs = [p.replace('package:', '', 1) for p in pkgs_str.split('\r\n') if p.startswith('package:')]
for pkgname in pkgs:
    print(f'Uninstalling {pkgname} ...')
    ftv.shell(f'pm uninstall {pkgname}')

Using code similar to the example above (multiple calls to AdbDeviceTcp.shell(), although not just uninstall commands), I occasionally get TcpTimeoutExceptions with a message indicating that the (read) timeout was 10 seconds (the default value) rather than the expected 60 seconds (the value I provided in the AdbDeviceTcp constructor and in AdbDeviceTcp.connect(), although the latter may be unrelated as that's a transport timeout):

adb_shell.exceptions.TcpTimeoutException: Reading from <device_ip>:<device_port> timed out (10.0 seconds)

Platform details (in case it matters)

I'm using adb_shell to control a Fire TV Stick with Alexa Voice Remote (i.e., Fire TV Stick 2nd generation that was reissued early 2019), that runs Fire OS 5 which is based on Android 5.1 (I'm on Fire OS 5.2.7.7 to be exact). This device is rather resource constrained and is in some instances rather slow at responding to ADB commands (including when using the ADB binary directly, not involving adb_shell), thus the default timeout is simply too strict for this particular device.

I'm using the latest adb_shell release:

$ pip show adb_shell
Name: adb-shell
Version: 0.2.3
Summary: A Python implementation of ADB with shell and FileSync functionality.
Home-page: https://github.com/JeffLIrion/adb_shell
Author: Jeff Irion
Author-email: jefflirion@users.noreply.github.com
License: UNKNOWN
Location: /home/username/python_venvs/some_venv/lib/python3.7/site-packages
Requires: rsa, pyasn1, cryptography
Required-by:

Full stack trace

An example of the full stack trace (I replaced local paths and IP addresses with placeholders):

Traceback (most recent call last):
  File "main.py", line 93, in <module>
    firetv_port=args.firetv_port)
  File "main.py", line 56, in main
    ftv_controller.uninstall_app(p)
  File "/home/username/src/firetv_controller.py", line 214, in uninstall_app
    return self.firetv.shell(f'pm uninstall {pkgname}')
  File "/home/username/python_venvs/some_venv/lib/python3.7/site-packages/adb_shell/adb_device.py", line 397, in shell
    return self._service(b'shell', command.encode('utf8'), transport_timeout_s, read_timeout_s, timeout_s, decode)
  File "/home/username/python_venvs/some_venv/lib/python3.7/site-packages/adb_shell/adb_device.py", line 302, in _service
    return b''.join(self._streaming_command(service, command, adb_info)).decode('utf8')
  File "/home/username/python_venvs/some_venv/lib/python3.7/site-packages/adb_shell/adb_device.py", line 940, in _streaming_command
    for data in self._read_until_close(adb_info):
  File "/home/username/python_venvs/some_venv/lib/python3.7/site-packages/adb_shell/adb_device.py", line 876, in _read_until_close
    cmd, data = self._read_until([constants.CLSE, constants.WRTE], adb_info)
  File "/home/username/python_venvs/some_venv/lib/python3.7/site-packages/adb_shell/adb_device.py", line 832, in _read_until
    cmd, remote_id2, local_id2, data = self._read(expected_cmds, adb_info)
  File "/home/username/python_venvs/some_venv/lib/python3.7/site-packages/adb_shell/adb_device.py", line 765, in _read
    msg = self._transport.bulk_read(constants.MESSAGE_SIZE, adb_info.transport_timeout_s)
  File "/home/username/python_venvs/some_venv/lib/python3.7/site-packages/adb_shell/transport/tcp_transport.py", line 127, in bulk_read
    raise TcpTimeoutException(msg)
adb_shell.exceptions.TcpTimeoutException: Reading from <FIRE_TV_IP>:<FIRE_TV_PORT> timed out (10.0 seconds)

Workaround

I've found a hacky workaround that seems to solve the issue for me for now. I manually modified /home/username/python_venvs/some_venv/lib/python3.7/site-packages/adb_shell/adb_device.py, adding the highlighted block of code to AdbDevice.shell():

class AdbDevice(object):
    # ...
    def shell(self, command, transport_timeout_s=None, read_timeout_s=constants.DEFAULT_READ_TIMEOUT_S, timeout_s=None, decode=True):
        if not self.available:
            raise exceptions.AdbConnectionError("ADB command not sent because a connection to the device has not been established.  (Did you call `AdbDevice.connect()`?)")
         # --- NEW CODE ---
        if self._transport is not None and isinstance(self._transport, TcpTransport):
            if transport_timeout_s is None:
                # Use timeout specified in encapsulated TcpTransport if no overwriting transport timeout specified here.
                transport_timeout_s = self._transport._default_transport_timeout_s
            if read_timeout_s == constants.DEFAULT_READ_TIMEOUT_S and self._transport._default_transport_timeout_s is not None:
                # Use timeout specified in encapsulated TcpTransport if no overwriting transport timeout specified here.
                # Note: does not handle the case where the caller explicitly sets read_timeout_s to constants.DEFAULT_READ_TIMEOUT_S
                read_timeout_s = self._transport._default_transport_timeout_s
        # ----------------
        return self._service(b'shell', command.encode('utf8'), transport_timeout_s, read_timeout_s, timeout_s, decode)

This is probably not the right place for this change - I went with the most straightforward solution that seemed to fit my needs. I'm guessing it should probably go in the AdbDeviceTcp subclass s.t. you don't have to do type checks, but I don't understand the codebase well enough to be the judge of that. Also note that the hack seems to fail if the user modifies adb_shell.constants.DEFAULT_READ_TIMEOUT_S, e.g., something like this:

# Attempt to increase the read timeout globally
adb_shell.constants.DEFAULT_READ_TIMEOUT_S = 60.0
# Connect to Fire TV, specifying custom timeout values
ftv = AdbDeviceTcp(firetv_ip, firetv_port, default_transport_timeout_s=60.0)
ftv.connect(rsa_keys=[adbkeypair], transport_timeout_s=60.0, auth_timeout_s=60.0, read_timeout_s=60.0)
pkgname = 'com.my.app'
ftv.shell(f'pm uninstall {pkgname}')

If I recall correctly, AdbDeviceTcp.shell() still gets called with read_timeout_s=10 for this scenario, which makes the if read_timeout_s == constants.DEFAULT_READ_TIMEOUT_S and ... check in the hack above false as constants.DEFAULT_READ_TIMEOUT_S == 60.0.

Once again thanks for providing a great, well-documented tool.

Best, Janus

JeffLIrion commented 3 years ago

I admit that the timeouts are confusing, I'll see if I can clean that up. But I don't think this is a bug...

https://github.com/JeffLIrion/adb_shell/blob/3b3d23f4754322514aab55eef0e3d3f0e6eab057/adb_shell/hidden_helpers.py#L89-L98

https://github.com/JeffLIrion/adb_shell/blob/3b3d23f4754322514aab55eef0e3d3f0e6eab057/adb_shell/hidden_helpers.py#L134-L139

Also note that the hack seems to fail if the user modifies adb_shell.constants.DEFAULT_READ_TIMEOUT_S

First of all, you shouldn't modify that -- it's supposed to be a constant, hence the constants.py module and the all caps name.

Second, I think you would need to modify adb_shell.adb_device.constants.DEFAULT_READ_TIMEOUT_S due to the way the imports are performed. But again, you shouldn't do this.

But most of all, you shouldn't need this workaround in the first place. Try providing all three timeout parameters to the shell() method and see if that fixes the problem. You can use the same value for all three.

JeffLIrion commented 3 years ago

FYI:

This device is rather resource constrained and is in some instances rather slow at responding to ADB commands (including when using the ADB binary directly, not involving adb_shell)

https://www.home-assistant.io/integrations/androidtv/#androidtvlearn_sendevent-for-faster-adb-commands

jvmk commented 3 years ago

Hi Jeff,

I admit that the timeouts are confusing, I'll see if I can clean that up. But I don't think this is a bug...

https://github.com/JeffLIrion/adb_shell/blob/3b3d23f4754322514aab55eef0e3d3f0e6eab057/adb_shell/hidden_helpers.py#L89-L98

https://github.com/JeffLIrion/adb_shell/blob/3b3d23f4754322514aab55eef0e3d3f0e6eab057/adb_shell/hidden_helpers.py#L134-L139

Yes, it might just be that I misunderstood what timeout values are supposed to trump others :). I (incorrectly?) assumed that the timeouts provided to the AdbDeviceTcp constructor and/or the connect() call would apply for .shell() calls if I omitted the timeout parameters when calling .shell().

Also note that the hack seems to fail if the user modifies adb_shell.constants.DEFAULT_READ_TIMEOUT_S

First of all, you shouldn't modify that -- it's supposed to be a constant, hence the constants.py module and the all caps name.

Second, I think you would need to modify adb_shell.adb_device.constants.DEFAULT_READ_TIMEOUT_S due to the way the imports are performed. But again, you shouldn't do this.

Yes, I realize I shouldn't be modifying these constants. I did this as a last resort because I didn't understand why the timeout I specified in the AdbDeviceTcp constructor and connect() call didn't come into effect.

But most of all, you shouldn't need this workaround in the first place. Try providing all three timeout parameters to the shell() method and see if that fixes the problem. You can use the same value for all three.

Makes sense, I'll try this out, cheers.

Thanks again, Janus

JeffLIrion commented 3 years ago

Try providing all three timeout parameters to the shell() method and see if that fixes the problem. You can use the same value for all three.

Did that work?

jvmk commented 3 years ago

Try providing all three timeout parameters to the shell() method and see if that fixes the problem. You can use the same value for all three.

Did that work?

Hi Jeff,

Yes, I just verified that if I set all 3 timeouts in .shell() to 4.0, the timeout does become 4.0 seconds as expected:

adb_shell.exceptions.TcpTimeoutException: Reading from <IP>:<PORT> timed out (4.0 seconds)

Thanks for guiding me to a cleaner solution!

Best, Janus