Skip to content

Commit

Permalink
Fix thread lock in 'connect()' method; improve exception handling
Browse files Browse the repository at this point in the history
  • Loading branch information
@JeffLIrion
JeffLIrion committed Nov 9, 2019
1 parent 2c2e140 commit 9377e98
Show file tree
Hide file tree
Showing 3 changed files with 94 additions and 66 deletions.
137 changes: 74 additions & 63 deletions androidtv/adb_manager.py
View file
Expand Up @@ -7,7 +7,6 @@


import logging
from socket import error as socket_error
import sys
import threading

Expand Down Expand Up @@ -82,50 +81,60 @@ def connect(self, always_log_errors=True, auth_timeout_s=DEFAULT_AUTH_TIMEOUT_S)
Whether or not the connection was successfully established and the device is available
"""
self._adb_lock.acquire(**LOCK_KWARGS) # pylint: disable=unexpected-keyword-arg

# Make sure that we release the lock
try:
# Catch errors
if self._adb_lock.acquire(**LOCK_KWARGS): # pylint: disable=unexpected-keyword-arg
# Make sure that we release the lock
try:
if self.adbkey:
# private key
with open(self.adbkey) as f:
priv = f.read()

# public key
try:
with open(self.adbkey + '.pub') as f:
pub = f.read()
except FileNotFoundError:
pub = ''

signer = PythonRSASigner(pub, priv)

# Connect to the device
self._adb.connect(rsa_keys=[signer], auth_timeout_s=auth_timeout_s)
else:
self._adb.connect(auth_timeout_s=auth_timeout_s)

# ADB connection successfully established
self._available = True
_LOGGER.debug("ADB connection to %s successfully established", self.host)

except socket_error as serr:
if self._available or always_log_errors:
if serr.strerror is None:
serr.strerror = "Timed out trying to connect to ADB device."
_LOGGER.warning("Couldn't connect to host %s, error: %s", self.host, serr.strerror)

# ADB connection attempt failed
self._adb.close()
self._available = False
# Catch errors
try:
if self.adbkey:
# private key
with open(self.adbkey) as f:
priv = f.read()

# public key
try:
with open(self.adbkey + '.pub') as f:
pub = f.read()
except FileNotFoundError:
pub = ''

signer = PythonRSASigner(pub, priv)

# Connect to the device
self._adb.connect(rsa_keys=[signer], auth_timeout_s=auth_timeout_s)
else:
self._adb.connect(auth_timeout_s=auth_timeout_s)

# ADB connection successfully established
self._available = True
_LOGGER.debug("ADB connection to %s successfully established", self.host)

except OSError as exc:
if self._available or always_log_errors:
if exc.strerror is None:
exc.strerror = "Timed out trying to connect to ADB device."
_LOGGER.warning("Couldn't connect to host %s. %s: %s", self.host, exc.__class__.__name__, exc.strerror)

# ADB connection attempt failed
self._adb.close()
self._available = False

except Exception as exc: # pylint: disable=broad-except
if self._available or always_log_errors:
_LOGGER.warning("Couldn't connect to host %s. %s: %s", self.host, exc.__class__.__name__, exc)

# ADB connection attempt failed
self._adb.close()
self._available = False

finally:
return self._available

finally:
return self._available
self._adb_lock.release()

finally:
self._adb_lock.release()
self._available = False
return False

def shell(self, cmd):
"""Send an ADB command using the Python ADB implementation.
Expand Down Expand Up @@ -249,37 +258,39 @@ def connect(self, always_log_errors=True):
Whether or not the connection was successfully established and the device is available
"""
self._adb_lock.acquire(**LOCK_KWARGS) # pylint: disable=unexpected-keyword-arg

# Make sure that we release the lock
try:
if self._adb_lock.acquire(**LOCK_KWARGS): # pylint: disable=unexpected-keyword-arg
# Make sure that we release the lock
try:
self._adb_client = Client(host=self.adb_server_ip, port=self.adb_server_port)
self._adb_device = self._adb_client.device(self.host)
try:
self._adb_client = Client(host=self.adb_server_ip, port=self.adb_server_port)
self._adb_device = self._adb_client.device(self.host)

# ADB connection successfully established
if self._adb_device:
_LOGGER.debug("ADB connection to %s via ADB server %s:%s successfully established", self.host, self.adb_server_ip, self.adb_server_port)
self._available = True
# ADB connection successfully established
if self._adb_device:
_LOGGER.debug("ADB connection to %s via ADB server %s:%s successfully established", self.host, self.adb_server_ip, self.adb_server_port)
self._available = True

# ADB connection attempt failed (without an exception)
else:
# ADB connection attempt failed (without an exception)
else:
if self._available or always_log_errors:
_LOGGER.warning("Couldn't connect to host %s via ADB server %s:%s", self.host, self.adb_server_ip, self.adb_server_port)
self._available = False

except Exception as exc: # noqa pylint: disable=broad-except
if self._available or always_log_errors:
_LOGGER.warning("Couldn't connect to host %s via ADB server %s:%s", self.host, self.adb_server_ip, self.adb_server_port)
self._available = False
_LOGGER.warning("Couldn't connect to host %s via ADB server %s:%s, error: %s", self.host, self.adb_server_ip, self.adb_server_port, exc)

except Exception as exc: # noqa pylint: disable=broad-except
if self._available or always_log_errors:
_LOGGER.warning("Couldn't connect to host %s via ADB server %s:%s, error: %s", self.host, self.adb_server_ip, self.adb_server_port, exc)
# ADB connection attempt failed
self._available = False

# ADB connection attempt failed
self._available = False
finally:
return self._available

finally:
return self._available
self._adb_lock.release()

finally:
self._adb_lock.release()
self._available = False
return False

def shell(self, cmd):
"""Send an ADB command using an ADB server.
Expand Down
7 changes: 4 additions & 3 deletions tests/patchers.py
View file
@@ -1,5 +1,3 @@
from socket import error as socket_error

try:
# Python3
from unittest.mock import patch
Expand Down Expand Up @@ -83,13 +81,16 @@ def connect_success_python(self, *args, **kwargs):

def connect_fail_python(self, *args, **kwargs):
"""Mock the `AdbDeviceFake.connect` method when it fails."""
raise socket_error
raise OSError

if success:
return {'python': patch('{}.AdbDeviceFake.connect'.format(__name__), connect_success_python), 'server': patch('androidtv.adb_manager.Client', ClientFakeSuccess)}
return {'python': patch('{}.AdbDeviceFake.connect'.format(__name__), connect_fail_python), 'server': patch('androidtv.adb_manager.Client', ClientFakeFail)}


PATCH_CONNECT_PYTHON_FAIL_EXCEPTION = patch('{}.AdbDeviceFake.connect'.format(__name__), side_effect=Exception)


def patch_shell(response=None, error=False):
"""Mock the `AdbDeviceFake.shell` and `DeviceFake.shell` methods."""

Expand Down
16 changes: 16 additions & 0 deletions tests/test_adb_manager.py
View file
Expand Up @@ -93,6 +93,22 @@ def test_connect_fail(self):
self.assertFalse(self.adb.available)
self.assertFalse(self.adb._available)

if self.PATCH_KEY == 'python':
with patchers.PATCH_CONNECT_PYTHON_FAIL_EXCEPTION:
self.assertFalse(self.adb.connect())
self.assertFalse(self.adb.available)
self.assertFalse(self.adb._available)

def test_connect_fail_lock(self):
"""Test when the connect attempt fails due to the lock.
"""
with patchers.patch_connect(True)[self.PATCH_KEY]:
with patch.object(self.adb, '_adb_lock', LockedLock):
self.assertFalse(self.adb.connect())
self.assertFalse(self.adb.available)
self.assertFalse(self.adb._available)

def test_adb_shell_fail(self):
"""Test when an ADB command is not sent because the device is unavailable.
Expand Down

0 comments on commit 9377e98

Please sign in to comment.