From cbbead37809a4f995cdac5f78978d74889e3a343 Mon Sep 17 00:00:00 2001 From: GeorgianaElena Date: Wed, 14 Aug 2019 13:21:13 +0300 Subject: [PATCH 1/4] Fix uncaught exception in pre_spawn_start --- jupyterhub/handlers/pages.py | 14 +++++++++++++- jupyterhub/user.py | 22 ++++++++++++---------- 2 files changed, 25 insertions(+), 11 deletions(-) diff --git a/jupyterhub/handlers/pages.py b/jupyterhub/handlers/pages.py index 2aa2c3e8..68264afc 100644 --- a/jupyterhub/handlers/pages.py +++ b/jupyterhub/handlers/pages.py @@ -172,7 +172,19 @@ class SpawnHandler(BaseHandler): spawner._spawn_future = None # not running, no form. Trigger spawn and redirect back to /user/:name f = asyncio.ensure_future(self.spawn_single_user(user, server_name)) - await asyncio.wait([f], timeout=1) + done, pending = await asyncio.wait([f], timeout=1) + # If spawn_single_user throws an exception, raise a 500 error + # otherwise it may cause a redirect loop + if f in done: + future, = done + exc = future.exception() + if exc: + raise web.HTTPError( + 500, + "Error in Authenticator.pre_spawn_start: %s %s" + % (type(exc).__name__, str(exc)), + ) + return self.redirect(pending_url) @web.authenticated diff --git a/jupyterhub/user.py b/jupyterhub/user.py index 97a3543f..407310db 100644 --- a/jupyterhub/user.py +++ b/jupyterhub/user.py @@ -529,17 +529,19 @@ class User: # trigger pre-spawn hook on authenticator authenticator = self.authenticator - if authenticator: - await maybe_future(authenticator.pre_spawn_start(self, spawner)) - - spawner._start_pending = True - # update spawner start time, and activity for both spawner and user - self.last_activity = ( - spawner.orm_spawner.started - ) = spawner.orm_spawner.last_activity = datetime.utcnow() - db.commit() - # wait for spawner.start to return try: + if authenticator: + # pre_spawn_start can thow errors that can lead to a redirect loop + # if left uncaught (see https://github.com/jupyterhub/jupyterhub/issues/2683) + await maybe_future(authenticator.pre_spawn_start(self, spawner)) + + spawner._start_pending = True + # update spawner start time, and activity for both spawner and user + self.last_activity = ( + spawner.orm_spawner.started + ) = spawner.orm_spawner.last_activity = datetime.utcnow() + db.commit() + # wait for spawner.start to return # run optional preparation work to bootstrap the notebook await maybe_future(spawner.run_pre_spawn_hook()) if self.settings.get('internal_ssl'): From 8a61eb17383c0dd128e5718e9a20d5457ce00936 Mon Sep 17 00:00:00 2001 From: GeorgianaElena Date: Wed, 14 Aug 2019 13:21:40 +0300 Subject: [PATCH 2/4] Test pre_spawn_start exception --- jupyterhub/tests/test_pages.py | 39 ++++++++++++++++++++++++++++++++++ 1 file changed, 39 insertions(+) diff --git a/jupyterhub/tests/test_pages.py b/jupyterhub/tests/test_pages.py index 6bc5663d..77b5cf1e 100644 --- a/jupyterhub/tests/test_pages.py +++ b/jupyterhub/tests/test_pages.py @@ -790,3 +790,42 @@ async def test_metrics_auth(app): async def test_health_check_request(app): r = await get_page('health', app) assert r.status_code == 200 + + +async def test_pre_spawn_start_exc_no_form(app): + exc = "pre_spawn_start error" + + # throw exception from pre_spawn_start + @gen.coroutine + def mock_pre_spawn_start(user, spawner): + raise Exception(exc) + + with mock.patch.object(app.authenticator, 'pre_spawn_start', mock_pre_spawn_start): + cookies = await app.login_user('summer') + # spawn page should thow a 500 error and show the pre_spawn_start error message + r = await get_page('spawn', app, cookies=cookies) + assert r.status_code == 500 + assert exc in r.text + + +async def test_pre_spawn_start_exc_options_form(app): + exc = "pre_spawn_start error" + + # throw exception from pre_spawn_start + @gen.coroutine + def mock_pre_spawn_start(user, spawner): + raise Exception(exc) + + with mock.patch.dict( + app.users.settings, {'spawner_class': FormSpawner} + ), mock.patch.object(app.authenticator, 'pre_spawn_start', mock_pre_spawn_start): + cookies = await app.login_user('spring') + user = app.users['spring'] + # spawn page shouldn't throw any error until the spawn is started + r = await get_page('spawn', app, cookies=cookies) + assert r.url.endswith('/spawn') + r.raise_for_status() + assert FormSpawner.options_form in r.text + # spawning the user server should throw the pre_spawn_start error + with pytest.raises(Exception, match="%s" % exc): + await user.spawn() From 0058ed803d611d1d585f7d96b80c3b0586f64560 Mon Sep 17 00:00:00 2001 From: GeorgianaElena Date: Thu, 22 Aug 2019 15:06:08 +0300 Subject: [PATCH 3/4] Address feedback --- jupyterhub/handlers/pages.py | 6 ++---- 1 file changed, 2 insertions(+), 4 deletions(-) diff --git a/jupyterhub/handlers/pages.py b/jupyterhub/handlers/pages.py index 68264afc..53525dc8 100644 --- a/jupyterhub/handlers/pages.py +++ b/jupyterhub/handlers/pages.py @@ -175,16 +175,14 @@ class SpawnHandler(BaseHandler): done, pending = await asyncio.wait([f], timeout=1) # If spawn_single_user throws an exception, raise a 500 error # otherwise it may cause a redirect loop - if f in done: - future, = done - exc = future.exception() + if f.done() and f.exception(): + exc = f.exception() if exc: raise web.HTTPError( 500, "Error in Authenticator.pre_spawn_start: %s %s" % (type(exc).__name__, str(exc)), ) - return self.redirect(pending_url) @web.authenticated From 03693c379e19f3823c3bad7f60cb0a4927e183e0 Mon Sep 17 00:00:00 2001 From: GeorgianaElena Date: Thu, 22 Aug 2019 15:53:40 +0300 Subject: [PATCH 4/4] Removed unnecesary check --- jupyterhub/handlers/pages.py | 11 +++++------ 1 file changed, 5 insertions(+), 6 deletions(-) diff --git a/jupyterhub/handlers/pages.py b/jupyterhub/handlers/pages.py index 53525dc8..aa2f37cc 100644 --- a/jupyterhub/handlers/pages.py +++ b/jupyterhub/handlers/pages.py @@ -177,12 +177,11 @@ class SpawnHandler(BaseHandler): # otherwise it may cause a redirect loop if f.done() and f.exception(): exc = f.exception() - if exc: - raise web.HTTPError( - 500, - "Error in Authenticator.pre_spawn_start: %s %s" - % (type(exc).__name__, str(exc)), - ) + raise web.HTTPError( + 500, + "Error in Authenticator.pre_spawn_start: %s %s" + % (type(exc).__name__, str(exc)), + ) self.redirect(pending_url) @web.authenticated