PR: Fix shutdown kernels associated to conda envs (IPython console)

[ccordoba12]

Description of Changes

• That was broken since jupyter_client 6.1.12.
• That made the kernel process to linger around after creating plots with the Automatic backend and then killing it (seems to happen only on Windows).
• Constraint IPython to be less than 8 to avoid headaches when that version is released.
• Depends on PR: Increase minimal required version of jupyter-client to 7.0+ for Python 3 spyder-kernels#356
• Depends on PR: Require jupyter-client 7.1.0 to fix issues on Windows spyder-kernels#357

Issue(s) Resolved

Fixes #17011
Fixes #14739

Affirmation

By submitting this Pull Request or typing my (user)name below,
I affirm the Developer Certificate of Origin
with respect to all commits and content included in this PR,
and understand I am releasing the same under Spyder's MIT (Expat) license.

I certify the above statement is true and correct: @ccordoba12

[ccordoba12]

@impact27, I think this change makes unnecessary the shutdown thread you added to avoid freezes (since the kernel is killed immediately, Spyder is not blocked by having to wait for it). Could you test that?

Of course, that's for another PR, but I'm curious about it.

[impact27]

The problem with that is that we don’t give any time to the kernel to try to shut down. The goal of my previous change was to give a little time (10s) to the kernel to shut down before killing it.

[impact27]

That is what ipykernel is doing. check e.g. _async_finish_shutdown. I suspect something is wrong with SpyderKernelManager

[ccordoba12]

As far as I can tell, there are two ways to kill a kernel:

1. Call _kill_kernel (when now is passed to the shutdown_kernel method)
2. Call finish_shutdown, as you said.

finish_shutdown in turn calls _send_kernel_sigterm here:

which ends up calling the terminate method of the subprocess associated to the kernel:

The problem with that is that, on Windows and after creating a plot with Automatic backend, terminate kills the batch script we use to activate the conda environment associated to the kernel, but not the kernel itself due to an AccessDenied error.

So, in the SpyderKernelManager we use psutil to kill all child processes associated to the activation script:

spyder/spyder/plugins/ipythonconsole/utils/manager.py

Lines 58 to 75 in d49b92a

	try:
	parent = psutil.Process(pid)
	except psutil.NoSuchProcess:
	return ([], [])
	children = parent.children(recursive=True)
	if include_parent:
	children.append(parent)
	for child_process in children:
	# This is necessary to avoid an error when restarting the
	# kernel that started a PyQt5 application in the background.
	# Fixes spyder-ide/spyder#13999
	try:
	child_process.send_signal(sig)
	except psutil.AccessDenied:
	return ([], [])

But for that, we need to use the now parameter in shutdown_kernel, so that we end up calling the _kill_kernel method we override in that class and not finish_shutdown.

Of course, another option would be to override finish_shutdown in our kernel manager and use psutil there instead of calling terminate. But I still don't understand why it's important to give the kernel some time to die, instead of killing it directly.

[impact27]

If the kernel is killed directly, it doesn't have the opportunity to run any cleanup code (Any __del__ methods for example). This is not a massive issue but could be problematic in some cases.

[impact27]

Is there a reason why psutil is not implemented upstream in jupyter_client?

[impact27]

Maybe we should override _send_kernel_sigterm?

[ccordoba12]

Maybe we should override _send_kernel_sigterm?

@impact27, I followed your advice in my latest commits. Please take a look at this PR again and let me know what you think about it.

[impact27]

This PR now reimplements _async_send_kernel_sigterm. I think it should also reimplement _async_kill_kernel. This is because both are used to shut down the kernel. (One to terminate, the other to kill).

We call _async_kill_kernel in spyder/plugins/ipythonconsole/widgets/shell.py:
_kill_kernel() is called in wait_all_shutdown

Furthermore, when shutdown_kernel() is called: (assuming jupyter_clients 6.0.2) in jupyter_client/manager.py
async def _async_shutdown_kernel

• calls self.interrupt_kernel()
• if now=True: calls self._kill_kernel() (not our case)
• else: request shutdown and then self.finish_shutdown()

So the interesting code path is finish_shutdown. The shutdown time is controlled by self.shutdown_wait_time in async def _async_finish_shutdown. First, it tries to terminate with:

• await self._async_send_kernel_sigterm()
  If that doesn't work, it kills with:
• await ensure_async(self._kill_kernel())

[ccordoba12]

This PR now reimplements _async_send_kernel_sigterm. I think it should also reimplement _async_kill_kernel. This is because both are used to shut down the kernel. (One to terminate, the other to kill).

Ok, I restored the previous _async_kill_kernel. I removed it because I thought it was not necessary anymore, but you're right, it is.

[impact27]

It looks good to me now. I wonder if killing rather than sending sigterm could produce issues but I can’t really
Think of any

[ccordoba12]

It looks good to me now

Great! Thanks for your help reviewing this one.

I wonder if killing rather than sending sigterm could produce issues but I can’t really
Think of any

Well, we send sigterm to the parent process and all its children here:

spyder/spyder/plugins/ipythonconsole/utils/manager.py

Lines 62 to 69 in dab29a2

	for child_process in children:
	# This is necessary to avoid an error when restarting the
	# kernel that started a PyQt5 application in the background.
	# Fixes spyder-ide/spyder#13999
	try:
	child_process.send_signal(sig)
	except psutil.AccessDenied:
	return ([], [])

(sig is sigterm).