From 79783fa09a8ed0fd3d0a15f40a6d2afc64f54c4a Mon Sep 17 00:00:00 2001 From: douglas-raillard-arm Date: Wed, 12 May 2021 16:46:40 +0100 Subject: [PATCH] target: Create new connection for reentrant calls When Target.conn property is required while the current connection is already in use, provide a fresh connection to avoid deadlocks. This is enabled by the @call_conn decorator that is used on all Target methods that use self.conn directly. --- devlib/target.py | 59 ++++++++++++++++++++++++++++++++++++++++++++ devlib/utils/misc.py | 47 ++++++++++++++++++++++++++++++++++- 2 files changed, 105 insertions(+), 1 deletion(-) diff --git a/devlib/target.py b/devlib/target.py index 1529d41..3b26a95 100644 --- a/devlib/target.py +++ b/devlib/target.py @@ -76,6 +76,48 @@ GOOGLE_DNS_SERVER_ADDRESS = '8.8.8.8' installed_package_info = namedtuple('installed_package_info', 'apk_path package') + +def call_conn(f): + """ + Decorator to be used on all :class:`devlib.target.Target` methods that + directly use a method of ``self.conn``. + + This ensures that if a call to any of the decorated method occurs while + executing, a new connection will be created in order to avoid possible + deadlocks. This can happen if e.g. a target's method is called from + ``__del__``, which could be executed by the garbage collector, interrupting + another call to a method of the connection instance. + + .. note:: This decorator could be applied directly to all methods with a + metaclass or ``__init_subclass__`` but it could create issues when + passing target methods as callbacks to connections' methods. + """ + + @functools.wraps(f) + def wrapper(self, *args, **kwargs): + reentered = self.conn.is_in_use + disconnect = False + try: + # If the connection was already in use we need to use a different + # instance to avoid reentrancy deadlocks. This can happen even in + # single threaded code via __del__ implementations that can be + # called at any point. + if reentered: + # Shallow copy so we can use another connection instance + _self = copy.copy(self) + _self.conn = _self.get_connection() + assert self.conn is not _self.conn + disconnect = True + else: + _self = self + return f(_self, *args, **kwargs) + finally: + if disconnect: + _self.disconnect() + + return wrapper + + class Target(object): path = None @@ -294,6 +336,14 @@ class Target(object): if connect: self.connect() + def __copy__(self): + new = self.__class__.__new__(self.__class__) + new.__dict__ = self.__dict__.copy() + # Avoid sharing the connection instance with the original target, so + # that each target can live its own independent life + del new.__dict__['_conn'] + return new + # connection and initialization def connect(self, timeout=None, check_boot_completed=True): @@ -433,6 +483,7 @@ class Target(object): dst_mkdir(dest) + @call_conn def push(self, source, dest, as_root=False, timeout=None, globbing=False): # pylint: disable=arguments-differ sources = glob.glob(source) if globbing else [source] self._prepare_xfer('push', sources, dest) @@ -488,6 +539,7 @@ class Target(object): return paths + @call_conn def pull(self, source, dest, as_root=False, timeout=None, globbing=False): # pylint: disable=arguments-differ if globbing: sources = self._expand_glob(source, as_root=as_root) @@ -557,6 +609,7 @@ class Target(object): return command + @call_conn def execute(self, command, timeout=None, check_exit_code=True, as_root=False, strip_colors=True, will_succeed=False, force_locale='C'): @@ -566,6 +619,7 @@ class Target(object): check_exit_code=check_exit_code, as_root=as_root, strip_colors=strip_colors, will_succeed=will_succeed) + @call_conn def background(self, command, stdout=subprocess.PIPE, stderr=subprocess.PIPE, as_root=False, force_locale='C', timeout=None): command = self._prepare_cmd(command, force_locale) @@ -691,6 +745,7 @@ class Target(object): pass self.conn.connected_as_root = None + @call_conn def check_responsive(self, explode=True): try: self.conn.execute('ls /', timeout=5) @@ -1005,6 +1060,7 @@ class Target(object): os.remove(shutils_ofile) os.rmdir(tmp_dir) + @call_conn def _execute_util(self, command, timeout=None, check_exit_code=True, as_root=False): command = '{} {}'.format(self.shutils, command) return self.conn.execute(command, timeout, check_exit_code, as_root) @@ -1169,6 +1225,7 @@ class LinuxTarget(Target): def wait_boot_complete(self, timeout=10): pass + @call_conn def kick_off(self, command, as_root=False): command = 'sh -c {} 1>/dev/null 2>/dev/null &'.format(quote(command)) return self.conn.execute(command, as_root=as_root) @@ -1711,9 +1768,11 @@ class AndroidTarget(Target): def get_logcat_monitor(self, regexps=None): return LogcatMonitor(self, regexps) + @call_conn def wait_for_device(self, timeout=30): self.conn.wait_for_device() + @call_conn def reboot_bootloader(self, timeout=30): self.conn.reboot_bootloader() diff --git a/devlib/utils/misc.py b/devlib/utils/misc.py index d2aabfa..e9eec7e 100644 --- a/devlib/utils/misc.py +++ b/devlib/utils/misc.py @@ -37,6 +37,7 @@ import string import subprocess import sys import threading +import types import wrapt import warnings @@ -883,10 +884,14 @@ class _BoundTLSProperty: class InitCheckpointMeta(type): """ - Metaclass providing an ``initialized`` boolean attributes on instances. + Metaclass providing an ``initialized`` and ``is_in_use`` boolean attributes + on instances. ``initialized`` is set to ``True`` once the ``__init__`` constructor has returned. It will deal cleanly with nested calls to ``super().__init__``. + + ``is_in_use`` is set to ``True`` when an instance method is being called. + This allows to detect reentrance. """ def __new__(metacls, name, bases, dct, **kwargs): cls = super().__new__(metacls, name, bases, dct, **kwargs) @@ -895,6 +900,7 @@ class InitCheckpointMeta(type): @wraps(init_f) def init_wrapper(self, *args, **kwargs): self.initialized = False + self.is_in_use = False # Track the nesting of super()__init__ to set initialized=True only # when the outer level is finished @@ -918,6 +924,45 @@ class InitCheckpointMeta(type): cls.__init__ = init_wrapper + # Set the is_in_use attribute to allow external code to detect if the + # methods are about to be re-entered. + def make_wrapper(f): + if f is None: + return None + + @wraps(f) + def wrapper(self, *args, **kwargs): + f_ = f.__get__(self, self.__class__) + initial_state = self.is_in_use + try: + self.is_in_use = True + return f_(*args, **kwargs) + finally: + self.is_in_use = initial_state + + return wrapper + + # This will not decorate methods defined in base classes, but we cannot + # use inspect.getmembers() as it uses __get__ to bind the attributes to + # the class, making staticmethod indistinguishible from instance + # methods. + for name, attr in cls.__dict__.items(): + # Only wrap the methods (exposed as functions), not things like + # classmethod or staticmethod + if ( + name not in ('__init__', '__new__') and + isinstance(attr, types.FunctionType) + ): + setattr(cls, name, make_wrapper(attr)) + elif isinstance(attr, property): + prop = property( + fget=make_wrapper(attr.fget), + fset=make_wrapper(attr.fset), + fdel=make_wrapper(attr.fdel), + doc=attr.__doc__, + ) + setattr(cls, name, prop) + return cls