Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Make TasksApplication.exit less fragile #505

Open
mdickinson opened this issue Feb 8, 2023 · 2 comments
Open

Make TasksApplication.exit less fragile #505

mdickinson opened this issue Feb 8, 2023 · 2 comments

Comments

@mdickinson
Copy link
Member

mdickinson commented Feb 8, 2023

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
Copy link
Member Author

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
Copy link
Member Author

#502 is mildly related

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

1 participant