Repository: jupyter/jupyter_client (python)

Issue:
get_all_specs not compatible with subclassing
[nb_conda_kernels](https://github.com/Anaconda-Platform/nb_conda_kernels) subclasses KernelSpecManager to find conda kernels. It does so by overriding `find_kernel_specs` to return its name list and resource directories, and `get_kernel_spec` to return the specs themselves.

We've added `.get_all_specs` as a simpler API to get all of the specs without separately finding and loading them. But we've done this in such a way that `.get_all_specs` calls `.find_kernel_specs`, a public API, and `_get_kernel_spec_by_name`, a private method. This means that implementations that override `.find_kernel_specs` in such a way that `_get_kernel_spec_by_name` won't work, then `.get_all_specs` is broken for any such subclass (this includes nb_conda_kernels 2.1).

Now, one option is for custom kernelspec managers to implement all three methods - find_kernel_specs, get_all_specs, and get_kernel_spec, but this is a backward-incompatible change, and I think we should consider it a bug.

I think we should ensure that `.get_all_specs` works for KSM subclasses that have only implemented the public methods `.find_kernel_specs` and `.get_kernel_spec` to fix this backward-incompatibility, and push that out in a bugfix release.

cc @dartdog who reported the issue

Pull request:
avoid calling private method in subclasses of KernelSpecManager
on the result of a public overrideable method, which breaks subclasses that don't override get_all_specs

rather than falling back on the public method, which is slower because it looks up the location of a directory we have already found, reserve this optimization for the base class itself, which is the only place this is a safe method to call.

closes #338

I think we should ship a 5.2.2 bugfix upon merging this one and confirming it fixes nb_conda_kernels

Patch:
diff --git a/jupyter_client/channels.py b/jupyter_client/channels.py
index 64e5651..dd99067 100644
--- a/jupyter_client/channels.py
+++ b/jupyter_client/channels.py
@@ -80,10 +80,7 @@ class HBChannel(Thread):
     @staticmethod
     @atexit.register
     def _notice_exit():
-        # Class definitions can be torn down during interpreter shutdown.
-        # We only need to set _exiting flag if this hasn't happened.
-        if HBChannel is not None:
-            HBChannel._exiting = True
+        HBChannel._exiting = True
 
     def _create_socket(self):
         if self.socket is not None:
diff --git a/jupyter_client/kernelspec.py b/jupyter_client/kernelspec.py
index 3e05d29..78a5b56 100644
--- a/jupyter_client/kernelspec.py
+++ b/jupyter_client/kernelspec.py
@@ -254,7 +254,14 @@ class KernelSpecManager(LoggingConfigurable):
         res = {}
         for kname, resource_dir in d.items():
             try:
-                spec = self._get_kernel_spec_by_name(kname, resource_dir)
+                if self.__class__ is KernelSpecManager:
+                    spec = self._get_kernel_spec_by_name(kname, resource_dir)
+                else:
+                    # avoid calling private methods in subclasses,
+                    # which may have overridden find_kernel_specs
+                    # and get_kernel_spec, but not the newer get_all_specs
+                    spec = self.get_kernel_spec(kname)
+
                 res[kname] = {
                     "resource_dir": resource_dir,
                     "spec": spec.to_dict()
diff --git a/jupyter_client/threaded.py b/jupyter_client/threaded.py
index fda3a08..f437aa5 100644
--- a/jupyter_client/threaded.py
+++ b/jupyter_client/threaded.py
@@ -151,10 +151,7 @@ class IOLoopThread(Thread):
     @staticmethod
     @atexit.register
     def _notice_exit():
-        # Class definitions can be torn down during interpreter shutdown.
-        # We only need to set _exiting flag if this hasn't happened.
-        if IOLoopThread is not None:
-            IOLoopThread._exiting = True
+        IOLoopThread._exiting = True
 
     def run(self):
         """Run my loop, ignoring EINTR events in the poller"""


Test patch:
diff --git a/jupyter_client/tests/test_kernelspec.py b/jupyter_client/tests/test_kernelspec.py
index b2ec419..2919923 100644
--- a/jupyter_client/tests/test_kernelspec.py
+++ b/jupyter_client/tests/test_kernelspec.py
@@ -4,6 +4,7 @@
 # Copyright (c) Jupyter Development Team.
 # Distributed under the terms of the Modified BSD License.
 
+import copy
 import io
 import json
 from logging import StreamHandler
@@ -11,6 +12,7 @@ import os
 from os.path import join as pjoin
 from subprocess import Popen, PIPE, STDOUT
 import sys
+import tempfile
 import unittest
 
 import pytest
@@ -156,7 +158,7 @@ class KernelSpecTests(unittest.TestCase):
             'Haskell-1-2-3',
         ]:
             assert kernelspec._is_valid_kernel_name(good)
-        
+
         for bad in [
             'has space',
             u'ünicode',
@@ -165,4 +167,33 @@ class KernelSpecTests(unittest.TestCase):
         ]:
             assert not kernelspec._is_valid_kernel_name(bad)
 
-
+    def test_subclass(self):
+        """Test get_all_specs in subclasses that override find_kernel_specs"""
+        ksm = self.ksm
+        resource_dir = tempfile.gettempdir()
+        native_name = kernelspec.NATIVE_KERNEL_NAME
+        native_kernel = ksm.get_kernel_spec(native_name)
+
+        class MyKSM(kernelspec.KernelSpecManager):
+            def get_kernel_spec(self, name):
+                spec = copy.copy(native_kernel)
+                if name == 'fake':
+                    spec.name = name
+                    spec.resource_dir = resource_dir
+                elif name == native_name:
+                    pass
+                else:
+                    raise KeyError(name)
+                return spec
+
+            def find_kernel_specs(self):
+                return {
+                    'fake': resource_dir,
+                    native_name: native_kernel.resource_dir,
+                }
+
+        # ensure that get_all_specs doesn't raise if only
+        # find_kernel_specs and get_kernel_spec are defined
+        myksm = MyKSM()
+        specs = myksm.get_all_specs()
+        assert sorted(specs) == ['fake', native_name]