AdbDeviceTcp.shell() ignores timeouts specfied in AdbDeviceTcp constructor

[jefflirion]

AdbDeviceTcp.shell() ignores timeouts specfied in AdbDeviceTcp constructor

[JeffLIrion]

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

[JeffLIrion]

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...

adb_shell/adb_shell/hidden_helpers.py

Lines 89 to 98 in 3b3d23f

	class _AdbTransactionInfo(object): # pylint: disable=too-few-public-methods
	"""A class for storing info and settings used during a single ADB "transaction."
	Note that if ``timeout_s`` is not ``None``, then:
	::
	self.transport_timeout_s <= self.read_timeout_s <= self.timeout_s
	If ``timeout_s`` is ``None``, the first inequality still applies.

adb_shell/adb_shell/hidden_helpers.py

Lines 134 to 139 in 3b3d23f

	def __init__(self, local_id, remote_id, transport_timeout_s=None, read_timeout_s=constants.DEFAULT_READ_TIMEOUT_S, timeout_s=None):
	self.local_id = local_id
	self.remote_id = remote_id
	self.timeout_s = timeout_s
	self.read_timeout_s = read_timeout_s if self.timeout_s is None else min(read_timeout_s, self.timeout_s)
	self.transport_timeout_s = self.read_timeout_s if transport_timeout_s is None else min(transport_timeout_s, self.read_timeout_s)

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.

[jvmk]

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...

adb_shell/adb_shell/hidden_helpers.py

Lines 89 to 98 in 3b3d23f

	class _AdbTransactionInfo(object): # pylint: disable=too-few-public-methods
	"""A class for storing info and settings used during a single ADB "transaction."
	Note that if ``timeout_s`` is not ``None``, then:
	::
	self.transport_timeout_s <= self.read_timeout_s <= self.timeout_s
	If ``timeout_s`` is ``None``, the first inequality still applies.

adb_shell/adb_shell/hidden_helpers.py

Lines 134 to 139 in 3b3d23f

	def __init__(self, local_id, remote_id, transport_timeout_s=None, read_timeout_s=constants.DEFAULT_READ_TIMEOUT_S, timeout_s=None):
	self.local_id = local_id
	self.remote_id = remote_id
	self.timeout_s = timeout_s
	self.read_timeout_s = read_timeout_s if self.timeout_s is None else min(read_timeout_s, self.timeout_s)
	self.transport_timeout_s = self.read_timeout_s if transport_timeout_s is None else min(transport_timeout_s, self.read_timeout_s)

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]

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]

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