Merge pull request #6412 from takluyver/sessions-rest-api-fix

Sessions rest api fix

IPython/html/services/sessions/sessionmanager.py

@@ -1,206 +1,227 b''
 """A base class session manager.
 
 Authors:
 
 * Zach Sailer
 """
 
 #-----------------------------------------------------------------------------
 #  Copyright (C) 2013  The IPython Development Team
 #
 #  Distributed under the terms of the BSD License.  The full license is in
 #  the file COPYING, distributed as part of this software.
 #-----------------------------------------------------------------------------
 
 #-----------------------------------------------------------------------------
 # Imports
 #-----------------------------------------------------------------------------
 
 import uuid
 import sqlite3
 
 from tornado import web
 
 from IPython.config.configurable import LoggingConfigurable
 from IPython.utils.py3compat import unicode_type
 from IPython.utils.traitlets import Instance
 
 #-----------------------------------------------------------------------------
 # Classes
 #-----------------------------------------------------------------------------
 
 class SessionManager(LoggingConfigurable):
 
     kernel_manager = Instance('IPython.html.services.kernels.kernelmanager.MappingKernelManager')
     contents_manager = Instance('IPython.html.services.contents.manager.ContentsManager', args=())
     
     # Session database initialized below
     _cursor = None
     _connection = None
     _columns = {'session_id', 'name', 'path', 'kernel_id'}
     
     @property
     def cursor(self):
         """Start a cursor and create a database called 'session'"""
         if self._cursor is None:
             self._cursor = self.connection.cursor()
             self._cursor.execute("""CREATE TABLE session 
                 (session_id, name, path, kernel_id)""")
         return self._cursor
 
     @property
     def connection(self):
         """Start a database connection"""
         if self._connection is None:
             self._connection = sqlite3.connect(':memory:')
-            self._connection.row_factory = self.row_factory
+            self._connection.row_factory = sqlite3.Row
         return self._connection
         
     def __del__(self):
         """Close connection once SessionManager closes"""
         self.cursor.close()
 
     def session_exists(self, name, path):
         """Check to see if the session for a given notebook exists"""
         self.cursor.execute("SELECT * FROM session WHERE name=? AND path=?", (name, path))
         reply = self.cursor.fetchone()
         if reply is None:
             return False
         else:
             return True
 
     def new_session_id(self):
         "Create a uuid for a new session"
         return unicode_type(uuid.uuid4())
 
     def create_session(self, name=None, path=None, kernel_name='python'):
         """Creates a session and returns its model"""
         session_id = self.new_session_id()
         # allow nbm to specify kernels cwd
         kernel_path = self.contents_manager.get_kernel_path(name=name, path=path)
         kernel_id = self.kernel_manager.start_kernel(path=kernel_path,
                                                      kernel_name=kernel_name)
         return self.save_session(session_id, name=name, path=path,
                                  kernel_id=kernel_id)
 
     def save_session(self, session_id, name=None, path=None, kernel_id=None):
         """Saves the items for the session with the given session_id
         
         Given a session_id (and any other of the arguments), this method
         creates a row in the sqlite session database that holds the information
         for a session.
         
         Parameters
         ----------
         session_id : str
             uuid for the session; this method must be given a session_id
         name : str
             the .ipynb notebook name that started the session
         path : str
             the path to the named notebook
         kernel_id : str
             a uuid for the kernel associated with this session
         
         Returns
         -------
         model : dict
             a dictionary of the session model
         """
         self.cursor.execute("INSERT INTO session VALUES (?,?,?,?)",
             (session_id, name, path, kernel_id)
         )
         return self.get_session(session_id=session_id)
 
     def get_session(self, **kwargs):
         """Returns the model for a particular session.
         
         Takes a keyword argument and searches for the value in the session
         database, then returns the rest of the session's info.
 
         Parameters
         ----------
         **kwargs : keyword argument
             must be given one of the keywords and values from the session database
             (i.e. session_id, name, path, kernel_id)
 
         Returns
         -------
         model : dict
             returns a dictionary that includes all the information from the 
             session described by the kwarg.
         """
         if not kwargs:
             raise TypeError("must specify a column to query")
 
         conditions = []
         for column in kwargs.keys():
             if column not in self._columns:
                 raise TypeError("No such column: %r", column)
             conditions.append("%s=?" % column)
 
         query = "SELECT * FROM session WHERE %s" % (' AND '.join(conditions))
 
         self.cursor.execute(query, list(kwargs.values()))
-        model = self.cursor.fetchone()
-        if model is None:
+        try:
+            row = self.cursor.fetchone()
+        except KeyError:
+            # The kernel is missing, so the session just got deleted.
+            row = None
+
+        if row is None:
             q = []
             for key, value in kwargs.items():
                 q.append("%s=%r" % (key, value))
 
             raise web.HTTPError(404, u'Session not found: %s' % (', '.join(q)))
-        return model
+
+        return self.row_to_model(row)
 
     def update_session(self, session_id, **kwargs):
         """Updates the values in the session database.
         
         Changes the values of the session with the given session_id
         with the values from the keyword arguments. 
         
         Parameters
         ----------
         session_id : str
             a uuid that identifies a session in the sqlite3 database
         **kwargs : str
             the key must correspond to a column title in session database,
             and the value replaces the current value in the session 
             with session_id.
         """
         self.get_session(session_id=session_id)
 
         if not kwargs:
             # no changes
             return
 
         sets = []
         for column in kwargs.keys():
             if column not in self._columns:
                 raise TypeError("No such column: %r" % column)
             sets.append("%s=?" % column)
         query = "UPDATE session SET %s WHERE session_id=?" % (', '.join(sets))
         self.cursor.execute(query, list(kwargs.values()) + [session_id])
 
-    def row_factory(self, cursor, row):
+    def row_to_model(self, row):
         """Takes sqlite database session row and turns it into a dictionary"""
-        row = sqlite3.Row(cursor, row)
+        if row['kernel_id'] not in self.kernel_manager:
+            # The kernel was killed or died without deleting the session.
+            # We can't use delete_session here because that tries to find
+            # and shut down the kernel.
+            self.cursor.execute("DELETE FROM session WHERE session_id=?", 
+                                (row['session_id'],))
+            raise KeyError
+
         model = {
             'id': row['session_id'],
             'notebook': {
                 'name': row['name'],
                 'path': row['path']
             },
             'kernel': self.kernel_manager.kernel_model(row['kernel_id'])
         }
         return model
 
     def list_sessions(self):
         """Returns a list of dictionaries containing all the information from
         the session database"""
         c = self.cursor.execute("SELECT * FROM session")
-        return list(c.fetchall())
+        result = []
+        # We need to use fetchall() here, because row_to_model can delete rows,
+        # which messes up the cursor if we're iterating over rows.
+        for row in c.fetchall():
+            try:
+                result.append(self.row_to_model(row))
+            except KeyError:
+                pass
+        return result
 
     def delete_session(self, session_id):
         """Deletes the row in the session database with given session_id"""
         # Check that session exists before deleting
         session = self.get_session(session_id=session_id)
         self.kernel_manager.shutdown_kernel(session['kernel']['id'])
         self.cursor.execute("DELETE FROM session WHERE session_id=?", (session_id,))

IPython/html/services/sessions/tests/test_sessionmanager.py

@@ -1,105 +1,140 b''
 """Tests for the session manager."""
 
 from unittest import TestCase
 
 from tornado import web
 
 from ..sessionmanager import SessionManager
 from IPython.html.services.kernels.kernelmanager import MappingKernelManager
 
 class DummyKernel(object):
     def __init__(self, kernel_name='python'):
         self.kernel_name = kernel_name
 
 class DummyMKM(MappingKernelManager):
     """MappingKernelManager interface that doesn't start kernels, for testing"""
     def __init__(self, *args, **kwargs):
         super(DummyMKM, self).__init__(*args, **kwargs)
         self.id_letters = iter(u'ABCDEFGHIJK')
 
     def _new_id(self):
         return next(self.id_letters)
     
     def start_kernel(self, kernel_id=None, path=None, kernel_name='python', **kwargs):
         kernel_id = kernel_id or self._new_id()
         self._kernels[kernel_id] = DummyKernel(kernel_name=kernel_name)
         return kernel_id
 
     def shutdown_kernel(self, kernel_id, now=False):
         del self._kernels[kernel_id]
 
 class TestSessionManager(TestCase):
     
     def test_get_session(self):
         sm = SessionManager(kernel_manager=DummyMKM())
         session_id = sm.create_session(name='test.ipynb', path='/path/to/',
                                        kernel_name='bar')['id']
         model = sm.get_session(session_id=session_id)
         expected = {'id':session_id,
                     'notebook':{'name':u'test.ipynb', 'path': u'/path/to/'},
                     'kernel': {'id':u'A', 'name': 'bar'}}
         self.assertEqual(model, expected)
 
     def test_bad_get_session(self):
         # Should raise error if a bad key is passed to the database.
         sm = SessionManager(kernel_manager=DummyMKM())
         session_id = sm.create_session(name='test.ipynb', path='/path/to/',
                                        kernel_name='foo')['id']
         self.assertRaises(TypeError, sm.get_session, bad_id=session_id) # Bad keyword
 
+    def test_get_session_dead_kernel(self):
+        sm = SessionManager(kernel_manager=DummyMKM())
+        session = sm.create_session(name='test1.ipynb', path='/path/to/1/', kernel_name='python')
+        # kill the kernel
+        sm.kernel_manager.shutdown_kernel(session['kernel']['id'])
+        with self.assertRaises(KeyError):
+            sm.get_session(session_id=session['id'])
+        # no sessions left
+        listed = sm.list_sessions()
+        self.assertEqual(listed, [])
+
     def test_list_sessions(self):
         sm = SessionManager(kernel_manager=DummyMKM())
         sessions = [
             sm.create_session(name='test1.ipynb', path='/path/to/1/', kernel_name='python'),
             sm.create_session(name='test2.ipynb', path='/path/to/2/', kernel_name='python'),
             sm.create_session(name='test3.ipynb', path='/path/to/3/', kernel_name='python'),
         ]
         sessions = sm.list_sessions()
         expected = [{'id':sessions[0]['id'], 'notebook':{'name':u'test1.ipynb', 
                     'path': u'/path/to/1/'}, 'kernel':{'id':u'A', 'name':'python'}},
                     {'id':sessions[1]['id'], 'notebook': {'name':u'test2.ipynb', 
                     'path': u'/path/to/2/'}, 'kernel':{'id':u'B', 'name':'python'}},
                     {'id':sessions[2]['id'], 'notebook':{'name':u'test3.ipynb', 
                     'path': u'/path/to/3/'}, 'kernel':{'id':u'C', 'name':'python'}}]
         self.assertEqual(sessions, expected)
 
+    def test_list_sessions_dead_kernel(self):
+        sm = SessionManager(kernel_manager=DummyMKM())
+        sessions = [
+            sm.create_session(name='test1.ipynb', path='/path/to/1/', kernel_name='python'),
+            sm.create_session(name='test2.ipynb', path='/path/to/2/', kernel_name='python'),
+        ]
+        # kill one of the kernels
+        sm.kernel_manager.shutdown_kernel(sessions[0]['kernel']['id'])
+        listed = sm.list_sessions()
+        expected = [
+            {
+                'id': sessions[1]['id'],
+                'notebook': {
+                    'name': u'test2.ipynb',
+                    'path': u'/path/to/2/',
+                },
+                'kernel': {
+                    'id': u'B',
+                    'name':'python',
+                }
+            }
+        ]
+        self.assertEqual(listed, expected)
+
     def test_update_session(self):
         sm = SessionManager(kernel_manager=DummyMKM())
         session_id = sm.create_session(name='test.ipynb', path='/path/to/',
                                        kernel_name='julia')['id']
         sm.update_session(session_id, name='new_name.ipynb')
         model = sm.get_session(session_id=session_id)
         expected = {'id':session_id,
                     'notebook':{'name':u'new_name.ipynb', 'path': u'/path/to/'},
                     'kernel':{'id':u'A', 'name':'julia'}}
         self.assertEqual(model, expected)
     
     def test_bad_update_session(self):
         # try to update a session with a bad keyword ~ raise error
         sm = SessionManager(kernel_manager=DummyMKM())
         session_id = sm.create_session(name='test.ipynb', path='/path/to/',
                                        kernel_name='ir')['id']
         self.assertRaises(TypeError, sm.update_session, session_id=session_id, bad_kw='test.ipynb') # Bad keyword
 
     def test_delete_session(self):
         sm = SessionManager(kernel_manager=DummyMKM())
         sessions = [
             sm.create_session(name='test1.ipynb', path='/path/to/1/', kernel_name='python'),
             sm.create_session(name='test2.ipynb', path='/path/to/2/', kernel_name='python'),
             sm.create_session(name='test3.ipynb', path='/path/to/3/', kernel_name='python'),
         ]
         sm.delete_session(sessions[1]['id'])
         new_sessions = sm.list_sessions()
         expected = [{'id':sessions[0]['id'], 'notebook':{'name':u'test1.ipynb', 
                     'path': u'/path/to/1/'}, 'kernel':{'id':u'A', 'name':'python'}},
                     {'id':sessions[2]['id'], 'notebook':{'name':u'test3.ipynb', 
                     'path': u'/path/to/3/'}, 'kernel':{'id':u'C', 'name':'python'}}]
         self.assertEqual(new_sessions, expected)
 
     def test_bad_delete_session(self):
         # try to delete a session that doesn't exist ~ raise error
         sm = SessionManager(kernel_manager=DummyMKM())
         sm.create_session(name='test.ipynb', path='/path/to/', kernel_name='python')
         self.assertRaises(TypeError, sm.delete_session, bad_kwarg='23424') # Bad keyword
         self.assertRaises(web.HTTPError, sm.delete_session, session_id='23424') # nonexistant
 

IPython/html/tests/launchnotebook.py

@@ -1,102 +1,102 b''
 """Base class for notebook tests."""
 
 from __future__ import print_function
 
 import sys
 import time
 import requests
 from contextlib import contextmanager
 from subprocess import Popen, STDOUT
 from unittest import TestCase
 
 import nose
 
 from IPython.utils.tempdir import TemporaryDirectory
 
 MAX_WAITTIME = 30   # seconds to wait for notebook server to start
 POLL_INTERVAL = 0.1 # time between attempts
 
 # TimeoutError is a builtin on Python 3. This can be removed when we stop
 # supporting Python 2.
 class TimeoutError(Exception):
     pass
 
 class NotebookTestBase(TestCase):
     """A base class for tests that need a running notebook.
     
     This creates an empty profile in a temp ipython_dir
     and then starts the notebook server with a separate temp notebook_dir.
     """
 
     port = 12341
 
     @classmethod
     def wait_until_alive(cls):
         """Wait for the server to be alive"""
         url = 'http://localhost:%i/api/contents' % cls.port
         for _ in range(int(MAX_WAITTIME/POLL_INTERVAL)):
             try:
                 requests.get(url)
             except Exception as e:
                 if cls.notebook.poll() is not None:
                     raise RuntimeError("The notebook server exited with status %s" \
                                         % cls.notebook.poll())
                 time.sleep(POLL_INTERVAL)
             else:
                 return
 
         raise TimeoutError("The notebook server didn't start up correctly.")
     
     @classmethod
     def wait_until_dead(cls):
         """Wait for the server process to terminate after shutdown"""
         for _ in range(int(MAX_WAITTIME/POLL_INTERVAL)):
             if cls.notebook.poll() is not None:
                 return
             time.sleep(POLL_INTERVAL)
     
         raise TimeoutError("Undead notebook server")
 
     @classmethod
     def setup_class(cls):
         cls.ipython_dir = TemporaryDirectory()
         cls.notebook_dir = TemporaryDirectory()
         notebook_args = [
             sys.executable, '-c',
             'from IPython.html.notebookapp import launch_new_instance; launch_new_instance()',
             '--port=%d' % cls.port,
             '--port-retries=0',  # Don't try any other ports
             '--no-browser',
             '--ipython-dir=%s' % cls.ipython_dir.name,
             '--notebook-dir=%s' % cls.notebook_dir.name,
         ]
         cls.notebook = Popen(notebook_args,
             stdout=nose.iptest_stdstreams_fileno(),
             stderr=STDOUT,
         )
         cls.wait_until_alive()
 
     @classmethod
     def teardown_class(cls):
         cls.notebook.terminate()
         cls.wait_until_dead()
         cls.ipython_dir.cleanup()
         cls.notebook_dir.cleanup()
 
     @classmethod
     def base_url(cls):
         return 'http://localhost:%i/' % cls.port
 
 
 @contextmanager
 def assert_http_error(status, msg=None):
     try:
         yield
     except requests.HTTPError as e:
         real_status = e.response.status_code
         assert real_status == status, \
-                    "Expected status %d, got %d" % (real_status, status)
+                    "Expected status %d, got %d" % (status, real_status)
         if msg:
             assert msg in str(e), e
     else:
         assert False, "Expected HTTP error status"
  No newline at end of file