-
-
Notifications
You must be signed in to change notification settings - Fork 42
Handle kernel restarts #304
Conversation
For now the This means that the handler triggers the restore flow before the restart, and the |
6740196
to
de78767
Compare
f8aa3be
to
4fcb68c
Compare
// 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; | ||
} | ||
}; |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
da8a7b0
to
976a413
Compare
Rebased. |
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. |
976a413
to
2dafa81
Compare
There was a problem hiding this 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.
Yes, I think we'll need to revisit the logic a bit. Thanks for looking at it 👍 |
Fixes #76, replaces #276.