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

Rework restart/interrupt to actually restart the entire server #5182

Merged
merged 3 commits into from
Apr 10, 2019

Conversation

rchiodo
Copy link

@rchiodo rchiodo commented Apr 10, 2019

For #5025

  • Pull request represents a single change (i.e. not fixing disparate/unrelated things in a single PR)
  • Title summarizes what is changing
  • Has a news entry file (remember to thank yourself!)
  • Has sufficient logging.
  • Has telemetry for enhancements.
  • Unit tests & system/integration tests are added/updated
  • Test plan is updated as appropriate
  • package-lock.json has been regenerated by running npm install (if dependencies have changed)
  • The wiki is updated with any design decisions/details.

@rchiodo rchiodo self-assigned this Apr 10, 2019
const no = localize.DataScience.restartKernelMessageNo();
this.applicationShell.showInformationMessage(message, yes, no).then(v => {
if (v === yes) {
this.restartKernelInternal().ignoreErrors();
Copy link
Member

Choose a reason for hiding this comment

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

From my reading I think this is intentional, but it looks like interruptKernel and restartKernel are not handling their asyncness the same way with regards to this. In particular restart uses await for the operations so it doesn't return back until the operations are done, but interruptKernel can return back while the restart operation is still queued. Is that how it's intended?

Copy link
Member

Choose a reason for hiding this comment

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

Actually never mind it's already in a then. Never mind.


In reply to: 274037540 [](ancestors = 274037540)

Copy link
Author

Choose a reason for hiding this comment

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

I think I should just await the showInformationMessage and then await the restartKernelInternal. That would be clearer. It wasn't directly intended for it to queue up the restart.


In reply to: 274037540 [](ancestors = 274037540)

this.restartKernelInternal().ignoreErrors();
}
});
} else if (result === InterruptResult.Restarted) {
Copy link
Member

Choose a reason for hiding this comment

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

[](start = 18, length = 1)

extra space here.

Copy link
Member

@IanMatthewHuff IanMatthewHuff left a comment

Choose a reason for hiding this comment

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

:shipit:

@rchiodo rchiodo merged commit a3103bf into master Apr 10, 2019
@rchiodo rchiodo deleted the rchiodo/fix_restart_interrupt branch April 10, 2019 16:23
@lock lock bot locked as resolved and limited conversation to collaborators Jul 30, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants