-
Notifications
You must be signed in to change notification settings - Fork 5.1k
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
support async kernel shutdown #4425
Conversation
This is an "add-on" commit to PR jupyter#4412 in which kernel startup was addressed.
I believe what @kevin-bates means is: could this be also merged into 5.7.x branch and be part of the possible release that is going to handle the tornado version cap. |
EDIT: Issue was in jupyter_client. Since we're going to want to address this shortly, we should continue with this change since its independent of jc. |
@@ -300,7 +301,8 @@ def shutdown_kernel(self, kernel_id, now=False): | |||
type=self._kernels[kernel_id].kernel_name | |||
).dec() | |||
|
|||
return super(MappingKernelManager, self).shutdown_kernel(kernel_id, now=now) | |||
ret = yield gen.maybe_future(super(MappingKernelManager, self).shutdown_kernel(kernel_id, now=now)) | |||
raise gen.Return(ret) |
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.
This already supported async shutdown_kernel without any change, so I don't think we should merge this. Since shutdown_kernel
is not itself async, returning the super().shutdown_kernel()
works if shutdown_kernel is async, since it will return a Future or coroutine object, which gets passed through to the caller of this method.
That's fine. If need be we can revisit since I had to back out the jupyter_client portion of this since there were restart side-effects that require further investigation. Closing. |
This is an "add-on" commit to PR #4412 in which kernel startup was addressed. As with that PR, this should be considered a "sibling PR" to the same jupyter_client PR - which I just updated with a shutdown-related commit.
I should have looked into shutdowns sooner. I thought it would be more involved than it was. Sorry about that.