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