From 76b05b24af40b8dde7ddc8b2177a9707221e7aa7 Mon Sep 17 00:00:00 2001 From: Thomas Kluyver Date: Thu, 3 Jul 2014 14:18:39 -0700 Subject: [PATCH 1/5] Handle sessions where the kernel has been killed --- .../html/services/sessions/sessionmanager.py | 23 +++++++++++++++---- 1 file changed, 19 insertions(+), 4 deletions(-) diff --git a/IPython/html/services/sessions/sessionmanager.py b/IPython/html/services/sessions/sessionmanager.py index 67adbb7c1..357595762 100644 --- a/IPython/html/services/sessions/sessionmanager.py +++ b/IPython/html/services/sessions/sessionmanager.py @@ -53,7 +53,6 @@ class SessionManager(LoggingConfigurable): """Start a database connection""" if self._connection is None: self._connection = sqlite3.connect(':memory:') - self._connection.row_factory = self.row_factory return self._connection def __del__(self): @@ -141,7 +140,12 @@ class SessionManager(LoggingConfigurable): query = "SELECT * FROM session WHERE %s" % (' AND '.join(conditions)) self.cursor.execute(query, list(kwargs.values())) - model = self.cursor.fetchone() + try: + model = self.row_to_model(self.cursor, self.cursor.fetchone()) + except KeyError: + # The kernel is missing, so the session just got deleted. + model = None + if model is None: q = [] for key, value in kwargs.items(): @@ -179,9 +183,14 @@ class SessionManager(LoggingConfigurable): 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, cursor, 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 without deleting the session. Should never occur. + self.delete_session(row['session_id']) + raise KeyError + model = { 'id': row['session_id'], 'notebook': { @@ -196,7 +205,13 @@ class SessionManager(LoggingConfigurable): """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 = [] + for row in c: + try: + result.append(self.row_to_model(c, row)) + except KeyError: + pass + return result def delete_session(self, session_id): """Deletes the row in the session database with given session_id""" From 4c4af2573403c272d4c3c9cf41c84cccea3711be Mon Sep 17 00:00:00 2001 From: Thomas Kluyver Date: Thu, 3 Jul 2014 14:29:43 -0700 Subject: [PATCH 2/5] Fix failure message for tests --- IPython/html/tests/launchnotebook.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/IPython/html/tests/launchnotebook.py b/IPython/html/tests/launchnotebook.py index e49fe80db..7775e2895 100644 --- a/IPython/html/tests/launchnotebook.py +++ b/IPython/html/tests/launchnotebook.py @@ -95,7 +95,7 @@ def assert_http_error(status, msg=None): 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: From ca8dabf70511f03eda207c62241bb82205455348 Mon Sep 17 00:00:00 2001 From: Thomas Kluyver Date: Thu, 3 Jul 2014 14:36:35 -0700 Subject: [PATCH 3/5] Fix 404 error when accessing nonexistant session --- IPython/html/services/sessions/sessionmanager.py | 15 ++++++++------- 1 file changed, 8 insertions(+), 7 deletions(-) diff --git a/IPython/html/services/sessions/sessionmanager.py b/IPython/html/services/sessions/sessionmanager.py index 357595762..2e579c376 100644 --- a/IPython/html/services/sessions/sessionmanager.py +++ b/IPython/html/services/sessions/sessionmanager.py @@ -53,6 +53,7 @@ class SessionManager(LoggingConfigurable): """Start a database connection""" if self._connection is None: self._connection = sqlite3.connect(':memory:') + self._connection.row_factory = sqlite3.Row return self._connection def __del__(self): @@ -141,18 +142,19 @@ class SessionManager(LoggingConfigurable): self.cursor.execute(query, list(kwargs.values())) try: - model = self.row_to_model(self.cursor, self.cursor.fetchone()) + row = self.cursor.fetchone() except KeyError: # The kernel is missing, so the session just got deleted. - model = None + row = None - if model is 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. @@ -183,9 +185,8 @@ class SessionManager(LoggingConfigurable): query = "UPDATE session SET %s WHERE session_id=?" % (', '.join(sets)) self.cursor.execute(query, list(kwargs.values()) + [session_id]) - def row_to_model(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 without deleting the session. Should never occur. self.delete_session(row['session_id']) @@ -208,7 +209,7 @@ class SessionManager(LoggingConfigurable): result = [] for row in c: try: - result.append(self.row_to_model(c, row)) + result.append(self.row_to_model(row)) except KeyError: pass return result From 0e109ee4bebfb6e324bb95aa1a2b03e0b62f3485 Mon Sep 17 00:00:00 2001 From: Thomas Kluyver Date: Fri, 5 Sep 2014 09:40:02 -0700 Subject: [PATCH 4/5] Correct comment --- IPython/html/services/sessions/sessionmanager.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/IPython/html/services/sessions/sessionmanager.py b/IPython/html/services/sessions/sessionmanager.py index 2e579c376..e347c055c 100644 --- a/IPython/html/services/sessions/sessionmanager.py +++ b/IPython/html/services/sessions/sessionmanager.py @@ -188,7 +188,7 @@ class SessionManager(LoggingConfigurable): def row_to_model(self, row): """Takes sqlite database session row and turns it into a dictionary""" if row['kernel_id'] not in self.kernel_manager: - # The kernel was killed without deleting the session. Should never occur. + # The kernel was killed or died without deleting the session. self.delete_session(row['session_id']) raise KeyError From b2737e668ebf565e615f52d2bd7e47ab824ee21d Mon Sep 17 00:00:00 2001 From: Thomas Kluyver Date: Fri, 5 Sep 2014 10:16:37 -0700 Subject: [PATCH 5/5] Add tests and fix some issues Tests taken from #6360 --- .../html/services/sessions/sessionmanager.py | 9 +++-- .../sessions/tests/test_sessionmanager.py | 35 +++++++++++++++++++ 2 files changed, 42 insertions(+), 2 deletions(-) diff --git a/IPython/html/services/sessions/sessionmanager.py b/IPython/html/services/sessions/sessionmanager.py index e347c055c..b105344c1 100644 --- a/IPython/html/services/sessions/sessionmanager.py +++ b/IPython/html/services/sessions/sessionmanager.py @@ -189,7 +189,10 @@ class SessionManager(LoggingConfigurable): """Takes sqlite database session row and turns it into a dictionary""" if row['kernel_id'] not in self.kernel_manager: # The kernel was killed or died without deleting the session. - self.delete_session(row['session_id']) + # 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 = { @@ -207,7 +210,9 @@ class SessionManager(LoggingConfigurable): the session database""" c = self.cursor.execute("SELECT * FROM session") result = [] - for row in c: + # 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: diff --git a/IPython/html/services/sessions/tests/test_sessionmanager.py b/IPython/html/services/sessions/tests/test_sessionmanager.py index ca080e77d..6383a0bad 100644 --- a/IPython/html/services/sessions/tests/test_sessionmanager.py +++ b/IPython/html/services/sessions/tests/test_sessionmanager.py @@ -47,6 +47,17 @@ class TestSessionManager(TestCase): 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 = [ @@ -63,6 +74,30 @@ class TestSessionManager(TestCase): '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/',