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