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

[John Snow <jsnow@redhat.com>]

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

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 :)

[-- Attachment #2: Type: text/html, Size: 3411 bytes --]