Re: [PATCH v6 2/4] bitbake: asyncrpc: always create new asyncio loops

["Joshua Watt" <JPEWhacker@gmail.com>]

On 8/19/21 11:46 AM, Scott Murray wrote:
> asyncio in older Python 3.x (seen with 3.7) can seemingly hang if
> new_event_loop is called repeatedly in the same process.  The
> reuse of processes in the Bitbake thread pool during parsing seems
> to be able to trigger this with the PR server export selftest.
> It appears that calling set_event_loop with the new loop avoids the
> issue, so that is now done in the asyncrpc Client initializer (with
> an explanatory comment).  This should be revisited when the day
> arrives that Python 3.9 becomes the minimum required for BitBake.
>
> Additionally, it was discovered that using get_event_loop in the
> asyncrpc server initialization can trigger hangs in the hashserv
> unittests when the second test is run.  To avoid this, switch to
> calling new_event_loop + set_event_loop in the initialization code
> there as well.
>
> Signed-off-by: Scott Murray <scott.murray@konsulko.com>

Looks good to me

Reviewed-by: Joshua Watt <JPEWhacker@gmail.com>


> ---
>   lib/bb/asyncrpc/client.py | 10 ++++++++++
>   lib/bb/asyncrpc/serv.py   | 24 ++++++++++++++++++++----
>   2 files changed, 30 insertions(+), 4 deletions(-)
>
> diff --git a/lib/bb/asyncrpc/client.py b/lib/bb/asyncrpc/client.py
> index 3eb4fdde..50e60d5c 100644
> --- a/lib/bb/asyncrpc/client.py
> +++ b/lib/bb/asyncrpc/client.py
> @@ -119,6 +119,16 @@ class Client(object):
>           self.client = self._get_async_client()
>           self.loop = asyncio.new_event_loop()
>   
> +        # Override any pre-existing loop.
> +        # Without this, the PR server export selftest triggers a hang
> +        # when running with Python 3.7.  The drawback is that there is
> +        # potential for issues if the PR and hash equiv (or some new)
> +        # clients need to both be instantiated in the same process.
> +        # This should be revisited if/when Python 3.9 becomes the
> +        # minimum required version for BitBake, as it seems not
> +        # required (but harmless) with it.
> +        asyncio.set_event_loop(self.loop)
> +
>           self._add_methods('connect_tcp', 'close', 'ping')
>   
>       @abc.abstractmethod
> diff --git a/lib/bb/asyncrpc/serv.py b/lib/bb/asyncrpc/serv.py
> index 45628698..b4cffff2 100644
> --- a/lib/bb/asyncrpc/serv.py
> +++ b/lib/bb/asyncrpc/serv.py
> @@ -136,10 +136,7 @@ class AsyncServer(object):
>           self.logger = logger
>           self.start = None
>           self.address = None
> -
> -    @property
> -    def loop(self):
> -        return asyncio.get_event_loop()
> +        self.loop = None
>   
>       def start_tcp_server(self, host, port):
>           def start_tcp():
> @@ -228,6 +225,12 @@ class AsyncServer(object):
>           """
>           Serve requests in the current process
>           """
> +        # Create loop and override any loop that may have existed in
> +        # a parent process.  It is possible that the usecases of
> +        # serve_forever might be constrained enough to allow using
> +        # get_event_loop here, but better safe than sorry for now.
> +        self.loop = asyncio.new_event_loop()
> +        asyncio.set_event_loop(self.loop)
>           self.start()
>           self._serve_forever()
>   
> @@ -236,6 +239,19 @@ class AsyncServer(object):
>           Serve requests in a child process
>           """
>           def run(queue):
> +            # Create loop and override any loop that may have existed
> +            # in a parent process.  Without doing this and instead
> +            # using get_event_loop, at the very minimum the hashserv
> +            # unit tests will hang when running the second test.
> +            # This happens since get_event_loop in the spawned server
> +            # process for the second testcase ends up with the loop
> +            # from the hashserv client created in the unit test process
> +            # when running the first testcase.  The problem is somewhat
> +            # more general, though, as any potential use of asyncio in
> +            # Cooker could create a loop that needs to replaced in this
> +            # new process.
> +            self.loop = asyncio.new_event_loop()
> +            asyncio.set_event_loop(self.loop)
>               try:
>                   self.start()
>               finally: