envisage icon indicating copy to clipboard operation
envisage copied to clipboard

Make TasksApplication.exit less fragile

Open mdickinson opened this issue 2 years ago • 2 comments

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

With the current TasksApplication design:

  • The event loop is exited when all windows are closed
  • The TasksApplication.exit method closes all TaskWindow windows (provided none of them vetoes the close), but is not aware of, and does not close, other windows (e.g., dialog windows that may be open).

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 avatar Feb 08 '23 11:02 mdickinson

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 avatar Feb 08 '23 11:02 mdickinson

#502 is mildly related

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

mdickinson avatar Mar 22 '23 12:03 mdickinson