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

[Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>]

16.12.2021 20:22, John Snow wrote:
> 
> 
> On Thu, Dec 16, 2021 at 4:51 AM Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com <mailto: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.

No. I simply searched for "await" reading the first sentence of commit message. Now I better follow what you are doing.

> 
>      >
>      > Signed-off-by: John Snow <jsnow@redhat.com <mailto: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?
> 


OK, that's all reasonable, thanks:
Reviewed-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>

-- 
Best regards,
Vladimir