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

Reset execution indicator state when kernel restarts #13832

Merged
merged 3 commits into from
Jan 30, 2023

Conversation

krassowski
Copy link
Member

References

Fixes #13780.

Code changes

Add special handling for restarting message, which is the only message we get if the kernel is killed non-gracefully. Because abrupt killing of kernel prevents it from responding with idle status we need to listen to restarting instead (and clear the set with scheduled cells).

User-facing changes

Execution indicator works after abrupt kernel death.

Backwards-incompatible changes

None

@krassowski krassowski added the bug label Jan 21, 2023
@krassowski krassowski added this to the 3.6.x milestone Jan 21, 2023
@jupyterlab-probot
Copy link

Thanks for making a pull request to jupyterlab!
To try out this branch on binder, follow this link: Binder

@krassowski krassowski marked this pull request as draft January 22, 2023 16:58
@krassowski
Copy link
Member Author

The added test is a bit blunt and while it works perfectly locally it does not pass on CI.

I did not find another way to reproduce the issue with a two-liner, but suggestions for an alternative testing strategy (or how to make the tests pass) are very much welcome!

@krassowski
Copy link
Member Author

I was able to reproduce the CI error locally; it was because previously I was only running the added test in isolation; as it was killing kernel we had to restart it. This was fixed in 40acbf2.

For record the traceback when tests were failing was:

Traceback (most recent call last):
  File "tornado/web.py", line 1713, in _execute
    result = await result
  File "jupyter_server/services/sessions/handlers.py", line 190, in delete
    await sm.delete_session(session_id)
  File "jupyter_server/services/sessions/sessionmanager.py", line 523, in delete_session
    await ensure_async(self.kernel_manager.shutdown_kernel(session["kernel"]["id"]))
  File "jupyter_core/utils/__init__.py", line 172, in ensure_async
    result = await obj
  File "jupyter_server/services/kernels/kernelmanager.py", line 426, in _async_shutdown_kernel
    return await self.pinned_superclass._async_shutdown_kernel(
  File "jupyter_client/multikernelmanager.py", line 264, in _async_shutdown_kernel
    if not km.ready.cancelled() and km.ready.exception():
asyncio.exceptions.InvalidStateError: Exception is not set.

@krassowski krassowski marked this pull request as ready for review January 30, 2023 00:12
@trungleduc
Copy link
Member

Thanks @krassowski!

@fcollonval fcollonval modified the milestones: 3.6.x, 3.6.0 Jan 30, 2023
@fcollonval fcollonval merged commit 98e684c into jupyterlab:master Jan 30, 2023
@fcollonval
Copy link
Member

@meeseeksdev please backport to 3.6.x

@lumberbot-app
Copy link

lumberbot-app bot commented Jan 30, 2023

Owee, I'm MrMeeseeks, Look at me.

There seem to be a conflict, please backport manually. Here are approximate instructions:

  1. Checkout backport branch and update it.
git checkout 3.6.x
git pull
  1. Cherry pick the first parent branch of the this PR on top of the older branch:
git cherry-pick -x -m1 98e684cfbe8982b65d9ea518ccb9801762cc8a8a
  1. You will likely have some merge/cherry-pick conflict here, fix them and commit:
git commit -am 'Backport PR #13832: Reset execution indicator state when kernel restarts'
  1. Push to a named branch:
git push YOURFORK 3.6.x:auto-backport-of-pr-13832-on-3.6.x
  1. Create a PR against branch 3.6.x, I would have named this PR:

"Backport PR #13832 on branch 3.6.x (Reset execution indicator state when kernel restarts)"

And apply the correct labels and milestones.

Congratulations — you did some good work! Hopefully your backport PR will be tested by the continuous integration and merged soon!

Remember to remove the Still Needs Manual Backport label once the PR gets merged.

If these instructions are inaccurate, feel free to suggest an improvement.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Incorrect kernel status after it dies unexpectedly and is restarted
3 participants