Re: [PATCH v2 01/25] python/aqmp: add __del__ method to legacy interface

[John Snow <jsnow@redhat.com>]

[-- Attachment #1: Type: text/plain, Size: 3334 bytes --]

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

> 15.12.2021 22:39, John Snow wrote:
> > asyncio can complain *very* loudly if you forget to back out of things
> > gracefully before the garbage collector starts destroying objects that
> > contain live references to asyncio Tasks.
> >
> > The usual fix is just to remember to call aqmp.disconnect(), but for the
> > sake of the legacy wrapper and quick, one-off scripts where a graceful
> > shutdown is not necessarily of paramount imporance, add a courtesy
> > cleanup that will trigger prior to seeing screenfuls of confusing
> > asyncio tracebacks.
> >
> > Note that we can't *always* save you from yourself; depending on when
> > the GC runs, you might just seriously be out of luck. The best we can do
> > in this case is to gently remind you to clean up after yourself.
> >
> > (Still much better than multiple pages of incomprehensible python
> > warnings for the crime of forgetting to put your toys away.)
> >
> > Signed-off-by: John Snow <jsnow@redhat.com>
> > ---
> >   python/qemu/aqmp/legacy.py | 18 ++++++++++++++++++
> >   1 file changed, 18 insertions(+)
> >
> > diff --git a/python/qemu/aqmp/legacy.py b/python/qemu/aqmp/legacy.py
> > index 9e7b9fb80b..2ccb136b02 100644
> > --- a/python/qemu/aqmp/legacy.py
> > +++ b/python/qemu/aqmp/legacy.py
> > @@ -16,6 +16,8 @@
> >   import qemu.qmp
> >   from qemu.qmp import QMPMessage, QMPReturnValue, SocketAddrT
> >
> > +from .error import AQMPError
> > +from .protocol import Runstate
> >   from .qmp_client import QMPClient
> >
> >
> > @@ -136,3 +138,19 @@ def settimeout(self, timeout: Optional[float]) ->
> None:
> >
> >       def send_fd_scm(self, fd: int) -> None:
> >           self._aqmp.send_fd_scm(fd)
> > +
> > +    def __del__(self) -> None:
> > +        if self._aqmp.runstate == Runstate.IDLE:
> > +            return
> > +
> > +        if not self._aloop.is_running():
> > +            self.close()
>
> As I understand, if close() was called by hand, runstate should already be
> IDLE, and we don't call close() twice here?
>
>
Right ... calling close() twice is actually OK (the second one just does
nothing) but what I am avoiding here is this scenario:

- close() was already called
- garbage collection runs while the event loop is running
- we should therefore NOT raise an exception.

The problem is that even if a second close() does nothing, we are not able
to call into it if we're already inside of an active event loop -- so in
order to achieve the no-op behavior from the GC context, I need to manually
check the runstate to determine if there /WILL/ be a problem when the GC
runs. If it's idle, there definitely won't be.


> > +        else:
> > +            # Garbage collection ran while the event loop was running.
> > +            # Nothing we can do about it now, but if we don't raise our
> > +            # own error, the user will be treated to a lot of traceback
> > +            # they might not understand.
> > +            raise AQMPError(
> > +                "QEMUMonitorProtocol.close()"
> > +                " was not called before object was garbage collected"
> > +            )
> >
>
> weak, as I'm far from understanding aqmp library:
>
> Reviewed-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
>
>
thanks!
--js

[-- Attachment #2: Type: text/html, Size: 4681 bytes --]