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

Restart the Launcher if it had crashed when invoked #7127

Merged
merged 1 commit into from
Oct 13, 2020

Conversation

ivan100sic
Copy link
Contributor

Summary of the Pull Request

Could apply to issues such as this one #6605. If the Launcher crashes, when we invoke it, nothing happens until it is manually restarted. This PR restarts the launcher if we detect that it crashed or exited somehow.

PR Checklist

  • Applies to PowerToys Run doesn't open #6605
  • CLA signed. If not, go over here and sign the CLA
  • Tests added/passed
  • Requires documentation to be updated
  • I've discussed this with core contributors already. If not checked, I'm ready to accept this work might be rejected in favor of a different grand plan. Issue number where discussion took place: #xxx

Validation Steps Performed

Start PT, enable the Launcher, terminate it using the Task Manager and hit the invoke hotkey. The launcher should appear immediately in the Task Manager and should show up on the screen in a few seconds.

@arjunbalgovind
Copy link
Contributor

Could this cause a crash loop if there is some unhandled exception on launcher startup?

@ivan100sic
Copy link
Contributor Author

Could this cause a crash loop if there is some unhandled exception on launcher startup?

Only if you hit the invoke hotkey, it's no worse than what we have now.

@crutkas
Copy link
Member

crutkas commented Oct 7, 2020

/azp run

@azure-pipelines
Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@htcfreek
Copy link
Collaborator

htcfreek commented Oct 9, 2020

Could this cause a crash loop if there is some unhandled exception on launcher startup?

Only if you hit the invoke hotkey, it's no worse than what we have now.

I know that's maybe not a nice workaround.

But what about adding a counter like trying three time to restart and then stop until hotkey is pressed again.

@enricogior
Copy link
Contributor

@htcfreek

But what about adding a counter like trying three time to restart and then stop until hotkey is pressed again.

No need for doing that, we should spend time making the actual module robust rather than add workarounds.

@enricogior enricogior added Area-Runner The PowerToys main executable Product-PowerToys Run Improved app launch PT Run (Win+R) Window and removed Area-Runner The PowerToys main executable labels Oct 9, 2020
Copy link
Contributor

@alekhyareddy28 alekhyareddy28 left a comment

Choose a reason for hiding this comment

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

LGTM! Thanks for this. Validated that PT Run is restarted on pressing the hotkey, after it's killed.

@ivan100sic
Copy link
Contributor Author

@alekhyareddy28
A bit off-topic, I tried to figure out why we need to kill the Launcher instead of quitting it. I tried calling System.Windows.Forms.Application.Exit(); from within the Launcher but that doesn't seem to work. Is this a known issue?

@ivan100sic ivan100sic merged commit bb8cc0a into microsoft:master Oct 13, 2020
@alekhyareddy28
Copy link
Contributor

@ivan100sic, I'm not aware of it. I remember @somil55 working on the issue where PT Run was not exiting as expected a while back but that was related to objects not being disposed off properly. I think he can add more to this and answer your question.

@dsrivastavv
Copy link
Contributor

@ivan100sic #5860 captures this issue and is discussed in #5860 (comment)

@ivan100sic ivan100sic deleted the restart-launcher branch October 20, 2020 11:26
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Product-PowerToys Run Improved app launch PT Run (Win+R) Window
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants