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

[John Snow <jsnow@redhat.com>]

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

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.

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