Repository: jupyter/jupyter_client (python)

Issue:
jupyter_client v6.20 (and maybe also v7.0.0?) interrupts jupyter-enterprise-gateway on kernel change, instead of just interrupting the kernel
I ran into a strange issue this morning: I upgraded the version of jupyter_client in my jupyter-enterprise-gateway (JEG) deployment to v6.2.0 and then became unable to switch kernels. I could still start as many kernels as I liked normally, but the moment I tried to restart, stop, or change a kernel the gateway instance would crash. This was on a work machine, so unfortunately I can't copy/paste the error message, but essentially the `os.killpg` line here:

https://github.com/jupyter/jupyter_client/blob/c16c0c9b183c7aa209fe9e6e1a20bada78fbf7ae/jupyter_client/manager.py#L659-L664

was somehow sending `SIGINT` to JEG whenever a kernel change was requested, which then "gracefully" shut down due to "keyboard interrupt". Apparently the process group id of JEG itself is somehow getting passed into `os.killpg`.

For me the fix was to downgrade to jupyter_client v6.1.12, so I'm fairly certain that this is a jupyter_client issue and not a JEG problem. Since jupyter_client v6.2.0 has already been yanked from pypi for an unrelated version conflict issue, the question now is whether the problem is still present in jupyter_client v7.0.0. Though it moved around a bit, the relevant `os.killpg` command is still present in the latest HEAD:

https://github.com/jupyter/jupyter_client/blob/109e7ac137253b9245b672b27f0ca36c716b1be6/jupyter_client/provisioning/local_provisioner.py#L82-L86

so it may still cause problems for JEG.

For right now, this can't be directly tested as the latest version of JEG is pinned `jupyter_client<7`, since JEG is currently incompatible with the kernel provisioner stuff. So this issue is mostly just something to keep an eye out for in the near future.

pinging @kevin-bates 

Pull request:
Use formal method names when called internally
Fixup a couple more instances where we were calling the internal method name rather than the "aliased" (for lack of a better term) name.  This prevented some lower-level method overrides in subclasses from being called.  The test which was introduced for this purpose has been extended accordingly.

See https://github.com/jupyter/jupyter_client/issues/682#issuecomment-902260179 for more information.

Resolves #682

Patch:
diff --git a/jupyter_client/manager.py b/jupyter_client/manager.py
index d30fb92..0c489d3 100644
--- a/jupyter_client/manager.py
+++ b/jupyter_client/manager.py
@@ -373,7 +373,7 @@ class KernelManager(ConnectionFileMixin):
         except asyncio.TimeoutError:
             self.log.debug("Kernel is taking too long to finish, terminating")
             self._shutdown_status = _ShutdownStatus.SigtermRequest
-            await self._async_send_kernel_sigterm()
+            await ensure_async(self._send_kernel_sigterm())
 
         try:
             await asyncio.wait_for(
@@ -531,7 +531,7 @@ class KernelManager(ConnectionFileMixin):
             assert self.kernel_spec is not None
             interrupt_mode = self.kernel_spec.interrupt_mode
             if interrupt_mode == "signal":
-                await self._async_signal_kernel(signal.SIGINT)
+                await ensure_async(self.signal_kernel(signal.SIGINT))
 
             elif interrupt_mode == "message":
                 msg = self.session.msg("interrupt_request", content={})
@@ -574,7 +574,7 @@ class KernelManager(ConnectionFileMixin):
         # not alive.  If we find the process is no longer alive, complete
         # its cleanup via the blocking wait().  Callers are responsible for
         # issuing calls to wait() using a timeout (see _kill_kernel()).
-        while await self._async_is_alive():
+        while await ensure_async(self.is_alive()):
             await asyncio.sleep(pollinterval)
 
 


Test patch:
diff --git a/jupyter_client/tests/test_kernelmanager.py b/jupyter_client/tests/test_kernelmanager.py
index f90fbc6..2106391 100644
--- a/jupyter_client/tests/test_kernelmanager.py
+++ b/jupyter_client/tests/test_kernelmanager.py
@@ -298,6 +298,7 @@ class TestKernelManager:
 
     def test_subclass_callables(self, km_subclass):
         km_subclass.reset_counts()
+
         km_subclass.start_kernel(stdout=PIPE, stderr=PIPE)
         assert km_subclass.call_count("start_kernel") == 1
         assert km_subclass.call_count("_launch_kernel") == 1
@@ -305,6 +306,7 @@ class TestKernelManager:
         is_alive = km_subclass.is_alive()
         assert is_alive
         km_subclass.reset_counts()
+
         km_subclass.restart_kernel(now=True)
         assert km_subclass.call_count("restart_kernel") == 1
         assert km_subclass.call_count("shutdown_kernel") == 1
@@ -313,26 +315,32 @@ class TestKernelManager:
         assert km_subclass.call_count("cleanup_resources") == 1
         assert km_subclass.call_count("start_kernel") == 1
         assert km_subclass.call_count("_launch_kernel") == 1
+        assert km_subclass.call_count("signal_kernel") == 1
 
         is_alive = km_subclass.is_alive()
         assert is_alive
-
+        assert km_subclass.call_count("is_alive") >= 1
         km_subclass.reset_counts()
+
         km_subclass.interrupt_kernel()
         assert km_subclass.call_count("interrupt_kernel") == 1
+        assert km_subclass.call_count("signal_kernel") == 1
 
         assert isinstance(km_subclass, KernelManager)
-
         km_subclass.reset_counts()
+
         km_subclass.shutdown_kernel(now=False)
         assert km_subclass.call_count("shutdown_kernel") == 1
         assert km_subclass.call_count("interrupt_kernel") == 1
         assert km_subclass.call_count("request_shutdown") == 1
         assert km_subclass.call_count("finish_shutdown") == 1
         assert km_subclass.call_count("cleanup_resources") == 1
+        assert km_subclass.call_count("signal_kernel") == 1
+        assert km_subclass.call_count("is_alive") >= 1
 
         is_alive = km_subclass.is_alive()
         assert is_alive is False
+        assert km_subclass.call_count("is_alive") >= 1
         assert km_subclass.context.closed
 
 
@@ -516,13 +524,16 @@ class TestAsyncKernelManager:
 
     async def test_subclass_callables(self, async_km_subclass):
         async_km_subclass.reset_counts()
+
         await async_km_subclass.start_kernel(stdout=PIPE, stderr=PIPE)
         assert async_km_subclass.call_count("start_kernel") == 1
         assert async_km_subclass.call_count("_launch_kernel") == 1
 
         is_alive = await async_km_subclass.is_alive()
         assert is_alive
+        assert async_km_subclass.call_count("is_alive") >= 1
         async_km_subclass.reset_counts()
+
         await async_km_subclass.restart_kernel(now=True)
         assert async_km_subclass.call_count("restart_kernel") == 1
         assert async_km_subclass.call_count("shutdown_kernel") == 1
@@ -531,24 +542,30 @@ class TestAsyncKernelManager:
         assert async_km_subclass.call_count("cleanup_resources") == 1
         assert async_km_subclass.call_count("start_kernel") == 1
         assert async_km_subclass.call_count("_launch_kernel") == 1
+        assert async_km_subclass.call_count("signal_kernel") == 1
 
         is_alive = await async_km_subclass.is_alive()
         assert is_alive
-
+        assert async_km_subclass.call_count("is_alive") >= 1
         async_km_subclass.reset_counts()
+
         await async_km_subclass.interrupt_kernel()
         assert async_km_subclass.call_count("interrupt_kernel") == 1
+        assert async_km_subclass.call_count("signal_kernel") == 1
 
         assert isinstance(async_km_subclass, AsyncKernelManager)
-
         async_km_subclass.reset_counts()
+
         await async_km_subclass.shutdown_kernel(now=False)
         assert async_km_subclass.call_count("shutdown_kernel") == 1
         assert async_km_subclass.call_count("interrupt_kernel") == 1
         assert async_km_subclass.call_count("request_shutdown") == 1
         assert async_km_subclass.call_count("finish_shutdown") == 1
         assert async_km_subclass.call_count("cleanup_resources") == 1
+        assert async_km_subclass.call_count("signal_kernel") == 1
+        assert async_km_subclass.call_count("is_alive") >= 1
 
         is_alive = await async_km_subclass.is_alive()
         assert is_alive is False
+        assert async_km_subclass.call_count("is_alive") >= 1
         assert async_km_subclass.context.closed
diff --git a/jupyter_client/tests/utils.py b/jupyter_client/tests/utils.py
index a1ef0a6..58d963d 100644
--- a/jupyter_client/tests/utils.py
+++ b/jupyter_client/tests/utils.py
@@ -145,6 +145,18 @@ class KMSubclass(RecordCallMixin):
     def cleanup_resources(self, restart=False):
         """ Record call and defer to superclass """
 
+    @subclass_recorder
+    def signal_kernel(self, signum: int):
+        """ Record call and defer to superclass """
+
+    @subclass_recorder
+    def is_alive(self):
+        """ Record call and defer to superclass """
+
+    @subclass_recorder
+    def _send_kernel_sigterm(self, restart: bool = False):
+        """ Record call and defer to superclass """
+
 
 class SyncKMSubclass(KMSubclass, KernelManager):
     """Used to test subclass hierarchies to ensure methods are called when expected."""