Re: [PATCH 1/6] test-runner: implement non_block_wait

Re: [PATCH 1/6] test-runner: implement non_block_wait

[Denis Kenzior]

[-- Attachment #1: Type: text/plain, Size: 657 bytes --]

Hi James,

On 8/13/21 11:24 AM, James Prestwood wrote:
> There was a common bit of code all over test-runner and utilities
> which would wait for 'something' in a loop. At best these loops
> would do the right thing and use the GLib.iteration call as to not
> block the main loop, and at worst would not use it and just busy
> wait.
> 
> Namespace.non_block_wait unifies all these into a single API to
> a) do the wait correctly and b) prevent duplicate code.
> ---
>   tools/test-runner | 80 ++++++++++++++++++++++++++++++++++++++---------
>   1 file changed, 65 insertions(+), 15 deletions(-)
> 

All applied, thanks.

Regards,
-Denis

[PATCH 1/6] test-runner: implement non_block_wait

[James Prestwood]

[-- Attachment #1: Type: text/plain, Size: 4263 bytes --]

There was a common bit of code all over test-runner and utilities
which would wait for 'something' in a loop. At best these loops
would do the right thing and use the GLib.iteration call as to not
block the main loop, and at worst would not use it and just busy
wait.

Namespace.non_block_wait unifies all these into a single API to
a) do the wait correctly and b) prevent duplicate code.
---
 tools/test-runner | 80 ++++++++++++++++++++++++++++++++++++++---------
 1 file changed, 65 insertions(+), 15 deletions(-)

diff --git a/tools/test-runner b/tools/test-runner
index 87f6ec6d..22e9066f 100755
--- a/tools/test-runner
+++ b/tools/test-runner
@@ -233,7 +233,7 @@ class Process:
 		if not wait and not check:
 			return
 
-		self.pid.wait(timeout=5)
+		Namespace.non_block_wait(self.wait_for_process, 10, 1)
 		self.killed = True
 		self.ret = self.pid.returncode
 
@@ -245,6 +245,13 @@ class Process:
 		if check and self.ret != 0:
 			raise subprocess.CalledProcessError(returncode=self.ret, cmd=self.args)
 
+	def wait_for_process(self, timeout):
+		try:
+			self.pid.wait(timeout)
+			return True
+		except:
+			return False
+
 	def process_io(self, source):
 		data = source.read()
 
@@ -312,12 +319,7 @@ class Process:
 		self.killed = True
 
 	def wait_for_socket(self, socket, wait):
-		waited = 0
-		while not os.path.exists(socket):
-			sleep(0.5)
-			waited += 0.5
-			if waited > wait:
-				raise Exception("Timed out waiting for socket")
+		Namespace.non_block_wait(os.path.exists, wait, socket)
 
 	def __str__(self):
 		return str(self.args) + '\n'
@@ -641,7 +643,7 @@ class Namespace:
 		p = self.start_process(['dbus-daemon', '--config-file=%s' % self.dbus_cfg],
 					wait=False, cleanup=self._cleanup_dbus)
 
-		p.wait_for_socket(self.dbus_address.split('=')[1], wait=5)
+		p.wait_for_socket(self.dbus_address.split('=')[1], 5)
 
 		self._bus = dbus.bus.BusConnection(address_or_type=self.dbus_address)
 
@@ -699,15 +701,62 @@ class Namespace:
 
 		return False
 
-	def wait_for_dbus_service(self, service):
-		tries = 0
+	@staticmethod
+	def non_block_wait(func, timeout, *args, exception=True):
+		'''
+			Convenience function for waiting in a non blocking
+			manor using GLibs context iteration i.e. does not block
+			the main loop while waiting.
+
+			'func' will be called at least once and repeatedly until
+			either it returns success, throws an exception, or the
+			'timeout' expires.
+
+			'timeout' is the ultimate timeout in seconds
+
+			'*args' will be passed to 'func'
+
+			If 'exception' is an Exception type it will be raised.
+			If 'exception' is True a generic TimeoutError will be raised.
+			Any other value will not result in an exception.
+		'''
+		# Simple class for signaling the wait timeout
+		class Bool:
+			def __init__(self, value):
+				self.value = value
+
+		def wait_timeout_cb(done):
+			done.value = True
+			return False
+
+		mainloop = GLib.MainLoop()
+		done = Bool(False)
+
+		timeout = GLib.timeout_add_seconds(timeout, wait_timeout_cb, done)
+		context = mainloop.get_context()
+
+		while True:
+			context.iteration(may_block=False)
+
+			try:
+				ret = func(*args)
+				if ret:
+					GLib.source_remove(timeout)
+					return ret
+			except Exception as e:
+				GLib.source_remove(timeout)
+				raise e
 
-		while not self._bus.name_has_owner(service):
-			if tries > 200:
-				raise TimeoutError('DBus service %s did not appear', service)
-			tries += 1
 			sleep(0.1)
 
+			if done.value == True:
+				if isinstance(exception, Exception):
+					raise exception
+				elif type(exception) == bool and exception:
+					raise TimeoutError("Timeout on non_block_wait")
+				else:
+					return
+
 	def __str__(self):
 		ret = 'Namespace: %s\n' % self.name
 		ret += 'Processes:\n'
@@ -765,7 +814,8 @@ class TestContext(Namespace):
 			args.extend(['--no-register'])
 
 		self.start_process(args)
-		self.wait_for_dbus_service('net.connman.hwsim')
+		self.non_block_wait(self._bus.name_has_owner, 20, 'net.connman.hwsim',
+					exception=TimeoutError('net.connman.hwsim did not appear'))
 
 		for i in range(nradios):
 			name = 'rad%u' % i
-- 
2.31.1

[PATCH 2/6] test-runner: wait for individual hostapd control sockets

[James Prestwood]

[-- Attachment #1: Type: text/plain, Size: 558 bytes --]

---
 tools/test-runner | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/tools/test-runner b/tools/test-runner
index 22e9066f..90a5f87a 100755
--- a/tools/test-runner
+++ b/tools/test-runner
@@ -497,6 +497,9 @@ class Hostapd:
 
 		self.process.wait_for_socket(self.global_ctrl_iface, 30)
 
+		for hapd in self.instances:
+			self.process.wait_for_socket(hapd.intf.ctrl_interface, 30)
+
 	def attach_cli(self):
 		for hapd in self.instances:
 			hapd.cli = importlib.import_module('hostapd').HostapdCLI(config=hapd.config)
-- 
2.31.1

[PATCH 3/6] auto-t: ofono.py: use non_block_wait

[James Prestwood]

[-- Attachment #1: Type: text/plain, Size: 2126 bytes --]

---
 autotests/util/ofono.py | 27 ++++-----------------------
 1 file changed, 4 insertions(+), 23 deletions(-)

diff --git a/autotests/util/ofono.py b/autotests/util/ofono.py
index 584a656a..ea3aad60 100644
--- a/autotests/util/ofono.py
+++ b/autotests/util/ofono.py
@@ -9,13 +9,8 @@ class Ofono(dbus.service.Object):
     def __init__(self, namespace=ctx):
         self._bus = namespace.get_bus()
 
-        tries = 0
-
-        while not self._bus.name_has_owner('org.ofono'):
-            if tries > 100:
-                raise TimeoutError('Waiting for org.ofono service timed out')
-            tries += 1
-            time.sleep(0.1)
+        ctx.non_block_wait(self._bus.name_has_owner, 10, 'org.ofono',
+                            exception=TimeoutError('Waiting for org.ofono service timed out'))
 
     def enable_modem(self, path):
         self._modem_path = path
@@ -31,7 +26,6 @@ class Ofono(dbus.service.Object):
                 self._sim_auth_up = True
 
     def wait_for_sim_auth(self, max_wait = 15):
-        mainloop = GLib.MainLoop()
         self._sim_auth_up = False
 
         props = self._modem_iface.GetProperties()
@@ -42,18 +36,5 @@ class Ofono(dbus.service.Object):
         self._modem_iface.connect_to_signal('PropertyChanged',
                                              self._modem_prop_changed)
 
-        self._wait_timed_out = False
-        def wait_timeout_cb():
-            self._wait_timed_out = True
-            return False
-
-        try:
-            timeout = GLib.timeout_add_seconds(max_wait, wait_timeout_cb)
-            context = mainloop.get_context()
-            while (not self._sim_auth_up):
-                context.iteration(may_block=True)
-                if self._wait_timed_out:
-                    raise TimeoutError('waiting for SimAuthentication timed out')
-
-        finally:
-            GLib.source_remove(timeout)
+        ctx.non_block_wait(lambda s : s._sim_auth_up, max_wait, self,
+                            exception=TimeoutError('waiting for SimAuthetication timed out'))
-- 
2.31.1

[PATCH 4/6] auto-t: iwd.py: use non_block_wait

[James Prestwood]

[-- Attachment #1: Type: text/plain, Size: 7247 bytes --]

---
 autotests/util/iwd.py | 111 ++++++++++--------------------------------
 1 file changed, 25 insertions(+), 86 deletions(-)

diff --git a/autotests/util/iwd.py b/autotests/util/iwd.py
index 18840cd6..11c37039 100755
--- a/autotests/util/iwd.py
+++ b/autotests/util/iwd.py
@@ -109,9 +109,7 @@ class AsyncOpAbstract(object):
         self._exception = _convert_dbus_ex(ex)
 
     def _wait_for_async_op(self):
-        context = ctx.mainloop.get_context()
-        while not self._is_completed:
-            context.iteration(may_block=True)
+        ctx.non_block_wait(lambda s: s._is_completed, 30, self, exception=None)
 
         self._is_completed = False
         if self._exception is not None:
@@ -1030,16 +1028,8 @@ class IWD(AsyncOpAbstract):
             self._iwd_proc = self.namespace.start_iwd(iwd_config_dir,
                                                         iwd_storage_dir)
 
-        tries = 0
-        while not self._bus.name_has_owner(IWD_SERVICE):
-            if not ctx.args.gdb:
-                if tries > 200:
-                    if start_iwd_daemon:
-                        self.namespace.stop_process(self._iwd_proc)
-                        self._iwd_proc = None
-                    raise TimeoutError('IWD has failed to start')
-                tries += 1
-            time.sleep(0.1)
+        ctx.non_block_wait(self._bus.name_has_owner, 20, IWD_SERVICE,
+                                exception=TimeoutError('IWD has failed to start'))
 
         self._devices = DeviceList(self)
 
@@ -1085,27 +1075,13 @@ class IWD(AsyncOpAbstract):
 
     @staticmethod
     def _wait_for_object_condition(obj, condition_str, max_wait = 50):
-        class TimeoutData:
-            _wait_timed_out = False
+        def _eval_wrap(obj, condition_str):
+            return eval(condition_str)
 
-        data = TimeoutData()
-
-        def wait_timeout_cb(data):
-            data._wait_timed_out = True
-            return False
-
-        try:
-            timeout = GLib.timeout_add_seconds(max_wait, wait_timeout_cb, data)
-            context = ctx.mainloop.get_context()
-            while not eval(condition_str):
-                context.iteration(may_block=True)
-                if data._wait_timed_out and ctx.args.gdb == None:
-                    raise TimeoutError('[' + condition_str + ']'\
-                                       ' condition was not met in '\
-                                       + str(max_wait) + ' sec')
-        finally:
-            if not data._wait_timed_out:
-                GLib.source_remove(timeout)
+        ctx.non_block_wait(_eval_wrap, max_wait, obj, condition_str,
+                            exception=TimeoutError('[' + condition_str + ']'\
+                                                   ' condition was not met in '\
+                                                   + str(max_wait) + ' sec'))
 
     def wait_for_object_condition(self, *args, **kwargs):
         self._wait_for_object_condition(*args, **kwargs)
@@ -1119,54 +1095,30 @@ class IWD(AsyncOpAbstract):
             This allows an object to be checked for a state transition without any
             intermediate state changes.
         '''
-        self._wait_timed_out = False
+        def _eval_from_to(obj, from_str, to_str):
+            # If neither the initial or expected condition evaluate the
+            # object must be in another unexpected state.
+            if not eval(from_str) and not eval(to_str):
+                raise Exception('unexpected condition between [%s] and [%s]' %
+                                        (from_str, to_str))
+
+            # Initial condition does not evaluate but expected does, pass
+            if not eval(from_str) and eval(to_str):
+                return True
 
-        def wait_timeout_cb():
-            self._wait_timed_out = True
             return False
 
         # Does initial condition pass?
         if not eval(from_str):
             raise Exception("initial condition [%s] not met" % from_str)
 
-        try:
-            timeout = GLib.timeout_add_seconds(max_wait, wait_timeout_cb)
-            context = ctx.mainloop.get_context()
-            while True:
-                context.iteration(may_block=True)
-
-                # If neither the initial or expected condition evaluate the
-                # object must be in another unexpected state.
-                if not eval(from_str) and not eval(to_str):
-                    raise Exception('unexpected condition between [%s] and [%s]' %
-                                        (from_str, to_str))
-
-                # Initial condition does not evaluate but expected does, pass
-                if not eval(from_str) and eval(to_str):
-                    break
-
-                if self._wait_timed_out and ctx.args.gdb == None:
-                    raise TimeoutError('[' + to_str + ']'\
+        ctx.non_block_wait(_eval_from_to, max_wait, obj, from_str, to_str,
+                            exception=TimeoutError('[' + to_str + ']'\
                                        ' condition was not met in '\
-                                       + str(max_wait) + ' sec')
-        finally:
-            if not self._wait_timed_out:
-                GLib.source_remove(timeout)
+                                       + str(max_wait) + ' sec'))
 
     def wait(self, time):
-        self._wait_timed_out = False
-        def wait_timeout_cb():
-            self._wait_timed_out = True
-            return False
-
-        try:
-            timeout = GLib.timeout_add(int(time * 1000), wait_timeout_cb)
-            context = ctx.mainloop.get_context()
-            while not self._wait_timed_out:
-                context.iteration(may_block=True)
-        finally:
-            if not self._wait_timed_out:
-                GLib.source_remove(timeout)
+        ctx.non_block_wait(lambda : False, time, exception=False)
 
     @staticmethod
     def clear_storage(storage_dir=IWD_STORAGE_DIR):
@@ -1211,21 +1163,8 @@ class IWD(AsyncOpAbstract):
         if not wait_to_appear:
             return list(self._devices.values() if not p2p else self._devices.p2p_dict.values())
 
-        self._wait_timed_out = False
-        def wait_timeout_cb():
-            self._wait_timed_out = True
-            return False
-
-        try:
-            timeout = GLib.timeout_add_seconds(max_wait, wait_timeout_cb)
-            context = ctx.mainloop.get_context()
-            while len(self._devices) < wait_to_appear:
-                context.iteration(may_block=True)
-                if self._wait_timed_out:
-                    raise TimeoutError('IWD has no associated devices')
-        finally:
-            if not self._wait_timed_out:
-                GLib.source_remove(timeout)
+        ctx.non_block_wait(lambda s, n: len(s._devices) >= n, max_wait, self, wait_to_appear,
+                            exception=TimeoutError('IWD has no associated devices'))
 
         return list(self._devices.values() if not p2p else self._devices.p2p_dict.values())[:wait_to_appear]
 
-- 
2.31.1

[PATCH 5/6] auto-t: hostapd.py: use non_block_wait

[James Prestwood]

[-- Attachment #1: Type: text/plain, Size: 2311 bytes --]

---
 autotests/util/hostapd.py | 35 ++++++++++++-----------------------
 1 file changed, 12 insertions(+), 23 deletions(-)

diff --git a/autotests/util/hostapd.py b/autotests/util/hostapd.py
index be6e1143..a61b2d39 100644
--- a/autotests/util/hostapd.py
+++ b/autotests/util/hostapd.py
@@ -86,30 +86,19 @@ class HostapdCLI:
     def __init__(self, config=None):
         self._init_hostapd(config)
 
-    def wait_for_event(self, event, timeout=10):
-        global mainloop
-        self._wait_timed_out = False
-
-        def wait_timeout_cb():
-            self._wait_timed_out = True
+    def _poll_event(self, event):
+        if not self._data_available(0.25):
             return False
 
-        timeout = GLib.timeout_add_seconds(timeout, wait_timeout_cb)
-        context = mainloop.get_context()
-
-        while True:
-            context.iteration(may_block=False)
+        data = self.ctrl_sock.recv(4096).decode('utf-8')
+        if event in data:
+            return data
 
-            while self._data_available(0.25):
-                data = self.ctrl_sock.recv(4096).decode('utf-8')
-                if event in data:
-                    GLib.source_remove(timeout)
-                    return data
-
-            if self._wait_timed_out:
-                raise TimeoutError('waiting for hostapd event timed out')
+        return False
 
-        return None
+    def wait_for_event(self, event, timeout=10):
+        return ctx.non_block_wait(self._poll_event, timeout, event,
+                                    exception=TimeoutError("waiting for event"))
 
     def _data_available(self, timeout=2):
         [r, w, e] = select.select([self.ctrl_sock], [], [], timeout)
@@ -123,10 +112,10 @@ class HostapdCLI:
 
         self.ctrl_sock.send(bytes(command))
 
-        if self._data_available(timeout):
-            return self.ctrl_sock.recv(4096).decode('utf-8')
+        ctx.non_block_wait(self._data_available, timeout,
+                            exception=TimeoutError("waiting for control response"))
 
-        raise Exception('timeout waiting for control response')
+        return self.ctrl_sock.recv(4096).decode('utf-8')
 
     def _del_hostapd(self, force=False):
         if not self.ctrl_sock:
-- 
2.31.1

[PATCH 6/6] auto-t: ead.py: use non_block_wait

[James Prestwood]

[-- Attachment #1: Type: text/plain, Size: 1816 bytes --]

---
 autotests/util/ead.py | 25 ++++---------------------
 1 file changed, 4 insertions(+), 21 deletions(-)

diff --git a/autotests/util/ead.py b/autotests/util/ead.py
index 3a613bbe..8f333a8a 100644
--- a/autotests/util/ead.py
+++ b/autotests/util/ead.py
@@ -81,12 +81,8 @@ class EAD(iwd.AsyncOpAbstract):
     _adapters = None
 
     def __init__(self):
-        tries = 0
-        while not self._bus.name_has_owner(EAD_SERVICE):
-            if tries > 200:
-                raise TimeoutError('IWD has failed to start')
-            tries += 1
-            time.sleep(0.1)
+        ctx.non_block_wait(self._bus.name_has_owner, 20, EAD_SERVICE,
+                            exception=TimeoutError('EAD has failed to start'))
 
         self._adapters = AdapterList(self)
 
@@ -103,21 +99,8 @@ class EAD(iwd.AsyncOpAbstract):
         if not wait_to_appear:
             return list(self._adapters.values())
 
-        self._wait_timed_out = False
-        def wait_timeout_cb():
-            self._wait_timed_out = True
-            return False
-
-        try:
-            timeout = GLib.timeout_add_seconds(max_wait, wait_timeout_cb)
-            context = ctx.mainloop.get_context()
-            while len(self._adapters) < wait_to_appear:
-                context.iteration(may_block=True)
-                if self._wait_timed_out:
-                    raise TimeoutError('IWD has no associated devices')
-        finally:
-            if not self._wait_timed_out:
-                GLib.source_remove(timeout)
+        ctx.non_block_wait(lambda s, num : len(s._adapters) >= num, max_wait, self, wait_to_appear,
+                            exception=TimeoutError('EAD has no associated devices'))
 
         return list(self._adapters.values())
 
-- 
2.31.1