On Thu, Dec 16, 2021 at 4:51 AM Vladimir Sementsov-Ogievskiy <
vsementsov@virtuozzo.com> wrote:

> 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 ?
>
>
Did any catch your eye? Depending on where it fails, it may not need any
additional cleanup. In this case, it's important to delete the _pending
entry so that we don't leave stale entries behind.


> >
> > 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.
>
>
pylint won't complain as long as you also ubiquitously re-raise the
exception. It's only a bad practice to suppress all exceptions, but it's OK
to define cleanup actions.

> +            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.
>

They could. I split them for the sake of sub-classing if you wanted to
perform additional actions on the outgoing/incoming arms of the execute()
action. Specifically, I am accommodating the case that someone wants to
subclass QMPClient and create methods where a QMP command is *sent* but is
not *awaited*, i.e. _issue() is called without an immediate _reply(). This
allows us the chance to create something like a PendingExecution object
that could be awaited later on.

The simpler case, execute(), doesn't bother with separating those actions
and just awaits the reply immediately.


>
>
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?
>
>