Re: [PATCH v2 00/24] python: introduce Asynchronous QMP package

Looping qemu-devel back in: I removed them by accident by not hitting reply-all :(

On Wed, Jul 21, 2021 at 2:06 PM Niteesh G. S. <niteesh.gs@gmail.com> wrote:

On Wed, Jul 21, 2021 at 11:03 PM John Snow <jsnow@redhat.com> wrote:

On Wed, Jul 21, 2021 at 1:04 PM Niteesh G. S. <niteesh.gs@gmail.com> wrote:
Hello all,

I recently rebased(incrementally) my TUI on this V2 patch and faced an issue.
https://gitlab.com/niteesh.gs/qemu/-/commits/aqmp-tui-prototype-v3
I decided to rebase incrementally so that I can address some of the comments posted
in my patch series. While testing out, the initial draft of TUI which worked fine in the V1
version of AQMP failed in this version.

Disconnecting from a fully connected state doesn't exit cleanly.
---------------------------------------------------------------------------------
To reproduce the issue:
1) Initiate a QMP server

Please provide the command line.
qemu-system-x86_64 -qmp tcp:localhost:1234,server,wait=on

2) Connect the TUI to the server using aqmp-tui localhost:1234 --log-file log.txt

The entry point isn't defined yet in your series, so I will assume "python3 -m qemu.aqmp.aqmp_tui localhost:1234" should work here.
Yup, sorry about that. I realized this later when recreated the venv.

3) Once the TUI is connected and running, press 'Esc' to exit the app. This should result
in the following exception.
--------------------------------------------------------------------------------------------------------------------------------------------
Transitioning from 'Runstate.IDLE' to 'Runstate.CONNECTING'.
Connecting to ('localhost', 1234) ...
Connected.
Awaiting greeting ...
Response: {
"QMP": {
.......... Skipping
}
}
Negotiating capabilities ...
Request: {
"execute": "qmp_capabilities",
.......... Skipping
}
}
Response: {
"return": {}
}
Transitioning from 'Runstate.CONNECTING' to 'Runstate.RUNNING'.
Transitioning from 'Runstate.RUNNING' to 'Runstate.DISCONNECTING'.
Scheduling disconnect.
Draining the outbound queue ...
Flushing the StreamWriter ...
Cancelling writer task ...
Task.Writer: cancelled.
Task.Writer: exiting.
Cancelling reader task ...
Task.Reader: cancelled.
Task.Reader: exiting.
Closing StreamWriter.
Waiting for StreamWriter to close ...
QMP Disconnected.
Transitioning from 'Runstate.DISCONNECTING' to 'Runstate.IDLE'.
_kill_app: Connection lost
Connection lost
| Traceback (most recent call last):
| File "/home/niteesh/development/qemu/python/qemu/aqmp/aqmp_tui.py", line 246, in run
| main_loop.run()
| File "/home/niteesh/development/qemu/python/.venv/lib/python3.6/site-packages/urwid/main_loop.py", line 287, in run
| self._run()
| File "/home/niteesh/development/qemu/python/.venv/lib/python3.6/site-packages/urwid/main_loop.py", line 385, in _run
| self.event_loop.run()
| File "/home/niteesh/development/qemu/python/.venv/lib/python3.6/site-packages/urwid/main_loop.py", line 1494, in run
| reraise(*exc_info)
| File "/home/niteesh/development/qemu/python/.venv/lib/python3.6/site-packages/urwid/compat.py", line 58, in reraise
| raise value
| File "/home/niteesh/development/qemu/python/qemu/aqmp/aqmp_tui.py", line 206, in _kill_app
| raise err
| File "/home/niteesh/development/qemu/python/qemu/aqmp/aqmp_tui.py", line 201, in _kill_app
| await self.disconnect()
| File "/home/niteesh/development/qemu/python/qemu/aqmp/protocol.py", line 303, in disconnect
| await self._wait_disconnect()
| File "/home/niteesh/development/qemu/python/qemu/aqmp/protocol.py", line 573, in _wait_disconnect
| await self._dc_task
| File "/home/niteesh/development/qemu/python/qemu/aqmp/qmp_client.py", line 316, in _bh_disconnect
| await super()._bh_disconnect()
| File "/home/niteesh/development/qemu/python/qemu/aqmp/protocol.py", line 644, in _bh_disconnect
| await wait_closed(self._writer)
| File "/home/niteesh/development/qemu/python/qemu/aqmp/util.py", line 137, in wait_closed
| await flush(writer)
| File "/home/niteesh/development/qemu/python/qemu/aqmp/util.py", line 49, in flush
| await writer.drain()
| File "/usr/lib/python3.6/asyncio/streams.py", line 339, in drain
| yield from self._protocol._drain_helper()
| File "/usr/lib/python3.6/asyncio/streams.py", line 210, in _drain_helper
| raise ConnectionResetError('Connection lost')
| ConnectionResetError: Connection lost
--------------------------------------------------------------------------------------------------------------------------------------------

I can't reproduce in Python 3.9, but I *can* reproduce in python 3.6 using the pipenv environment; i.e.

> make check-pipenv
> pipenv shell
> python3 -m qemu.aqmp.aqmp_tui 127.0.0.1:1234

What python version are you using to see this failure? Is it 3.6 ?
Yes, I was using python 3.6. I just tried it on 3.8 and I don't face this issue.

It seems like the wait_closed() wrapper I wrote isn't quite compatible with Python 3.6, it looks like it's not really safe to try and flush a closing socket. I was doing so in an attempt to tell when the socket had finished closing out its buffer (expecting it to normally be a no-op) but in this case even a no-op drain in 3.6 seems to raise an error if we attempt it after we've asked for the socket to close.

wait_closed() was added in Python 3.7 and we just don't have access to it here ... I'm not sure if there's something else we can do here to serve as a workaround for not having this function.

--js

I can't find a *nice* workaround, but I found one that should probably work in most universes. We can remove this ugly code when we support 3.7 as a minimum. However, please try this patch as a fixup:

diff --git a/python/qemu/aqmp/util.py b/python/qemu/aqmp/util.py
index de0df44cbd7..eaa5fc7d5f9 100644
--- a/python/qemu/aqmp/util.py
+++ b/python/qemu/aqmp/util.py
@@ -134,7 +134,17 @@ async def wait_closed(writer: asyncio.StreamWriter) -> None:

while not transport.is_closing():
await asyncio.sleep(0)
- await flush(writer)
+
+ # This is an ugly workaround, but it's the best I can come up with.
+ sock = transport.get_extra_info('socket')
+
+ if sock is None:
+ # Our transport doesn't have a socket? ...
+ # Nothing we can reasonably do.
+ return
+
+ while sock.fileno() != -1:
+ await asyncio.sleep(0)

From:	John Snow
Subject:	Re: [PATCH v2 00/24] python: introduce Asynchronous QMP package
Date:	Wed, 21 Jul 2021 15:55:03 -0400