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

[Beraldo Leal <bleal@redhat.com>]

On Wed, Dec 15, 2021 at 02:39:15PM -0500, 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()
> +        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"
> +            )

From the Python PoV, LGTM.

Reviewed-by: Beraldo Leal <bleal@redhat.com>

--
Beraldo