Re: [PATCH v2 02/25] python/aqmp: handle asyncio.TimeoutError on execute()

[Vladimir Sementsov-Ogievskiy]

15.12.2021 22:39, John Snow wrote:
> This exception can be injected into any await statement. If we are
> canceled via timeout, we want to clear the pending execution record on
> our way out.

Hmm, but there are more await statements in the file, shouldn't we care about 
them too ?

> Signed-off-by: John Snow <jsnow@redhat.com>
> ---
>   python/qemu/aqmp/qmp_client.py | 8 ++++++--
>   1 file changed, 6 insertions(+), 2 deletions(-)
>
> diff --git a/python/qemu/aqmp/qmp_client.py b/python/qemu/aqmp/qmp_client.py
> index 8105e29fa8..6a985ffe30 100644
> --- a/python/qemu/aqmp/qmp_client.py
> +++ b/python/qemu/aqmp/qmp_client.py
> @@ -435,7 +435,11 @@ async def _issue(self, msg: Message) -> Union[None, str]:
>               msg_id = msg['id']
>   
>           self._pending[msg_id] = asyncio.Queue(maxsize=1)
> -        await self._outgoing.put(msg)
> +        try:
> +            await self._outgoing.put(msg)
> +        except:

Doesn't pylint and others complain about plain "except". Do we really need to 
catch any exception here? As far as I know that's not a good practice.

> +            del self._pending[msg_id]
> +            raise
>   
>           return msg_id
>   
> @@ -452,9 +456,9 @@ async def _reply(self, msg_id: Union[str, None]) -> Message:
>               was lost, or some other problem.
>           """
>           queue = self._pending[msg_id]
> -        result = await queue.get()
>   
>           try:
> +            result = await queue.get()
>               if isinstance(result, ExecInterruptedError):
>                   raise result
>               return result

This one looks good, just include it into existing try-finally

Hmm. _issue() and _reply() are used only in one place, as a pair. It looks like both 
"awaits" should be better under one try-finally block.

For example, move "self._pending[msg_id] = asyncio.Queue(maxsize=1)" to 
_execute, and just do try-finally in _execute() around _issue and _reply. Or may be just 
merge the whole logic in _execute, it doesn't seem too much. What do you think?


--
Best regards,
Vladimir