diff --git a/jupyterhub/apihandlers/users.py b/jupyterhub/apihandlers/users.py index 2e3cbae3..321a0851 100644 --- a/jupyterhub/apihandlers/users.py +++ b/jupyterhub/apihandlers/users.py @@ -515,7 +515,7 @@ class UserServerAPIHandler(APIHandler): user_name, self.named_server_limit_per_user ), ) - spawner = user.spawners[server_name] + spawner = user.get_spawner(server_name, replace_failed=True) pending = spawner.pending if pending == 'spawn': self.set_header('Content-Type', 'text/plain') diff --git a/jupyterhub/handlers/pages.py b/jupyterhub/handlers/pages.py index 3a063f62..cd311375 100644 --- a/jupyterhub/handlers/pages.py +++ b/jupyterhub/handlers/pages.py @@ -151,7 +151,7 @@ class SpawnHandler(BaseHandler): self.redirect(url) return - spawner = user.spawners[server_name] + spawner = user.get_spawner(server_name, replace_failed=True) pending_url = self._get_pending_url(user, server_name) @@ -237,7 +237,7 @@ class SpawnHandler(BaseHandler): if user is None: raise web.HTTPError(404, "No such user: %s" % for_user) - spawner = user.spawners[server_name] + spawner = user.get_spawner(server_name, replace_failed=True) if spawner.ready: raise web.HTTPError(400, "%s is already running" % (spawner._log_name)) @@ -369,13 +369,9 @@ class SpawnPendingHandler(BaseHandler): auth_state = await user.get_auth_state() # First, check for previous failure. - if ( - not spawner.active - and spawner._spawn_future - and spawner._spawn_future.done() - and spawner._spawn_future.exception() - ): - # Condition: spawner not active and _spawn_future exists and contains an Exception + if not spawner.active and spawner._failed: + # Condition: spawner not active and last spawn failed + # (failure is available as spawner._spawn_future.exception()). # Implicit spawn on /user/:name is not allowed if the user's last spawn failed. # We should point the user to Home if the most recent spawn failed. exc = spawner._spawn_future.exception() diff --git a/jupyterhub/tests/test_api.py b/jupyterhub/tests/test_api.py index cc63e78d..38ad4a5b 100644 --- a/jupyterhub/tests/test_api.py +++ b/jupyterhub/tests/test_api.py @@ -1030,7 +1030,7 @@ async def test_never_spawn(app, no_patience, never_spawn): assert not app_user.spawner._spawn_pending status = await app_user.spawner.poll() assert status is not None - # failed spawn should decrements pending count + # failed spawn should decrement pending count assert app.users.count_active_users()['pending'] == 0 @@ -1039,9 +1039,16 @@ async def test_bad_spawn(app, bad_spawn): name = 'prim' user = add_user(db, app=app, name=name) r = await api_request(app, 'users', name, 'server', method='post') + # check that we don't re-use spawners that failed + user.spawners[''].reused = True assert r.status_code == 500 assert app.users.count_active_users()['pending'] == 0 + r = await api_request(app, 'users', name, 'server', method='post') + # check that we don't re-use spawners that failed + spawner = user.spawners[''] + assert not getattr(spawner, 'reused', False) + async def test_spawn_nosuch_user(app): r = await api_request(app, 'users', "nosuchuser", 'server', method='post') diff --git a/jupyterhub/tests/test_pages.py b/jupyterhub/tests/test_pages.py index d5a84d4e..0edfd2db 100644 --- a/jupyterhub/tests/test_pages.py +++ b/jupyterhub/tests/test_pages.py @@ -128,11 +128,20 @@ async def test_admin_sort(app, sort): assert r.status_code == 200 -async def test_spawn_redirect(app): +@pytest.mark.parametrize("last_failed", [True, False]) +async def test_spawn_redirect(app, last_failed): name = 'wash' cookies = await app.login_user(name) u = app.users[orm.User.find(app.db, name)] + if last_failed: + # mock a failed spawn + last_spawner = u.spawners[''] + last_spawner._spawn_future = asyncio.Future() + last_spawner._spawn_future.set_exception(RuntimeError("I failed!")) + else: + last_spawner = None + status = await u.spawner.poll() assert status is not None @@ -141,6 +150,10 @@ async def test_spawn_redirect(app): r.raise_for_status() print(urlparse(r.url)) path = urlparse(r.url).path + + # ensure we got a new spawner + assert u.spawners[''] is not last_spawner + # make sure we visited hub/spawn-pending after spawn # if spawn was really quick, we might get redirected all the way to the running server, # so check history instead of r.url @@ -258,6 +271,25 @@ async def test_spawn_page(app): assert FormSpawner.options_form in r.text +async def test_spawn_page_after_failed(app, user): + cookies = await app.login_user(user.name) + + # mock a failed spawn + last_spawner = user.spawners[''] + last_spawner._spawn_future = asyncio.Future() + last_spawner._spawn_future.set_exception(RuntimeError("I failed!")) + + with mock.patch.dict(app.users.settings, {'spawner_class': FormSpawner}): + r = await get_page('spawn', app, cookies=cookies) + spawner = user.spawners[''] + # make sure we didn't reuse last spawner + assert isinstance(spawner, FormSpawner) + assert spawner is not last_spawner + assert r.url.endswith('/spawn') + spawner = user.spawners[''] + assert FormSpawner.options_form in r.text + + async def test_spawn_page_falsy_callable(app): with mock.patch.dict( app.users.settings, {'spawner_class': FalsyCallableFormSpawner} diff --git a/jupyterhub/user.py b/jupyterhub/user.py index b841e22c..71ee6ce5 100644 --- a/jupyterhub/user.py +++ b/jupyterhub/user.py @@ -253,6 +253,22 @@ class User: def spawner_class(self): return self.settings.get('spawner_class', LocalProcessSpawner) + def get_spawner(self, server_name="", replace_failed=False): + """Get a spawner by name + + replace_failed governs whether a failed spawner should be replaced + or returned (default: returned). + + .. versionadded:: 2.2 + """ + spawner = self.spawners[server_name] + if replace_failed and spawner._failed: + self.log.debug(f"Discarding failed spawner {spawner._log_name}") + # remove failed spawner, create a new one + self.spawners.pop(server_name) + spawner = self.spawners[server_name] + return spawner + def sync_groups(self, group_names): """Synchronize groups with database""" @@ -628,7 +644,7 @@ class User: api_token = self.new_api_token(note=note, roles=['server']) db.commit() - spawner = self.spawners[server_name] + spawner = self.get_spawner(server_name, replace_failed=True) spawner.server = server = Server(orm_server=orm_server) assert spawner.orm_spawner.server is orm_server