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...@gmail.com> wrote: > > > On Wed, Jul 21, 2021 at 11:03 PM John Snow <js...@redhat.com> wrote: > >> >> >> On Wed, Jul 21, 2021 at 1:04 PM Niteesh G. S. <niteesh...@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)
