Use heuristic to identify "stable" startup of kernel

This is mainly due to the weakness of the previous method: `is_alive` only tests that the kernel process is alive, it does not indicate that the kernel has successfully completed startup. To solve this correctly, we would need to wait for a kernel info reply, but it is not necessarily appropriate to start a kernel client + channels in the restarter. Therefore, we use a "has been alive continuously for X time" as a heuristic for a stable start up.

Author: vidartf

jupyter_client/ioloop/restarter.py

@@ -6,6 +6,7 @@
 # Copyright (c) Jupyter Development Team.
 # Distributed under the terms of the Modified BSD License.
 import inspect
+import time
 import warnings
 
 from traitlets import Instance
@@ -34,7 +35,10 @@ def start(self):
         """Start the polling of the kernel."""
         if self._pcallback is None:
             if inspect.isawaitable(self.poll):
-                cb = lambda: run_sync(self.poll)
+
+                def cb():
+                    run_sync(self.poll)
+
             else:
                 cb = self.poll
             self._pcallback = ioloop.PeriodicCallback(
@@ -55,13 +59,15 @@ async def poll(self):
         if self.debug:
             self.log.debug("Polling kernel...")
         is_alive = await self.kernel_manager.is_alive()
+        now = time.time()
         if not is_alive:
+            self._last_dead = now
             if self._restarting:
                 self._restart_count += 1
             else:
                 self._restart_count = 1
 
-            if self._restart_count >= self.restart_limit:
+            if self._restart_count > self.restart_limit:
                 self.log.warning("AsyncIOLoopKernelRestarter: restart failed")
                 self._fire_callbacks("dead")
                 self._restarting = False
@@ -79,8 +85,8 @@ async def poll(self):
                 await self.kernel_manager.restart_kernel(now=True, newports=newports)
                 self._restarting = True
         else:
-            if self._initial_startup:
+            if self._initial_startup and self._last_dead - now >= self.stable_start_time:
                 self._initial_startup = False
-            if self._restarting:
+            if self._restarting and self._last_dead - now >= self.stable_start_time:
                 self.log.debug("AsyncIOLoopKernelRestarter: restart apparently succeeded")
-            self._restarting = False
+                self._restarting = False

jupyter_client/restarter.py

@@ -7,7 +7,10 @@
 """
 # Copyright (c) Jupyter Development Team.
 # Distributed under the terms of the Modified BSD License.
+import time
+
 from traitlets import Bool  # type: ignore
+from traitlets import default  # type: ignore
 from traitlets import Dict
 from traitlets import Float
 from traitlets import Instance
@@ -31,6 +34,12 @@ class KernelRestarter(LoggingConfigurable):
 
     time_to_dead = Float(3.0, config=True, help="""Kernel heartbeat interval in seconds.""")
 
+    stable_start_time = Float(
+        10.0,
+        config=True,
+        help="""The time in seconds to consider the kernel to have completed a stable start up.""",
+    )
+
     restart_limit = Integer(
         5,
         config=True,
@@ -45,6 +54,11 @@ class KernelRestarter(LoggingConfigurable):
     _restarting = Bool(False)
     _restart_count = Integer(0)
     _initial_startup = Bool(True)
+    _last_dead = Float()
+
+    @default("_last_dead")
+    def _default_last_dead(self):
+        return time.time()
 
     callbacks = Dict()
 
@@ -103,13 +117,15 @@ def poll(self):
         if self.kernel_manager.shutting_down:
             self.log.debug("Kernel shutdown in progress...")
             return
+        now = time.time()
         if not self.kernel_manager.is_alive():
+            self._last_dead = now
             if self._restarting:
                 self._restart_count += 1
             else:
                 self._restart_count = 1
 
-            if self._restart_count >= self.restart_limit:
+            if self._restart_count > self.restart_limit:
                 self.log.warning("KernelRestarter: restart failed")
                 self._fire_callbacks("dead")
                 self._restarting = False
@@ -127,8 +143,8 @@ def poll(self):
                 self.kernel_manager.restart_kernel(now=True, newports=newports)
                 self._restarting = True
         else:
-            if self._initial_startup:
+            if self._initial_startup and self._last_dead - now >= self.stable_start_time:
                 self._initial_startup = False
-            if self._restarting:
+            if self._restarting and self._last_dead - now >= self.stable_start_time:
                 self.log.debug("KernelRestarter: restart apparently succeeded")
-            self._restarting = False
+                self._restarting = False

jupyter_client/tests/problemkernel.py

@@ -2,10 +2,7 @@
 # Copyright (c) Jupyter Development Team.
 # Distributed under the terms of the Modified BSD License.
 import os
-import signal
 import time
-from subprocess import PIPE
-from subprocess import Popen
 
 from ipykernel.displayhook import ZMQDisplayHook
 from ipykernel.kernelapp import IPKernelApp

jupyter_client/tests/test_restarter.py

@@ -1,28 +1,22 @@
 """Tests for the KernelManager"""
 # Copyright (c) Jupyter Development Team.
 # Distributed under the terms of the Modified BSD License.
-
 import asyncio
-import concurrent.futures
 import json
 import os
-import signal
 import sys
-import time
-from subprocess import PIPE
 
 import pytest
 from jupyter_core import paths
 from traitlets.config.loader import Config
 from traitlets.log import get_logger
 
-from jupyter_client import AsyncKernelManager
-from jupyter_client.ioloop import AsyncIOLoopKernelManager, IOLoopKernelManager
-from ..manager import start_new_async_kernel
-from ..manager import start_new_kernel
+from jupyter_client.ioloop import AsyncIOLoopKernelManager
+from jupyter_client.ioloop import IOLoopKernelManager
 
 pjoin = os.path.join
 
+
 def _install_kernel(name="problemtest", extra_env=None):
     if extra_env is None:
         extra_env = dict()
@@ -46,29 +40,31 @@ def _install_kernel(name="problemtest", extra_env=None):
         )
     return name
 
+
 @pytest.fixture
 def install_kernel():
     return _install_kernel("problemtest")
 
+
 @pytest.fixture
 def install_fail_kernel():
-    return _install_kernel("problemtest-fail", extra_env={
-        "FAIL_ON_START": "1"
-    })
+    return _install_kernel("problemtest-fail", extra_env={"FAIL_ON_START": "1"})
+
 
 @pytest.fixture
 def install_slow_fail_kernel():
-    return _install_kernel("problemtest-slow", extra_env={
-        "STARTUP_DELAY": "5",
-        "FAIL_ON_START": "1"
-    })
+    return _install_kernel(
+        "problemtest-slow", extra_env={"STARTUP_DELAY": "5", "FAIL_ON_START": "1"}
+    )
+
 
 @pytest.fixture(params=["tcp", "ipc"])
 def transport(request):
     if sys.platform == "win32" and request.param == "ipc":  #
         pytest.skip("Transport 'ipc' not supported on Windows.")
     return request.param
 
+
 @pytest.fixture
 def config(transport):
     c = Config()
@@ -77,6 +73,7 @@ def config(transport):
         c.KernelManager.ip = "test"
     return c
 
+
 @pytest.fixture
 def debug_logging():
     get_logger().setLevel("DEBUG")
@@ -93,6 +90,7 @@ async def test_restart_check(config, install_kernel):
 
     cbs = 0
     restarts = [asyncio.Future() for i in range(N_restarts)]
+
     def cb():
         nonlocal cbs
         if cbs >= N_restarts:
@@ -103,7 +101,7 @@ def cb():
     try:
         km.start_kernel()
         km.add_restart_callback(cb, 'restart')
-    except:
+    except BaseException:
         if km.has_kernel:
             km.shutdown_kernel()
         raise
@@ -127,6 +125,7 @@ def cb():
         km.shutdown_kernel(now=True)
         assert km.context.closed
 
+
 @pytest.mark.asyncio
 async def test_restarter_gives_up(config, install_fail_kernel):
     """Test that the restarter gives up after reaching the restart limit"""
@@ -138,6 +137,7 @@ async def test_restarter_gives_up(config, install_fail_kernel):
 
     cbs = 0
     restarts = [asyncio.Future() for i in range(N_restarts)]
+
     def cb():
         nonlocal cbs
         if cbs >= N_restarts:
@@ -146,14 +146,15 @@ def cb():
         cbs += 1
 
     died = asyncio.Future()
+
     def on_death():
         died.set_result(True)
 
     try:
         km.start_kernel()
         km.add_restart_callback(cb, 'restart')
         km.add_restart_callback(on_death, 'dead')
-    except:
+    except BaseException:
         if km.has_kernel:
             km.shutdown_kernel()
         raise
@@ -182,6 +183,7 @@ async def test_async_restart_check(config, install_kernel):
 
     cbs = 0
     restarts = [asyncio.Future() for i in range(N_restarts)]
+
     def cb():
         nonlocal cbs
         if cbs >= N_restarts:
@@ -192,7 +194,7 @@ def cb():
     try:
         await km.start_kernel()
         km.add_restart_callback(cb, 'restart')
-    except:
+    except BaseException:
         if km.has_kernel:
             await km.shutdown_kernel()
         raise
@@ -216,18 +218,20 @@ def cb():
         await km.shutdown_kernel(now=True)
         assert km.context.closed
 
+
 @pytest.mark.asyncio
 async def test_async_restarter_gives_up(config, install_slow_fail_kernel):
     """Test that the restarter gives up after reaching the restart limit"""
     # If this test failes, run it with --log-cli-level=DEBUG to inspect
     N_restarts = 2
     config.KernelRestarter.restart_limit = N_restarts
     config.KernelRestarter.debug = True
-    config.KernelRestarter.stable_start_time = 30.
+    config.KernelRestarter.stable_start_time = 30.0
     km = AsyncIOLoopKernelManager(kernel_name=install_slow_fail_kernel, config=config)
 
     cbs = 0
     restarts = [asyncio.Future() for i in range(N_restarts)]
+
     def cb():
         nonlocal cbs
         if cbs >= N_restarts:
@@ -236,14 +240,15 @@ def cb():
         cbs += 1
 
     died = asyncio.Future()
+
     def on_death():
         died.set_result(True)
 
     try:
         await km.start_kernel()
         km.add_restart_callback(cb, 'restart')
         km.add_restart_callback(on_death, 'dead')
-    except:
+    except BaseException:
         if km.has_kernel:
             await km.shutdown_kernel()
         raise
@@ -258,4 +263,3 @@ def on_death():
 
         await km.shutdown_kernel(now=True)
         assert km.context.closed
-