Re: [PATCH v3 09/13] python/aqmp-tui: Add QMP connection manager

[John Snow]

On Tue, Aug 17, 2021 at 3:07 PM Niteesh G. S. <niteesh.gs@gmail.com> wrote:

On Tue, Aug 17, 2021 at 10:21 AM John Snow <jsnow@redhat.com> wrote:

On Fri, Jul 30, 2021 at 4:19 PM G S Niteesh Babu <niteesh.gs@gmail.com> wrote:

[...]

 

 

+
+
 class App(QMPClient):
-    def __init__(self, address: Union[str, Tuple[str, int]]) -> None:
+    def __init__(self, address: Union[str, Tuple[str, int]], num_retries: int,
+                 retry_delay: Optional[int]) -> None:
         urwid.register_signal(type(self), UPDATE_MSG)
         self.window = Window(self)
         self.address = address
         self.aloop: Optional[Any] = None  # FIXME: Use more concrete type.
+        self.num_retries = num_retries
+        self.retry_delay = retry_delay
+        self.retry: bool = False
+        self.disconnecting: bool = False

Why is this one needed again ? ...

 

A race condition occurs in protocol.py line 597

The reason behind this is there are two disconnect calls initiated. The first one via kill_app

and the second one via manage_connection when the state is set to disconnecting by the first call.

One of the calls set's the state to IDLE(protocol.py:584) after it has finished disconnecting, meanwhile

the second call is somehow in the process of disconnecting and assert the state to be in DISCONNECTING

in protocol.py:597, which it is not since it has been set to IDLE by the first call.

If I don't gaurd against the second call I get the following exception

------------------------------------------------------------------------------------------

Traceback (most recent call last):
  File "/home/niteesh/development/qemu/python/.venv/bin/aqmp-tui", line 33, in <module>
    sys.exit(load_entry_point('qemu', 'console_scripts', 'aqmp-tui')())
  File "/home/niteesh/development/qemu/python/qemu/aqmp/aqmp_tui.py", line 695, in main
    app.run(args.asyncio_debug)
  File "/home/niteesh/development/qemu/python/qemu/aqmp/aqmp_tui.py", line 444, in run
    raise err
  File "/home/niteesh/development/qemu/python/qemu/aqmp/aqmp_tui.py", line 441, 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 391, in manage_connection
    await self.disconnect()
  File "/home/niteesh/development/qemu/python/qemu/aqmp/aqmp_tui.py", line 312, in disconnect
    raise err
  File "/home/niteesh/development/qemu/python/qemu/aqmp/aqmp_tui.py", line 300, in disconnect
    await super().disconnect()
  File "/home/niteesh/development/qemu/python/qemu/aqmp/protocol.py", line 302, in disconnect
    await self._wait_disconnect()
  File "/home/niteesh/development/qemu/python/qemu/aqmp/protocol.py", line 583, in _wait_disconnect
    self._cleanup()
  File "/home/niteesh/development/qemu/python/qemu/aqmp/qmp_client.py", line 331, in _cleanup
    super()._cleanup()
  File "/home/niteesh/development/qemu/python/qemu/aqmp/protocol.py", line 597, in _cleanup
    assert self.runstate == Runstate.DISCONNECTING
AssertionError

-------------------------------------------------------------------------------------------

Hm, OK. I'm not sure if this is a bug on my part or not yet, I'll investigate.

 

     def add_to_history(self, msg: str, level: Optional[str] = None) -> None:
@@ -119,7 +132,7 @@ def _cb_inbound(self, msg: Message) -> Message:
             LOGGER.info('Error server disconnected before reply')
             urwid.emit_signal(self, UPDATE_MSG,
                               '{"error": "Server disconnected before reply"}')
-            self._set_status("Server disconnected")
+            await self.disconnect()
         except Exception as err:
             LOGGER.error('Exception from _send_to_server: %s', str(err))
             raise err
@@ -136,15 +149,29 @@ def kill_app(self) -> None:
         create_task(self._kill_app())

Is this required? I would have hoped that after calling disconnect that the state would have again changed to IDLE and you wouldn't need this clause here.

After you mentioned it I too felt it was redundant. But on removing it the whole app freezes when trying to exit.

I logged the state after the call to disconnect, instead of being in the IDLE state, it is still in DISCONNECTING state.

I suspect this results in the constant infinite looping which doesn't give other coroutines a chance to run and blocks

the event loop thus resulting in the freezing of the app. But I am not sure why the state isn't changing to IDLE.

Hmm ... That may well be a bug in AQMP then. I will investigate.