altendky / qtrio

QTrio - a library bringing Qt GUIs together with async and await via Trio
https://qtrio.readthedocs.io/
Other
40 stars 4 forks source link

Trio warning when exiting the application #262

Open vxgmichel opened 3 years ago

vxgmichel commented 3 years ago

Hi @altendky,

When exiting our qtrio application (which runs with qtrio.run), we sometimes get the following warning:

.../python3.8/site-packages/trio/_core/_run.py:2221: RuntimeWarning: Trio guest run got abandoned without properly finishing... weird stuff might happen

The comment here says something about garbage collection:

https://github.com/python-trio/trio/blob/3f4e1a23e567825926b225f87c64db56d95f96fb/trio/_core/_run.py#L2247-L2253

Do you have some ideas about what could cause this?

vxgmichel commented 3 years ago

Also, I tried to reproduce the issue with a minimal example but I couldn't. There's probably more to it, here's my attempt:

import trio
import qtrio

from PyQt5.QtWidgets import QApplication, QMainWindow

async def task():
    stream = await trio.open_tcp_stream("localhost", 8000)
    await stream.send_all(b"test")
    await stream.receive_some()

async def main():
    window = QMainWindow()
    window.show()
    async with trio.open_nursery() as nursery:
        nursery.start_soon(task)
        await trio.sleep(1)
        window.close()
        QApplication.quit()

if __name__ == "__main__":
    qtrio.run(main)

Run with:

nc localhost -l 8000 & python test.py
vxgmichel commented 3 years ago

I realized QApplication.quit() was the issue as it causes the qt loop to terminate without letting the trio loop terminate as well. However, the warning is not generated in all cases. Consider the following example that silently fails (it doesn't print Very important stuff!) without any trio warning:

import trio
import qtrio

from PyQt5.QtWidgets import QApplication, QMainWindow

async def task(window):
    await trio.sleep(1)
    window.close()
    QApplication.quit()

async def main():
    try:
        window = QMainWindow()
        window.show()
        async with trio.open_nursery() as nursery:
            nursery.start_soon(task, window)
            await trio.sleep(3)
    finally:
        print("Very important stuff!")

if __name__ == "__main__":
    qtrio.run(main)
altendky commented 3 years ago

Do you need to call QApplication.quit()? Can you just return from your main() and let QTrio handle the application?

https://github.com/altendky/qtrio/blob/cdd3fe71241b15fcefd5837d70d006de98269fa5/qtrio/_core.py#L731

https://qtrio.readthedocs.io/en/v0.4.2/core.html#qtrio.Runner.quit_application


quit_application
    When true, the done_callback() method will quit the application
    when the async function passed to qtrio.Runner.run() has completed.
vxgmichel commented 3 years ago

Do you need to call QApplication.quit() ?

I don't, it's simply an issue I ran into while migrating our code base. I think the problem I described in my last comment is quite serious as it's a common pattern to use methods like loop.stop() in asyncio or app.quit() in qt to stop the execution of an event loop, and silently missing finally clauses is a big deal.

I just ran into this section in the trio documentation:

You must let the Trio program run to completion. Many event loops let you stop the event loop at any point, and any pending callbacks/tasks/etc. just… don’t run. Trio follows a more structured system, where you can cancel things, but the code always runs to completion, so finally blocks run, resources are cleaned up, etc. If you stop your host loop early, before the done_callback is invoked, then that cuts off the Trio run in the middle without a chance to clean up. This can leave your code in an inconsistent state, and will definitely leave Trio’s internals in an inconsistent state, which will cause errors if you try to use Trio again in that thread.

Some programs need to be able to quit at any time, for example in response to a GUI window being closed or a user selecting a “Quit” from a menu. In these cases, we recommend wrapping your whole program in a trio.CancelScope, and cancelling it when you want to quit.

Maybe qtrio could wrap the execution of async_fn with a cancel scope and expose a method in the qtrio runner to stop the application properly? Also it'd be nice if qtrio could detect those incorrect shutdown of the event loop and show a warning advising against the use of QApplication.quit(). I think a warning in the documentation would also be useful.

I hope this makes sense :)

altendky commented 3 years ago

There's qtrio.Runner.cancel_scope.

I'll have to look more at the other bits to refresh myself. We might be able to check if the application is running at the end of every Trio callback into the Qt loop.

altendky commented 3 years ago

So looking back at this, it appears that there is already a RuntimeWarning that gets raise. It just references directly to Trio rather than QTrio. Would that warning customized for QTrio be something that would fulfill your interests? (plus discussion in the documentation, I haven't looked there yet)

vxgmichel commented 3 years ago

So looking back at this, it appears that there is already a RuntimeWarning that gets raise.

The warning is not always raised. Consider the example below:

import trio
import qtrio

from PyQt5.QtWidgets import QApplication, QMainWindow

async def task(window):
    await trio.sleep(1)
    window.close()
    QApplication.quit()

async def main():
    try:
        window = QMainWindow()
        window.show()
        async with trio.open_nursery() as nursery:
            nursery.start_soon(task, window)
            await trio.sleep(3)
    finally:
        print("Very important stuff!")

if __name__ == "__main__":
    qtrio.run(main)

See how Very important stuff is never printed and no warning gets displayed.

richardsheridan commented 3 years ago

Is it possible to monkeypatch QApplication.quit? That might be a way for qtrio to put a relevant warning up, if there's not really a legitimate use for it except in done_callback which qtrio handles.

graingert commented 3 years ago

Is it possible to monkeypatch QApplication.quit? That might be a way for qtrio to put a relevant warning up, if there's not really a legitimate use for it except in done_callback which qtrio handles.

apparently you can listen to aboutToQuit which runs after user input is disabled and before the event loop is closed

njsmith commented 3 years ago

Can you call exec repeatedly on the same QApplication? If so then I guess qtrio could detect when exec exits before trio, and just reinvoke exec, effectively making QApplication.quit a no-op.

The aboutToQuit signal does seem like it could at least reliably detect this case and give a good warning.

altendky commented 3 years ago

I'm not looking for a bunch of work right now so I wasn't going to take on actively keeping the loop going. Perhaps the monkey patching to disable quitting would be good, but I'm trying to muck around less, not more. Maybe later. For now I put in a warning triggered by aboutToQuit() (https://github.com/altendky/qtrio/pull/267). I'll take a look over it later. At least the docs could be fleshed out with some more things that can cause this. Perhaps they can be better integrated with the rest of the documentation as well.

altendky commented 3 years ago

Without agreeing to take it on now, it sounds like actually making sure everything runs in .aboutToQuit() will be needed.

https://doc.qt.io/qt-5/qcoreapplication.html#exec

We recommend that you connect clean-up code to the aboutToQuit() signal, instead of putting it in your application's main() function because on some platforms the exec() call may not return. For example, on Windows when the user logs off, the system terminates the process after Qt closes all top-level windows. Hence, there is no guarantee that the application will have time to exit its event loop and execute code at the end of the main() function after the exec() call.

thanks for that...

njsmith commented 3 years ago

while you could in theory block in aboutToQuit and run the trio guest loop to completion, I'm not sure it's worth trying... the problem is that the trio code is probably written to assume that the qt loop is available, so resuming it in an environment where it can't use qt apis isn't necessarily any better than just quitting with a loud warning.