enthought / envisage

Envisage is a Python-based framework for building applications whose functionalities can be extended by adding "plug-ins".
http://docs.enthought.com/envisage/
Other
81 stars 26 forks source link

Make TasksApplication.exit less fragile #505

Open mdickinson opened 1 year ago

mdickinson commented 1 year ago

tl;dr: It's way too easy to make TasksApplication hang at application exit time.

With the current TasksApplication design:

This can lead to hangs at application exit time: the user closes the application, all windows become invisible, but the event loop is still running. This is particularly problematic when there are exit tasks with side-effects - e.g., storing layout state, or preferences state, since interrupting the hanging application will bypass those exit tasks.

In a downstream application class that inherits from TasksApplication, we've ended up having to implement a workaround along the following lines:

    def exit(self, force=False):
        """
        Exits the application, closing all windows.
        """
        # We extend the base class TasksApplication.exit method. That method
        # does a veto-able close on all TaskWindow windows, but does not close
        # other windows (for example dialogs) that may be present. Since the
        # event loop doesn't exit until _all_ windows are closed, this can lead
        # to hangs if there's a dialog window open at the time that this method
        # is called.

        # This method should be considered a workaround; a better solution
        # would be to fix at the Envisage level.
        # xref: https://github.com/enthought/envisage/issues/505
        exited = super().exit(force=force)
        if exited:
            # Exit was not vetoed by any of the TaskWindows. Ensure that all
            # windows are closed so that we can exit the event loop.
            QApplication.instance().closeAllWindows()
        return exited

Rather than requiring downstream projects to work around this, it would be good to find a cleaner way to do this at TasksApplication level.

502 is mildly related: if we're making changes to the TasksApplication life cycle, we should fix #502 while we're at it. (This doesn't affect most of our downstream projects, since they override the run method.)

mdickinson commented 1 year ago

A good solution would also expose a standard way to exit an application without the necessity of waiting for all windows to be closed.

mdickinson commented 1 year ago

502 is mildly related

502 is now fixed, for what it's worth.