Add comment explaining choice of heuristic

Author: vidartf

jupyter_client/ioloop/restarter.py

@@ -85,6 +85,13 @@ async def poll(self):
                 await self.kernel_manager.restart_kernel(now=True, newports=newports)
                 self._restarting = True
         else:
+            # Since `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 "has been alive continuously for X time" as a
+            # heuristic for a stable start up.
+            # See https://github.com/jupyter/jupyter_client/pull/717 for details.
             if self._initial_startup and self._last_dead - now >= self.stable_start_time:
                 self._initial_startup = False
             if self._restarting and self._last_dead - now >= self.stable_start_time:

jupyter_client/restarter.py

@@ -143,6 +143,13 @@ def poll(self):
                 self.kernel_manager.restart_kernel(now=True, newports=newports)
                 self._restarting = True
         else:
+            # Since `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 "has been alive continuously for X time" as a
+            # heuristic for a stable start up.
+            # See https://github.com/jupyter/jupyter_client/pull/717 for details.
             if self._initial_startup and self._last_dead - now >= self.stable_start_time:
                 self._initial_startup = False
             if self._restarting and self._last_dead - now >= self.stable_start_time:

jupyter_client/tests/test_restarter.py

@@ -80,7 +80,7 @@ def debug_logging():
 
 
 @pytest.mark.asyncio
-async def test_restart_check(config, install_kernel):
+async def test_restart_check(config, install_kernel, debug_logging):
     """Test that the kernel is restarted and recovers"""
     # If this test failes, run it with --log-cli-level=DEBUG to inspect
     N_restarts = 1
@@ -116,6 +116,15 @@ def cb():
                 # Kill without cleanup to simulate crash:
                 await km.provisioner.kill()
                 await restarts[i]
+                # Wait for kill + restart
+                max_wait = 10.0
+                waited = 0.0
+                while waited < max_wait and km.is_alive():
+                    await asyncio.sleep(0.1)
+                    waited += 0.1
+                while waited < max_wait and not km.is_alive():
+                    await asyncio.sleep(0.1)
+                    waited += 0.1
 
         assert cbs == N_restarts
         assert km.is_alive()
@@ -127,7 +136,7 @@ def cb():
 
 
 @pytest.mark.asyncio
-async def test_restarter_gives_up(config, install_fail_kernel):
+async def test_restarter_gives_up(config, install_fail_kernel, debug_logging):
     """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 = 1
@@ -173,7 +182,7 @@ def on_death():
 
 
 @pytest.mark.asyncio
-async def test_async_restart_check(config, install_kernel):
+async def test_async_restart_check(config, install_kernel, debug_logging):
     """Test that the kernel is restarted and recovers"""
     # If this test failes, run it with --log-cli-level=DEBUG to inspect
     N_restarts = 1
@@ -209,6 +218,15 @@ def cb():
                 # Kill without cleanup to simulate crash:
                 await km.provisioner.kill()
                 await restarts[i]
+                # Wait for kill + restart
+                max_wait = 10.0
+                waited = 0.0
+                while waited < max_wait and await km.is_alive():
+                    await asyncio.sleep(0.1)
+                    waited += 0.1
+                while waited < max_wait and not await km.is_alive():
+                    await asyncio.sleep(0.1)
+                    waited += 0.1
 
         assert cbs == N_restarts
         assert await km.is_alive()
@@ -220,7 +238,7 @@ def cb():
 
 
 @pytest.mark.asyncio
-async def test_async_restarter_gives_up(config, install_slow_fail_kernel):
+async def test_async_restarter_gives_up(config, install_slow_fail_kernel, debug_logging):
     """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