Skip to content
This repository was archived by the owner on Jan 26, 2022. It is now read-only.

Handle kernel restarts #304

Merged
merged 2 commits into from
Dec 20, 2019
Merged

Handle kernel restarts #304

merged 2 commits into from
Dec 20, 2019

Conversation

jtpio
Copy link
Member

@jtpio jtpio commented Dec 18, 2019

Fixes #76, replaces #276.

kernel-restart2

@jtpio jtpio added this to the 0.1.0 milestone Dec 18, 2019
@jtpio
Copy link
Member Author

jtpio commented Dec 18, 2019

For now the restarting status seems to be emitted before the kernel is actually restarted.

This means that the handler triggers the restore flow before the restart, and the debugInfo returns isStarted: true.

@jtpio jtpio force-pushed the kernel-restarts branch 2 times, most recently from 6740196 to de78767 Compare December 18, 2019 14:06
@jtpio
Copy link
Member Author

jtpio commented Dec 18, 2019

This should also handle kernels being killed and automatically restarted:

kill-kernel-restarts

@jtpio jtpio force-pushed the kernel-restarts branch 2 times, most recently from f8aa3be to 4fcb68c Compare December 18, 2019 17:23
Comment on lines +155 to +168
// setup handler when the status of the kernel changes (restart)
// TODO: is there a better way to handle restarts?
let restarted = false;
const statusChanged = async () => {
// wait for the first `idle` status after a restart
if (restarted && client.status === 'idle') {
restarted = false;
return updateHandler();
}
// handle `starting`, `restarting` and `autorestarting`
if (client.status.endsWith('starting')) {
restarted = true;
}
};
Copy link
Member Author

Choose a reason for hiding this comment

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

Not super satisfied with this logic.

It seems to be a bit tricky to really detect a kernel restart.

Copy link
Member Author

Choose a reason for hiding this comment

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

An alternative would be to schedule the call to updateHandler with a small time delay.

@jtpio jtpio marked this pull request as ready for review December 18, 2019 19:33
@jtpio
Copy link
Member Author

jtpio commented Dec 18, 2019

Rebased.

@jtpio
Copy link
Member Author

jtpio commented Dec 19, 2019

cc @afshin @KsavinN if you want to try it.

This should handle most of the restart scenarios, for example a click on the restart button or a kernel killed from the command line.

However the logic to properly detect a kernel restart is a bit convoluted, and it would be great to have a second look at it later on if we decide to keep it like this for now.

Copy link
Member

@afshin afshin left a comment

Choose a reason for hiding this comment

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

Awesome, thanks! There are still kernel restart quirks but this is strictly an improvement so let's leave those for another PR.

@afshin afshin merged commit c93e5fa into jupyterlab:master Dec 20, 2019
@jtpio
Copy link
Member Author

jtpio commented Dec 20, 2019

Yes, I think we'll need to revisit the logic a bit.

Thanks for looking at it 👍

@jtpio jtpio deleted the kernel-restarts branch December 20, 2019 16:34
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.

Handle kernel status
2 participants