On Thu, Dec 16, 2021 at 4:51 AM Vladimir Sementsov-Ogievskiy < 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. > > > > Signed-off-by: John Snow <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? > >
