Merge pull request #2684 from GeorgianaElena/display_pre_spawn_start_exc

Handle pre_spawn_start possible exceptions

jupyterhub/handlers/pages.py

@@ -172,7 +172,16 @@ 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.done() and f.exception():
+                exc = f.exception()
+                raise web.HTTPError(
+                    500,
+                    "Error in Authenticator.pre_spawn_start: %s %s"
+                    % (type(exc).__name__, str(exc)),
+                )
             self.redirect(pending_url)
 
     @web.authenticated

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()

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'):