General Fixes for Comms

ipykernel/comm/comm.py

@@ -17,28 +17,20 @@
 
 class Comm(LoggingConfigurable):
     """Class for communicating between a Frontend and a Kernel"""
-    # If this is instantiated by a non-IPython kernel, shell will be None
-    shell = Instance('IPython.core.interactiveshell.InteractiveShellABC',
-                     allow_none=True)
     kernel = Instance('ipykernel.kernelbase.Kernel')
 
     @default('kernel')
     def _default_kernel(self):
         if Kernel.initialized():
             return Kernel.instance()
 
-    iopub_socket = Any()
-
-    @default('iopub_socket')
-    def _default_iopub_socket(self):
-        return self.kernel.iopub_socket
+    comm_id = Unicode()
 
-    session = Instance('jupyter_client.session.Session')
+    @default('comm_id')
+    def _default_comm_id(self):
+        return uuid.uuid4().hex
 
-    @default('session')
-    def _default_session(self):
-        if self.kernel is not None:
-            return self.kernel.session
+    primary = Bool(True, help="Am I the primary or secondary Comm?")
 
     target_name = Unicode('comm')
     target_module = Unicode(None, allow_none=True, help="""requirejs module from
@@ -57,13 +49,6 @@ def _default_topic(self):
     _close_callback = Any()
 
     _closed = Bool(True)
-    comm_id = Unicode()
-
-    @default('comm_id')
-    def _default_comm_id(self):
-        return uuid.uuid4().hex
-
-    primary = Bool(True, help="Am I the primary or secondary Comm?")
 
     def __init__(self, target_name='', data=None, metadata=None, buffers=None, **kwargs):
         if target_name:
@@ -84,7 +69,7 @@ def _publish_msg(self, msg_type, data=None, metadata=None, buffers=None, **keys)
         data = {} if data is None else data
         metadata = {} if metadata is None else metadata
         content = json_clean(dict(data=data, comm_id=self.comm_id, **keys))
-        self.session.send(self.iopub_socket, msg_type,
+        self.kernel.session.send(self.kernel.iopub_socket, msg_type,
             content,
             metadata=json_clean(metadata),
             parent=self.kernel._parent_header,
@@ -170,11 +155,12 @@ def handle_msg(self, msg):
         """Handle a comm_msg message"""
         self.log.debug("handle_msg[%s](%s)", self.comm_id, msg)
         if self._msg_callback:
-            if self.shell:
-                self.shell.events.trigger('pre_execute')
+            shell = self.kernel.shell
+            if shell:
+                shell.events.trigger('pre_execute')
             self._msg_callback(msg)
-            if self.shell:
-                self.shell.events.trigger('post_execute')
+            if shell:
+                shell.events.trigger('post_execute')
 
 
 __all__ = ['Comm']

ipykernel/comm/manager.py

@@ -6,8 +6,6 @@
 import sys
 
 from traitlets.config import LoggingConfigurable
-from IPython.core.prompts import LazyEvaluate
-from IPython.core.getipython import get_ipython
 
 from ipython_genutils.importstring import import_item
 from ipython_genutils.py3compat import string_types
@@ -16,35 +14,10 @@
 from .comm import Comm
 
 
-def lazy_keys(dikt):
-    """Return lazy-evaluated string representation of a dictionary's keys
-
-    Key list is only constructed if it will actually be used.
-    Used for debug-logging.
-    """
-    return LazyEvaluate(lambda d: list(d.keys()))
-
-
 class CommManager(LoggingConfigurable):
     """Manager for Comms in the Kernel"""
 
-    # If this is instantiated by a non-IPython kernel, shell will be None
-    shell = Instance('IPython.core.interactiveshell.InteractiveShellABC',
-                     allow_none=True)
     kernel = Instance('ipykernel.kernelbase.Kernel')
-
-    iopub_socket = Any()
-
-    @default('iopub_socket')
-    def _default_iopub_socket(self):
-        return self.kernel.iopub_socket
-
-    session = Instance('jupyter_client.session.Session')
-
-    @default('session')
-    def _default_session(self):
-        return self.kernel.session
-
     comms = Dict()
     targets = Dict()
 
@@ -67,14 +40,12 @@ def register_target(self, target_name, f):
 
     def unregister_target(self, target_name, f):
         """Unregister a callable registered with register_target"""
-        return self.targets.pop(target_name);
+        return self.targets.pop(target_name)
 
     def register_comm(self, comm):
         """Register a new comm"""
         comm_id = comm.comm_id
-        comm.shell = self.shell
         comm.kernel = self.kernel
-        comm.iopub_socket = self.iopub_socket
         self.comms[comm_id] = comm
         return comm_id
 
@@ -91,13 +62,12 @@ def get_comm(self, comm_id):
         This will not raise an error,
         it will log messages if the comm cannot be found.
         """
-        if comm_id not in self.comms:
+        try:
+            return self.comms[comm_id]
+        except KeyError:
             self.log.warn("No such comm: %s", comm_id)
-            self.log.debug("Current comms: %s", lazy_keys(self.comms))
-            return
-        # call, because we store weakrefs
-        comm = self.comms[comm_id]
-        return comm
+            if self.log.isEnabledFor(self.log.DEBUG):
+                self.log.debug("Current comms: %s", self.comms.keys())
 
     # Message handlers
     def comm_open(self, stream, ident, msg):
@@ -107,9 +77,6 @@ def comm_open(self, stream, ident, msg):
         target_name = content['target_name']
         f = self.targets.get(target_name, None)
         comm = Comm(comm_id=comm_id,
-                    shell=self.shell,
-                    kernel=self.kernel,
-                    iopub_socket=self.iopub_socket,
                     primary=False,
                     target_name=target_name,
         )
@@ -132,32 +99,37 @@ def comm_open(self, stream, ident, msg):
 
     def comm_msg(self, stream, ident, msg):
         """Handler for comm_msg messages"""
-        content = msg['content']
-        comm_id = content['comm_id']
-        comm = self.get_comm(comm_id)
-        if comm is None:
-            # no such comm
+        comm_id, comm = self._get_id_and_comm(msg)
+        if self._comm_is_not_valid(comm, comm_id):
             return
-        try:
-            comm.handle_msg(msg)
-        except Exception:
-            self.log.error("Exception in comm_msg for %s", comm_id, exc_info=True)
+
+        self._safe_handle(msg, comm.handle_msg)
 
     def comm_close(self, stream, ident, msg):
         """Handler for comm_close messages"""
-        content = msg['content']
-        comm_id = content['comm_id']
-        comm = self.get_comm(comm_id)
-        if comm is None:
-            # no such comm
-            self.log.debug("No such comm to close: %s", comm_id)
+        comm_id, comm = self._get_id_and_comm(msg)
+        if self._comm_is_not_valid(comm, comm_id):
             return
+
         del self.comms[comm_id]
+        self._safe_handle(msg, comm.handle_close)
+
+    def _comm_is_not_valid(self, comm, comm_id):
+        invalid = comm is None
+        if invalid:
+            self.log.debug('No such comm: ', str(comm_id))
+        return not invalid
+
+    def _get_id_and_comm(self, msg):
+        content = msg['content']
+        comm_id = content['comm_id']
+        return (comm_id, self.get_comm(comm_id))
 
+    def _safe_handle(self, msg, callback):
         try:
-            comm.handle_close(msg)
+            callback(msg)
         except Exception:
-            self.log.error("Exception handling comm_close for %s", comm_id, exc_info=True)
+            self.log.error('Exception handling ' + callback.__name__ + ' for %s', msg['content']['comm_id'], exc_info=True)
 
 
 __all__ = ['CommManager']

ipykernel/ipkernel.py

@@ -54,8 +54,7 @@ def __init__(self, **kwargs):
         # TMP - hack while developing
         self.shell._reply_content = None
 
-        self.comm_manager = CommManager(shell=self.shell, parent=self,
-                                        kernel=self)
+        self.comm_manager = CommManager(kernel=self)
 
         self.shell.configurables.append(self.comm_manager)
         comm_msg_types = [ 'comm_open', 'comm_msg', 'comm_close' ]
@@ -126,7 +125,7 @@ def set_parent(self, ident, parent):
 
     def init_metadata(self, parent):
         """Initialize metadata.
-        
+
         Run at the beginning of each execution request.
         """
         md = super(IPythonKernel, self).init_metadata(parent)
@@ -137,10 +136,10 @@ def init_metadata(self, parent):
             'engine' : self.ident,
         })
         return md
-    
+
     def finish_metadata(self, parent, metadata, reply_content):
         """Finish populating metadata.
-        
+
         Run after completing an execution request.
         """
         # FIXME: remove deprecated ipyparallel-specific code
@@ -359,7 +358,7 @@ def do_apply(self, content, bufs, msg_id, reply_metadata):
                 reply_content.update(shell._reply_content)
                 # reset after use
                 shell._reply_content = None
-                
+
                 # FIXME: deprecate piece for ipyparallel:
                 e_info = dict(engine_uuid=self.ident, engine_id=self.int_id, method='apply')
                 reply_content['engine_info'] = e_info