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

Execute app shutdown vetoes in sequence #10861

Merged
merged 1 commit into from
Mar 21, 2022
Merged

Conversation

msujew
Copy link
Member

@msujew msujew commented Mar 8, 2022

What it does

Closes #10860

Since we prioritize actions anyway, it didn't make a lot of sense to execute the actions in parallel. For example, if we have the following two OnWillStop results, the second one returns first, even though its priority is lower:

[{
  reason: 'a',
  priority: 10,
  action: async () => {
    await timeout(10);
    return ...;
  }
},{
  reason: 'b',
  priority: 0,
  action: async () => {
    return ...;
  }
}]

How to test

  1. Create a new multi-root workspace and don't save it
  2. Edit a file in your workspace and don't save it
  3. Exit the app
  4. Any shutdown action returning false will prevent execution of subsequent actions

Review checklist

Reminder for reviewers

@msujew msujew requested a review from colin-grant-work March 8, 2022 13:04
@colin-grant-work
Copy link
Contributor

The motivation for running the actions in parallel is that it's possible for an action to resolve itself without any user intervention (e.g. terminals need to check whether there are running processes in the terminals, which is asynch but may end up finding none). In that case, running several such actions in sequence would only delay shutdown. Is there a definite reason for wanting to run the actions sequentially?

@msujew
Copy link
Member Author

msujew commented Mar 8, 2022

I liked the parallel approach initially as well, however I noticed one issue in particular with it: When we have multiple FrontendApplicationContributions which have their own dialog to ensure that the user definitely wants to quit the app - even though it would be wise not to - every dialog after the first canceled dialog is completely useless and has no effect on the shutdown procedure. This is kind of bad UX in my opinion.

The reproduction/testing steps show this issue fairly well with the base app. This becomes more exaggerated in downstream Theia apps, where there are more conditions which might prevent the app from shutting down gracefully.

@colin-grant-work
Copy link
Contributor

Ah - that clarifies the problem for me. It definitely makes sense to bail after the first rejected option. Your code definitely does that - let me think a moment about other possibilities.

@colin-grant-work
Copy link
Contributor

The current implementation was influenced by VSCode's, and VSCode has the same problem: if you have two vetoes with dialogs (e.g. dirty file and terminal), canceling one doesn't stop the next one from appearing. I don't really see an alternative to running in sequence if we want to avoid showing meaningless dialogs.

@msujew
Copy link
Member Author

msujew commented Mar 10, 2022

@colin-grant-work I have an idea which should at least alleviate the issue of "data that needs some time to be prepared". What if we change the OnWillStopAction to be:

interface OnWillStopAction<T = undefined> {
  reason: string
  priority?: number
  prepare?(): MaybePromise<T>
  action(prepared: T): MaybePromise<boolean>
}

We could run OnWillStopAction#prepare in parallel, while passing the prepared return values in sequence into the action method. WDYT?

@colin-grant-work
Copy link
Contributor

... WDYT?

@msujew, I think I like it :-).

@msujew msujew force-pushed the msujew/veto-sequencing branch from 06ef72c to 1dfd87a Compare March 11, 2022 17:46
Copy link
Contributor

@colin-grant-work colin-grant-work left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This looks good to me. It should not break any implementations oriented to the older behavior, but it does provide flexibility and cancel any irrelevant dialogues.

@msujew msujew merged commit 2ba1574 into master Mar 21, 2022
@msujew msujew deleted the msujew/veto-sequencing branch March 21, 2022 16:23
@github-actions github-actions bot added this to the 1.24.0 milestone Mar 21, 2022
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

Successfully merging this pull request may close these issues.

Application shutdown should stop asking for vetoes after first rejection
2 participants