qemu-devel
[Top][All Lists]
Advanced

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

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


From: John Snow
Subject: Re: [PATCH v2 02/25] python/aqmp: handle asyncio.TimeoutError on execute()
Date: Thu, 16 Dec 2021 12:26:18 -0500



On Thu, Dec 16, 2021 at 7:39 AM Beraldo Leal <bleal@redhat.com> wrote:
On Wed, Dec 15, 2021 at 02:39:16PM -0500, 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.
>
> 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:
> +            del self._pending[msg_id]
> +            raise

At first glance both, except and raise are not good practices, but I
think I got what you are doing here, just intercepting to delete the
pending message and raising it again. So maybe it will be safe to close
the eyes here. ;)


Yeah, IMO it's 100% legitimate to define cleanup actions using catch-all except statements as long as you re-raise. There's no other construct that handles this case, AFAIK.

try
except <- on error
else <- on NOT error
finally <- always

Sometimes you just wanna undo stuff only on error. Maybe a higher layer will "handle" that exception, maybe it'll crash the program. We don't know here, we just know we need to undo some stuff.
 

>          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

Reviewed-by: Beraldo Leal <bleal@redhat.com>


ty :)

reply via email to

[Prev in Thread] Current Thread [Next in Thread]