From 30b8bc36649e4e5f2ef069270314add4a9e9c808 Mon Sep 17 00:00:00 2001 From: Min RK Date: Thu, 17 Sep 2020 12:20:50 +0200 Subject: [PATCH 01/57] add ?state= filter for GET /users allows selecting users based on the 'ready' 'active' or 'inactive' states of their servers - ready: users who have any servers in the 'ready' state - active: users who have any servers in the 'active' state (i.e. ready OR pending) - inactive: users who have *no* servers in the 'active' state (inactive + active = all users, no overlap) Does not change the user model, so a user with *any* ready servers will still return all their servers --- docs/rest-api.yml | 15 ++++++++ jupyterhub/apihandlers/users.py | 53 ++++++++++++++++++++++++++- jupyterhub/tests/test_api.py | 65 +++++++++++++++++++++++++++++++++ 3 files changed, 132 insertions(+), 1 deletion(-) diff --git a/docs/rest-api.yml b/docs/rest-api.yml index 09d0ed99..8a230f8f 100644 --- a/docs/rest-api.yml +++ b/docs/rest-api.yml @@ -79,6 +79,21 @@ paths: /users: get: summary: List users + parameters: + - name: state + in: query + required: false + type: string + enum: ["inactive", "active", "ready"] + description: | + Return only users who have servers in the given state. + If unspecified, return all users. + + active: all users with any active servers (ready OR pending) + ready: all users who have any ready servers (running, not pending) + inactive: all users who have *no* active servers (complement of active) + + Added in JupyterHub 1.3 responses: '200': description: The Hub's user list diff --git a/jupyterhub/apihandlers/users.py b/jupyterhub/apihandlers/users.py index 44c829ff..02975371 100644 --- a/jupyterhub/apihandlers/users.py +++ b/jupyterhub/apihandlers/users.py @@ -9,6 +9,7 @@ from datetime import timezone from async_generator import aclosing from dateutil.parser import parse as parse_date +from sqlalchemy import func from tornado import web from tornado.iostream import StreamClosedError @@ -39,11 +40,61 @@ class SelfAPIHandler(APIHandler): class UserListAPIHandler(APIHandler): + def _user_has_ready_spawner(self, orm_user): + """Return True if a user has *any* ready spawners + + Used for filtering from active -> ready + """ + user = self.users[orm_user] + return any(spawner.ready for spawner in user.spawners.values()) + @admin_only def get(self): + state_filter = self.get_argument("state", None) + + # post_filter + post_filter = None + + if state_filter in {"active", "ready"}: + # only get users with active servers + # an 'active' Spawner has a server record in the database + # which means Spawner.server != None + # it may still be in a pending start/stop state. + # join filters out users with no Spawners + query = ( + self.db.query(orm.User) + # join filters out any Users with no Spawners + .join(orm.Spawner) + # this implicitly gets Users with *any* active server + .filter(orm.Spawner.server != None) + ) + if state_filter == "ready": + # have to post-process query results because active vs ready + # can only be distinguished with in-memory Spawner properties + post_filter = self._user_has_ready_spawner + + elif state_filter == "inactive": + # only get users with *no* active servers + # as opposed to users with *any inactive servers* + # this is the complement to the above query. + # how expensive is this with lots of servers? + query = ( + self.db.query(orm.User) + .outerjoin(orm.Spawner) + .outerjoin(orm.Server) + .group_by(orm.User.id) + .having(func.count(orm.Server.id) == 0) + ) + elif state_filter: + raise web.HTTPError(400, "Unrecognized state filter: %r" % state_filter) + else: + # no filter, return all users + query = self.db.query(orm.User) + data = [ self.user_model(u, include_servers=True, include_state=True) - for u in self.db.query(orm.User) + for u in query + if (post_filter is None or post_filter(u)) ] self.write(json.dumps(data)) diff --git a/jupyterhub/tests/test_api.py b/jupyterhub/tests/test_api.py index 56697614..7f799729 100644 --- a/jupyterhub/tests/test_api.py +++ b/jupyterhub/tests/test_api.py @@ -18,6 +18,7 @@ from tornado import gen import jupyterhub from .. import orm +from ..objects import Server from ..utils import url_path_join as ujoin from ..utils import utcnow from .mocking import public_host @@ -183,6 +184,70 @@ async def test_get_users(app): assert r.status_code == 403 +@mark.user +@mark.parametrize( + "state", ("inactive", "active", "ready", "invalid"), +) +async def test_get_users_state_filter(app, state): + db = app.db + + # has_one_active: one active, one inactive, zero ready + has_one_active = add_user(db, app=app, name='has_one_active') + # has_two_active: two active, ready servers + has_two_active = add_user(db, app=app, name='has_two_active') + # has_two_inactive: two spawners, neither active + has_two_inactive = add_user(db, app=app, name='has_two_inactive') + # has_zero: no Spawners registered at all + has_zero = add_user(db, app=app, name='has_zero') + + test_usernames = set( + ("has_one_active", "has_two_active", "has_two_inactive", "has_zero") + ) + + user_states = { + "inactive": ["has_two_inactive", "has_zero"], + "ready": ["has_two_active"], + "active": ["has_one_active", "has_two_active"], + "invalid": [], + } + expected = user_states[state] + + def add_spawner(user, name='', active=True, ready=True): + """Add a spawner in a requested state + + If active, should turn up in an active query + If active and ready, should turn up in a ready query + If not active, should turn up in an inactive query + """ + spawner = user.spawners[name] + db.commit() + if active: + orm_server = orm.Server() + db.add(orm_server) + db.commit() + spawner.server = Server(orm_server=orm_server) + db.commit() + if not ready: + spawner._spawn_pending = True + return spawner + + for name in ("", "secondary"): + add_spawner(has_two_active, name, active=True) + add_spawner(has_two_inactive, name, active=False) + + add_spawner(has_one_active, active=True, ready=False) + add_spawner(has_one_active, "inactive", active=False) + + r = await api_request(app, 'users?state={}'.format(state)) + if state == "invalid": + assert r.status_code == 400 + return + assert r.status_code == 200 + + usernames = sorted(u["name"] for u in r.json() if u["name"] in test_usernames) + assert usernames == expected + + @mark.user async def test_get_self(app): db = app.db From 6428ad9f0ba384e4de77438119577b09b3f2af94 Mon Sep 17 00:00:00 2001 From: Alex Weaver Date: Fri, 30 Oct 2020 12:13:50 -0500 Subject: [PATCH 02/57] Check proxy cmd before shutting down, cleaner shutdown on Windows --- jupyterhub/proxy.py | 27 +++++++++++++++++++++------ 1 file changed, 21 insertions(+), 6 deletions(-) diff --git a/jupyterhub/proxy.py b/jupyterhub/proxy.py index cbe1cba3..e4df553f 100644 --- a/jupyterhub/proxy.py +++ b/jupyterhub/proxy.py @@ -492,12 +492,18 @@ class ConfigurableHTTPProxy(Proxy): ) def _check_pid(self, pid): - if os.name == 'nt': - import psutil + import psutil - if not psutil.pid_exists(pid): + if not psutil.pid_exists(pid): + raise ProcessLookupError + + process = psutil.Process(pid) + if self.command and self.command[0]: + process_cmd = process.cmdline() + if process_cmd and not any(self.command[0] in clause for clause in process_cmd): raise ProcessLookupError - else: + + if not os.name == 'nt': os.kill(pid, 0) def __init__(self, **kwargs): @@ -692,8 +698,17 @@ class ConfigurableHTTPProxy(Proxy): parent = psutil.Process(pid) children = parent.children(recursive=True) for child in children: - child.kill() - psutil.wait_procs(children, timeout=5) + child.terminate() + gone, alive = psutil.wait_procs(children, timeout=5) + for p in alive: + p.kill() + # Clear the shell, too, if it still exists. + try: + parent.terminate() + parent.wait(timeout=5) + parent.kill() + except psutil.NoSuchProcess: + pass def _terminate(self): """Terminate our process""" From efb1f3c824a48bdce9f996fa2df5fe7349761c90 Mon Sep 17 00:00:00 2001 From: Alex Weaver Date: Fri, 30 Oct 2020 12:35:01 -0500 Subject: [PATCH 03/57] Run precommit hooks, fix formatting issue --- jupyterhub/proxy.py | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/jupyterhub/proxy.py b/jupyterhub/proxy.py index b8524cd5..a10b1ac1 100644 --- a/jupyterhub/proxy.py +++ b/jupyterhub/proxy.py @@ -500,7 +500,9 @@ class ConfigurableHTTPProxy(Proxy): process = psutil.Process(pid) if self.command and self.command[0]: process_cmd = process.cmdline() - if process_cmd and not any(self.command[0] in clause for clause in process_cmd): + if process_cmd and not any( + self.command[0] in clause for clause in process_cmd + ): raise ProcessLookupError if not os.name == 'nt': From 55a59a2e43155de4e98eee286cc4ba37919480bb Mon Sep 17 00:00:00 2001 From: Min RK Date: Wed, 11 Nov 2020 09:12:58 +0100 Subject: [PATCH 04/57] remove push-branch conditions for CI testing other branches is useful, and there's little cost to removing the conditions: - we don't run PRs from our repo, so test runs aren't duplicated on the repo - testing on a fork without opening a PR is still useful (I use this often) - if we push a branch, it should probably be tested (e.g. backport branch), and filters make this extra work - the cost of running a few extra tests is low, especially given actions' current quotas and parallelism --- .github/workflows/test.yml | 2 -- 1 file changed, 2 deletions(-) diff --git a/.github/workflows/test.yml b/.github/workflows/test.yml index 2a4d5760..671bf5d6 100644 --- a/.github/workflows/test.yml +++ b/.github/workflows/test.yml @@ -9,8 +9,6 @@ name: Run tests on: pull_request: push: - branches: [main, master, pr/migrate-to-gh-actions-from-travis] - tags: defaults: run: From e1166ec83445b0c58fdc049d490d5df2306a41e8 Mon Sep 17 00:00:00 2001 From: Erik Sundell Date: Wed, 4 Nov 2020 04:41:48 +0100 Subject: [PATCH 05/57] Replace @gen.coroutine/yield with async/await --- docs/source/reference/authenticators.md | 12 ++--- jupyterhub/app.py | 2 +- jupyterhub/services/auth.py | 4 +- jupyterhub/spawner.py | 6 +-- jupyterhub/tests/conftest.py | 18 +------ jupyterhub/tests/mocking.py | 49 +++++++------------ jupyterhub/tests/test_api.py | 8 +-- .../tests/test_internal_ssl_connections.py | 9 ++-- jupyterhub/tests/test_orm.py | 3 +- jupyterhub/tests/test_pages.py | 9 ++-- 10 files changed, 43 insertions(+), 77 deletions(-) diff --git a/docs/source/reference/authenticators.md b/docs/source/reference/authenticators.md index 0775809f..f0db22f0 100644 --- a/docs/source/reference/authenticators.md +++ b/docs/source/reference/authenticators.md @@ -235,10 +235,9 @@ to Spawner environment: ```python class MyAuthenticator(Authenticator): - @gen.coroutine - def authenticate(self, handler, data=None): - username = yield identify_user(handler, data) - upstream_token = yield token_for_user(username) + async def authenticate(self, handler, data=None): + username = await identify_user(handler, data) + upstream_token = await token_for_user(username) return { 'name': username, 'auth_state': { @@ -246,10 +245,9 @@ class MyAuthenticator(Authenticator): }, } - @gen.coroutine - def pre_spawn_start(self, user, spawner): + async def pre_spawn_start(self, user, spawner): """Pass upstream_token to spawner via environment variable""" - auth_state = yield user.get_auth_state() + auth_state = await user.get_auth_state() if not auth_state: # auth_state not enabled return diff --git a/jupyterhub/app.py b/jupyterhub/app.py index 407f6bdb..2aae6383 100644 --- a/jupyterhub/app.py +++ b/jupyterhub/app.py @@ -930,7 +930,7 @@ class JupyterHub(Application): with an :meth:`authenticate` method that: - - is a coroutine (asyncio or tornado) + - is a coroutine (asyncio) - returns username on success, None on failure - takes two arguments: (handler, data), where `handler` is the calling web.RequestHandler, diff --git a/jupyterhub/services/auth.py b/jupyterhub/services/auth.py index c9e6974f..2818b7df 100644 --- a/jupyterhub/services/auth.py +++ b/jupyterhub/services/auth.py @@ -23,7 +23,6 @@ from urllib.parse import quote from urllib.parse import urlencode import requests -from tornado.gen import coroutine from tornado.httputil import url_concat from tornado.log import app_log from tornado.web import HTTPError @@ -950,8 +949,7 @@ class HubOAuthCallbackHandler(HubOAuthenticated, RequestHandler): .. versionadded: 0.8 """ - @coroutine - def get(self): + async def get(self): error = self.get_argument("error", False) if error: msg = self.get_argument("error_description", error) diff --git a/jupyterhub/spawner.py b/jupyterhub/spawner.py index ad07fa44..92b6e5fd 100644 --- a/jupyterhub/spawner.py +++ b/jupyterhub/spawner.py @@ -1079,7 +1079,7 @@ class Spawner(LoggingConfigurable): Return ip, port instead of setting on self.user.server directly. """ raise NotImplementedError( - "Override in subclass. Must be a Tornado gen.coroutine." + "Override in subclass. Must be a coroutine." ) async def stop(self, now=False): @@ -1094,7 +1094,7 @@ class Spawner(LoggingConfigurable): Must be a coroutine. """ raise NotImplementedError( - "Override in subclass. Must be a Tornado gen.coroutine." + "Override in subclass. Must be a coroutine." ) async def poll(self): @@ -1122,7 +1122,7 @@ class Spawner(LoggingConfigurable): """ raise NotImplementedError( - "Override in subclass. Must be a Tornado gen.coroutine." + "Override in subclass. Must be a coroutine." ) def add_poll_callback(self, callback, *args, **kwargs): diff --git a/jupyterhub/tests/conftest.py b/jupyterhub/tests/conftest.py index 7126baa7..0e8fa04c 100644 --- a/jupyterhub/tests/conftest.py +++ b/jupyterhub/tests/conftest.py @@ -36,7 +36,6 @@ from unittest import mock from pytest import fixture from pytest import raises -from tornado import gen from tornado import ioloop from tornado.httpclient import HTTPError from tornado.platform.asyncio import AsyncIOMainLoop @@ -55,16 +54,6 @@ from .utils import ssl_setup _db = None -def pytest_collection_modifyitems(items): - """add asyncio marker to all async tests""" - for item in items: - if inspect.iscoroutinefunction(item.obj): - item.add_marker('asyncio') - if hasattr(inspect, 'isasyncgenfunction'): - # double-check that we aren't mixing yield and async def - assert not inspect.isasyncgenfunction(item.obj) - - @fixture(scope='module') def ssl_tmpdir(tmpdir_factory): return tmpdir_factory.mktemp('ssl') @@ -244,17 +233,14 @@ def _mockservice(request, app, url=False): assert name in app._service_map service = app._service_map[name] - @gen.coroutine - def start(): + async def start(): # wait for proxy to be updated before starting the service - yield app.proxy.add_all_services(app._service_map) + await app.proxy.add_all_services(app._service_map) service.start() io_loop.run_sync(start) def cleanup(): - import asyncio - asyncio.get_event_loop().run_until_complete(service.stop()) app.services[:] = [] app._service_map.clear() diff --git a/jupyterhub/tests/mocking.py b/jupyterhub/tests/mocking.py index 5084bab3..00cbfc8d 100644 --- a/jupyterhub/tests/mocking.py +++ b/jupyterhub/tests/mocking.py @@ -37,7 +37,6 @@ from urllib.parse import urlparse from pamela import PAMError from tornado import gen -from tornado.concurrent import Future from tornado.ioloop import IOLoop from traitlets import Bool from traitlets import default @@ -110,19 +109,17 @@ class SlowSpawner(MockSpawner): delay = 2 _start_future = None - @gen.coroutine - def start(self): - (ip, port) = yield super().start() + async def start(self): + (ip, port) = await super().start() if self._start_future is not None: - yield self._start_future + await self._start_future else: - yield gen.sleep(self.delay) + await gen.sleep(self.delay) return ip, port - @gen.coroutine - def stop(self): - yield gen.sleep(self.delay) - yield super().stop() + async def stop(self): + await gen.sleep(self.delay) + await super().stop() class NeverSpawner(MockSpawner): @@ -134,14 +131,12 @@ class NeverSpawner(MockSpawner): def start(self): """Return a Future that will never finish""" - return Future() + return asyncio.Future() - @gen.coroutine - def stop(self): + async def stop(self): pass - @gen.coroutine - def poll(self): + async def poll(self): return 0 @@ -215,8 +210,7 @@ class MockPAMAuthenticator(PAMAuthenticator): # skip the add-system-user bit return not user.name.startswith('dne') - @gen.coroutine - def authenticate(self, *args, **kwargs): + async def authenticate(self, *args, **kwargs): with mock.patch.multiple( 'pamela', authenticate=mock_authenticate, @@ -224,7 +218,7 @@ class MockPAMAuthenticator(PAMAuthenticator): close_session=mock_open_session, check_account=mock_check_account, ): - username = yield super(MockPAMAuthenticator, self).authenticate( + username = await super(MockPAMAuthenticator, self).authenticate( *args, **kwargs ) if username is None: @@ -320,14 +314,13 @@ class MockHub(JupyterHub): self.db.delete(group) self.db.commit() - @gen.coroutine - def initialize(self, argv=None): + async def initialize(self, argv=None): self.pid_file = NamedTemporaryFile(delete=False).name self.db_file = NamedTemporaryFile() self.db_url = os.getenv('JUPYTERHUB_TEST_DB_URL') or self.db_file.name if 'mysql' in self.db_url: self.db_kwargs['connect_args'] = {'auth_plugin': 'mysql_native_password'} - yield super().initialize([]) + await super().initialize([]) # add an initial user user = self.db.query(orm.User).filter(orm.User.name == 'user').first() @@ -358,14 +351,13 @@ class MockHub(JupyterHub): self.cleanup = lambda: None self.db_file.close() - @gen.coroutine - def login_user(self, name): + async def login_user(self, name): """Login a user by name, returning her cookies.""" base_url = public_url(self) external_ca = None if self.internal_ssl: external_ca = self.external_certs['files']['ca'] - r = yield async_requests.post( + r = await async_requests.post( base_url + 'hub/login', data={'username': name, 'password': name}, allow_redirects=False, @@ -407,8 +399,7 @@ class StubSingleUserSpawner(MockSpawner): _thread = None - @gen.coroutine - def start(self): + async def start(self): ip = self.ip = '127.0.0.1' port = self.port = random_port() env = self.get_env() @@ -435,14 +426,12 @@ class StubSingleUserSpawner(MockSpawner): assert ready return (ip, port) - @gen.coroutine - def stop(self): + async def stop(self): self._app.stop() self._thread.join(timeout=30) assert not self._thread.is_alive() - @gen.coroutine - def poll(self): + async def poll(self): if self._thread is None: return 0 if self._thread.is_alive(): diff --git a/jupyterhub/tests/test_api.py b/jupyterhub/tests/test_api.py index fa6a20d1..7ec1726a 100644 --- a/jupyterhub/tests/test_api.py +++ b/jupyterhub/tests/test_api.py @@ -1,9 +1,9 @@ """Tests for the REST API.""" +import asyncio import json import re import sys import uuid -from concurrent.futures import Future from datetime import datetime from datetime import timedelta from unittest import mock @@ -885,8 +885,8 @@ async def test_spawn_limit(app, no_patience, slow_spawn, request): # start two pending spawns names = ['ykka', 'hjarka'] users = [add_user(db, app=app, name=name) for name in names] - users[0].spawner._start_future = Future() - users[1].spawner._start_future = Future() + users[0].spawner._start_future = asyncio.Future() + users[1].spawner._start_future = asyncio.Future() for name in names: await api_request(app, 'users', name, 'server', method='post') assert app.users.count_active_users()['pending'] == 2 @@ -894,7 +894,7 @@ async def test_spawn_limit(app, no_patience, slow_spawn, request): # ykka and hjarka's spawns are both pending. Essun should fail with 429 name = 'essun' user = add_user(db, app=app, name=name) - user.spawner._start_future = Future() + user.spawner._start_future = asyncio.Future() r = await api_request(app, 'users', name, 'server', method='post') assert r.status_code == 429 diff --git a/jupyterhub/tests/test_internal_ssl_connections.py b/jupyterhub/tests/test_internal_ssl_connections.py index a650806c..47845651 100644 --- a/jupyterhub/tests/test_internal_ssl_connections.py +++ b/jupyterhub/tests/test_internal_ssl_connections.py @@ -20,8 +20,7 @@ ssl_enabled = True SSL_ERROR = (SSLError, ConnectionError) -@gen.coroutine -def wait_for_spawner(spawner, timeout=10): +async def wait_for_spawner(spawner, timeout=10): """Wait for an http server to show up polling at shorter intervals for early termination @@ -32,15 +31,15 @@ def wait_for_spawner(spawner, timeout=10): return spawner.server.wait_up(timeout=1, http=True) while time.monotonic() < deadline: - status = yield spawner.poll() + status = await spawner.poll() assert status is None try: - yield wait() + await wait() except TimeoutError: continue else: break - yield wait() + await wait() async def test_connection_hub_wrong_certs(app): diff --git a/jupyterhub/tests/test_orm.py b/jupyterhub/tests/test_orm.py index 0c125c5a..0e31cc85 100644 --- a/jupyterhub/tests/test_orm.py +++ b/jupyterhub/tests/test_orm.py @@ -222,8 +222,7 @@ async def test_spawn_fails(db): db.commit() class BadSpawner(MockSpawner): - @gen.coroutine - def start(self): + async def start(self): raise RuntimeError("Split the party") user = User( diff --git a/jupyterhub/tests/test_pages.py b/jupyterhub/tests/test_pages.py index fbb67e8c..17309465 100644 --- a/jupyterhub/tests/test_pages.py +++ b/jupyterhub/tests/test_pages.py @@ -586,8 +586,7 @@ async def test_login_strip(app): base_url = public_url(app) called_with = [] - @gen.coroutine - def mock_authenticate(handler, data): + async def mock_authenticate(handler, data): called_with.append(data) with mock.patch.object(app.authenticator, 'authenticate', mock_authenticate): @@ -943,8 +942,7 @@ 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): + async def mock_pre_spawn_start(user, spawner): raise Exception(exc) with mock.patch.object(app.authenticator, 'pre_spawn_start', mock_pre_spawn_start): @@ -959,8 +957,7 @@ 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): + async def mock_pre_spawn_start(user, spawner): raise Exception(exc) with mock.patch.dict( From 4a17441e5ac04bb7022515dd0e9bd0f141a95efd Mon Sep 17 00:00:00 2001 From: Erik Sundell Date: Thu, 5 Nov 2020 03:31:01 +0100 Subject: [PATCH 06/57] Replace gen.sleep with asyncio.sleep --- jupyterhub/handlers/base.py | 2 +- jupyterhub/singleuser/mixins.py | 3 +-- jupyterhub/tests/mocking.py | 5 ++--- jupyterhub/tests/test_api.py | 24 ++++++++++++------------ jupyterhub/tests/test_pages.py | 1 - jupyterhub/tests/test_spawner.py | 12 ++++++------ jupyterhub/utils.py | 3 +-- 7 files changed, 23 insertions(+), 27 deletions(-) diff --git a/jupyterhub/handlers/base.py b/jupyterhub/handlers/base.py index c8b2dc9e..8e358527 100644 --- a/jupyterhub/handlers/base.py +++ b/jupyterhub/handlers/base.py @@ -1538,7 +1538,7 @@ class UserUrlHandler(BaseHandler): if redirects: self.log.warning("Redirect loop detected on %s", self.request.uri) # add capped exponential backoff where cap is 10s - await gen.sleep(min(1 * (2 ** redirects), 10)) + await asyncio.sleep(min(1 * (2 ** redirects), 10)) # rewrite target url with new `redirects` query value url_parts = urlparse(target) query_parts = parse_qs(url_parts.query) diff --git a/jupyterhub/singleuser/mixins.py b/jupyterhub/singleuser/mixins.py index 1703e9a0..c0912710 100755 --- a/jupyterhub/singleuser/mixins.py +++ b/jupyterhub/singleuser/mixins.py @@ -20,7 +20,6 @@ from urllib.parse import urlparse from jinja2 import ChoiceLoader from jinja2 import FunctionLoader -from tornado import gen from tornado import ioloop from tornado.httpclient import AsyncHTTPClient from tornado.httpclient import HTTPRequest @@ -434,7 +433,7 @@ class SingleUserNotebookAppMixin(Configurable): i, RETRIES, ) - await gen.sleep(min(2 ** i, 16)) + await asyncio.sleep(min(2 ** i, 16)) else: break else: diff --git a/jupyterhub/tests/mocking.py b/jupyterhub/tests/mocking.py index 00cbfc8d..4cadd128 100644 --- a/jupyterhub/tests/mocking.py +++ b/jupyterhub/tests/mocking.py @@ -36,7 +36,6 @@ from unittest import mock from urllib.parse import urlparse from pamela import PAMError -from tornado import gen from tornado.ioloop import IOLoop from traitlets import Bool from traitlets import default @@ -114,11 +113,11 @@ class SlowSpawner(MockSpawner): if self._start_future is not None: await self._start_future else: - await gen.sleep(self.delay) + await asyncio.sleep(self.delay) return ip, port async def stop(self): - await gen.sleep(self.delay) + await asyncio.sleep(self.delay) await super().stop() diff --git a/jupyterhub/tests/test_api.py b/jupyterhub/tests/test_api.py index 7ec1726a..3f38bf40 100644 --- a/jupyterhub/tests/test_api.py +++ b/jupyterhub/tests/test_api.py @@ -613,7 +613,7 @@ async def test_slow_spawn(app, no_patience, slow_spawn): async def wait_spawn(): while not app_user.running: - await gen.sleep(0.1) + await asyncio.sleep(0.1) await wait_spawn() assert not app_user.spawner._spawn_pending @@ -622,7 +622,7 @@ async def test_slow_spawn(app, no_patience, slow_spawn): async def wait_stop(): while app_user.spawner._stop_pending: - await gen.sleep(0.1) + await asyncio.sleep(0.1) r = await api_request(app, 'users', name, 'server', method='delete') r.raise_for_status() @@ -656,7 +656,7 @@ async def test_never_spawn(app, no_patience, never_spawn): assert app.users.count_active_users()['pending'] == 1 while app_user.spawner.pending: - await gen.sleep(0.1) + await asyncio.sleep(0.1) print(app_user.spawner.pending) assert not app_user.spawner._spawn_pending @@ -682,7 +682,7 @@ async def test_slow_bad_spawn(app, no_patience, slow_bad_spawn): r = await api_request(app, 'users', name, 'server', method='post') r.raise_for_status() while user.spawner.pending: - await gen.sleep(0.1) + await asyncio.sleep(0.1) # spawn failed assert not user.running assert app.users.count_active_users()['pending'] == 0 @@ -824,7 +824,7 @@ async def progress_forever(): for i in range(1, 10): await yield_({'progress': i, 'message': 'Stage %s' % i}) # wait a long time before the next event - await gen.sleep(10) + await asyncio.sleep(10) if sys.version_info >= (3, 6): @@ -840,7 +840,7 @@ async def progress_forever_native(): 'message': 'Stage %s' % i, } # wait a long time before the next event - await gen.sleep(10) + await asyncio.sleep(10) """, globals(), ) @@ -902,7 +902,7 @@ async def test_spawn_limit(app, no_patience, slow_spawn, request): users[0].spawner._start_future.set_result(None) # wait for ykka to finish while not users[0].running: - await gen.sleep(0.1) + await asyncio.sleep(0.1) assert app.users.count_active_users()['pending'] == 1 r = await api_request(app, 'users', name, 'server', method='post') @@ -913,7 +913,7 @@ async def test_spawn_limit(app, no_patience, slow_spawn, request): for user in users[1:]: user.spawner._start_future.set_result(None) while not all(u.running for u in users): - await gen.sleep(0.1) + await asyncio.sleep(0.1) # everybody's running, pending count should be back to 0 assert app.users.count_active_users()['pending'] == 0 @@ -922,7 +922,7 @@ async def test_spawn_limit(app, no_patience, slow_spawn, request): r = await api_request(app, 'users', u.name, 'server', method='delete') r.raise_for_status() while any(u.spawner.active for u in users): - await gen.sleep(0.1) + await asyncio.sleep(0.1) @mark.slow @@ -1000,7 +1000,7 @@ async def test_start_stop_race(app, no_patience, slow_spawn): r = await api_request(app, 'users', user.name, 'server', method='delete') assert r.status_code == 400 while not spawner.ready: - await gen.sleep(0.1) + await asyncio.sleep(0.1) spawner.delay = 3 # stop the spawner @@ -1008,7 +1008,7 @@ async def test_start_stop_race(app, no_patience, slow_spawn): assert r.status_code == 202 assert spawner.pending == 'stop' # make sure we get past deleting from the proxy - await gen.sleep(1) + await asyncio.sleep(1) # additional stops while stopping shouldn't trigger a new stop with mock.patch.object(spawner, 'stop') as m: r = await api_request(app, 'users', user.name, 'server', method='delete') @@ -1020,7 +1020,7 @@ async def test_start_stop_race(app, no_patience, slow_spawn): assert r.status_code == 400 while spawner.active: - await gen.sleep(0.1) + await asyncio.sleep(0.1) # start after stop is okay r = await api_request(app, 'users', user.name, 'server', method='post') assert r.status_code == 202 diff --git a/jupyterhub/tests/test_pages.py b/jupyterhub/tests/test_pages.py index 17309465..faef5072 100644 --- a/jupyterhub/tests/test_pages.py +++ b/jupyterhub/tests/test_pages.py @@ -8,7 +8,6 @@ from urllib.parse import urlparse import pytest from bs4 import BeautifulSoup -from tornado import gen from tornado.escape import url_escape from tornado.httputil import url_concat diff --git a/jupyterhub/tests/test_spawner.py b/jupyterhub/tests/test_spawner.py index 99b84393..e728081d 100644 --- a/jupyterhub/tests/test_spawner.py +++ b/jupyterhub/tests/test_spawner.py @@ -1,6 +1,7 @@ """Tests for process spawning""" # Copyright (c) Jupyter Development Team. # Distributed under the terms of the Modified BSD License. +import asyncio import logging import os import signal @@ -12,7 +13,6 @@ from unittest import mock from urllib.parse import urlparse import pytest -from tornado import gen from .. import orm from .. import spawner as spawnermod @@ -123,7 +123,7 @@ async def test_stop_spawner_sigint_fails(db): await spawner.start() # wait for the process to get to the while True: loop - await gen.sleep(1) + await asyncio.sleep(1) status = await spawner.poll() assert status is None @@ -138,7 +138,7 @@ async def test_stop_spawner_stop_now(db): await spawner.start() # wait for the process to get to the while True: loop - await gen.sleep(1) + await asyncio.sleep(1) status = await spawner.poll() assert status is None @@ -165,7 +165,7 @@ async def test_spawner_poll(db): spawner.start_polling() # wait for the process to get to the while True: loop - await gen.sleep(1) + await asyncio.sleep(1) status = await spawner.poll() assert status is None @@ -173,12 +173,12 @@ async def test_spawner_poll(db): proc.terminate() for i in range(10): if proc.poll() is None: - await gen.sleep(1) + await asyncio.sleep(1) else: break assert proc.poll() is not None - await gen.sleep(2) + await asyncio.sleep(2) status = await spawner.poll() assert status is not None diff --git a/jupyterhub/utils.py b/jupyterhub/utils.py index 73235914..9499feeb 100644 --- a/jupyterhub/utils.py +++ b/jupyterhub/utils.py @@ -24,7 +24,6 @@ from async_generator import aclosing from async_generator import async_generator from async_generator import asynccontextmanager from async_generator import yield_ -from tornado import gen from tornado import ioloop from tornado import web from tornado.httpclient import AsyncHTTPClient @@ -175,7 +174,7 @@ async def exponential_backoff( dt = min(max_wait, remaining, random.uniform(0, start_wait * scale)) if dt < max_wait: scale *= scale_factor - await gen.sleep(dt) + await asyncio.sleep(dt) raise TimeoutError(fail_message) From 77ed2faf317a28964c8f522176536251ab1f45ee Mon Sep 17 00:00:00 2001 From: Erik Sundell Date: Fri, 6 Nov 2020 06:41:29 +0100 Subject: [PATCH 07/57] Replace gen.multi(futures) with asyncio.gather(*futures) --- jupyterhub/app.py | 2 +- jupyterhub/proxy.py | 7 +++---- jupyterhub/tests/test_api.py | 1 - 3 files changed, 4 insertions(+), 6 deletions(-) diff --git a/jupyterhub/app.py b/jupyterhub/app.py index 2aae6383..af394990 100644 --- a/jupyterhub/app.py +++ b/jupyterhub/app.py @@ -2120,7 +2120,7 @@ class JupyterHub(Application): self.log.debug( "Awaiting checks for %i possibly-running spawners", len(check_futures) ) - await gen.multi(check_futures) + await asyncio.gather(*check_futures) db.commit() # only perform this query if we are going to log it diff --git a/jupyterhub/proxy.py b/jupyterhub/proxy.py index 5b8d386a..973efa02 100644 --- a/jupyterhub/proxy.py +++ b/jupyterhub/proxy.py @@ -25,7 +25,6 @@ from functools import wraps from subprocess import Popen from urllib.parse import quote -from tornado import gen from tornado.httpclient import AsyncHTTPClient from tornado.httpclient import HTTPError from tornado.httpclient import HTTPRequest @@ -292,7 +291,7 @@ class Proxy(LoggingConfigurable): if service.server: futures.append(self.add_service(service)) # wait after submitting them all - await gen.multi(futures) + await asyncio.gather(*futures) async def add_all_users(self, user_dict): """Update the proxy table from the database. @@ -305,7 +304,7 @@ class Proxy(LoggingConfigurable): if spawner.ready: futures.append(self.add_user(user, name)) # wait after submitting them all - await gen.multi(futures) + await asyncio.gather(*futures) @_one_at_a_time async def check_routes(self, user_dict, service_dict, routes=None): @@ -391,7 +390,7 @@ class Proxy(LoggingConfigurable): self.log.warning("Deleting stale route %s", routespec) futures.append(self.delete_route(routespec)) - await gen.multi(futures) + await asyncio.gather(*futures) stop = time.perf_counter() # timer stops here when user is deleted CHECK_ROUTES_DURATION_SECONDS.observe(stop - start) # histogram metric diff --git a/jupyterhub/tests/test_api.py b/jupyterhub/tests/test_api.py index 3f38bf40..f8c647ec 100644 --- a/jupyterhub/tests/test_api.py +++ b/jupyterhub/tests/test_api.py @@ -13,7 +13,6 @@ from urllib.parse import urlparse from async_generator import async_generator from async_generator import yield_ from pytest import mark -from tornado import gen import jupyterhub from .. import orm From 5edd2464748f3aab08977bc071571305ba63f8e0 Mon Sep 17 00:00:00 2001 From: Erik Sundell Date: Fri, 6 Nov 2020 07:43:08 +0100 Subject: [PATCH 08/57] Replace @async_generator/yeild_ with async/yeild --- jupyterhub/spawner.py | 26 +++++++------------------- jupyterhub/tests/test_api.py | 30 ++---------------------------- jupyterhub/tests/test_services.py | 5 +---- jupyterhub/tests/test_utils.py | 8 ++------ jupyterhub/utils.py | 21 +-------------------- 5 files changed, 13 insertions(+), 77 deletions(-) diff --git a/jupyterhub/spawner.py b/jupyterhub/spawner.py index 92b6e5fd..7dea35d7 100644 --- a/jupyterhub/spawner.py +++ b/jupyterhub/spawner.py @@ -16,8 +16,7 @@ from tempfile import mkdtemp if os.name == 'nt': import psutil -from async_generator import async_generator -from async_generator import yield_ +from async_generator import aclosing from sqlalchemy import inspect from tornado.ioloop import PeriodicCallback from traitlets import Any @@ -1024,7 +1023,6 @@ class Spawner(LoggingConfigurable): def _progress_url(self): return self.user.progress_url(self.name) - @async_generator async def _generate_progress(self): """Private wrapper of progress generator @@ -1036,21 +1034,17 @@ class Spawner(LoggingConfigurable): ) return - await yield_({"progress": 0, "message": "Server requested"}) - from async_generator import aclosing + yield {"progress": 0, "message": "Server requested"} async with aclosing(self.progress()) as progress: async for event in progress: - await yield_(event) + yield event - @async_generator async def progress(self): """Async generator for progress events Must be an async generator - For Python 3.5-compatibility, use the async_generator package - Should yield messages of the form: :: @@ -1067,7 +1061,7 @@ class Spawner(LoggingConfigurable): .. versionadded:: 0.9 """ - await yield_({"progress": 50, "message": "Spawning server..."}) + yield {"progress": 50, "message": "Spawning server..."} async def start(self): """Start the single-user server @@ -1078,9 +1072,7 @@ class Spawner(LoggingConfigurable): .. versionchanged:: 0.7 Return ip, port instead of setting on self.user.server directly. """ - raise NotImplementedError( - "Override in subclass. Must be a coroutine." - ) + raise NotImplementedError("Override in subclass. Must be a coroutine.") async def stop(self, now=False): """Stop the single-user server @@ -1093,9 +1085,7 @@ class Spawner(LoggingConfigurable): Must be a coroutine. """ - raise NotImplementedError( - "Override in subclass. Must be a coroutine." - ) + raise NotImplementedError("Override in subclass. Must be a coroutine.") async def poll(self): """Check if the single-user process is running @@ -1121,9 +1111,7 @@ class Spawner(LoggingConfigurable): process has not yet completed. """ - raise NotImplementedError( - "Override in subclass. Must be a coroutine." - ) + raise NotImplementedError("Override in subclass. Must be a coroutine.") def add_poll_callback(self, callback, *args, **kwargs): """Add a callback to fire when the single-user server stops""" diff --git a/jupyterhub/tests/test_api.py b/jupyterhub/tests/test_api.py index f8c647ec..0099ec14 100644 --- a/jupyterhub/tests/test_api.py +++ b/jupyterhub/tests/test_api.py @@ -10,8 +10,6 @@ from unittest import mock from urllib.parse import quote from urllib.parse import urlparse -from async_generator import async_generator -from async_generator import yield_ from pytest import mark import jupyterhub @@ -817,34 +815,14 @@ async def test_progress_bad_slow(request, app, no_patience, slow_bad_spawn): } -@async_generator async def progress_forever(): """progress function that yields messages forever""" for i in range(1, 10): - await yield_({'progress': i, 'message': 'Stage %s' % i}) + yield {'progress': i, 'message': 'Stage %s' % i} # wait a long time before the next event await asyncio.sleep(10) -if sys.version_info >= (3, 6): - # additional progress_forever defined as native - # async generator - # to test for issues with async_generator wrappers - exec( - """ -async def progress_forever_native(): - for i in range(1, 10): - yield { - 'progress': i, - 'message': 'Stage %s' % i, - } - # wait a long time before the next event - await asyncio.sleep(10) -""", - globals(), - ) - - async def test_spawn_progress_cutoff(request, app, no_patience, slow_spawn): """Progress events stop when Spawner finishes @@ -853,11 +831,7 @@ async def test_spawn_progress_cutoff(request, app, no_patience, slow_spawn): db = app.db name = 'geddy' app_user = add_user(db, app=app, name=name) - if sys.version_info >= (3, 6): - # Python >= 3.6, try native async generator - app_user.spawner.progress = globals()['progress_forever_native'] - else: - app_user.spawner.progress = progress_forever + app_user.spawner.progress = progress_forever app_user.spawner.delay = 1 r = await api_request(app, 'users', name, 'server', method='post') diff --git a/jupyterhub/tests/test_services.py b/jupyterhub/tests/test_services.py index 127a9f45..94b56711 100644 --- a/jupyterhub/tests/test_services.py +++ b/jupyterhub/tests/test_services.py @@ -6,9 +6,7 @@ from binascii import hexlify from contextlib import contextmanager from subprocess import Popen -from async_generator import async_generator from async_generator import asynccontextmanager -from async_generator import yield_ from tornado.ioloop import IOLoop from ..utils import maybe_future @@ -24,7 +22,6 @@ mockservice_cmd = [sys.executable, mockservice_py] @asynccontextmanager -@async_generator async def external_service(app, name='mockservice'): env = { 'JUPYTERHUB_API_TOKEN': hexlify(os.urandom(5)), @@ -35,7 +32,7 @@ async def external_service(app, name='mockservice'): proc = Popen(mockservice_cmd, env=env) try: await wait_for_http_server(env['JUPYTERHUB_SERVICE_URL']) - await yield_(env) + yield env finally: proc.terminate() diff --git a/jupyterhub/tests/test_utils.py b/jupyterhub/tests/test_utils.py index 49155e92..1c80c71a 100644 --- a/jupyterhub/tests/test_utils.py +++ b/jupyterhub/tests/test_utils.py @@ -3,19 +3,16 @@ import asyncio import pytest from async_generator import aclosing -from async_generator import async_generator -from async_generator import yield_ from ..utils import iterate_until -@async_generator async def yield_n(n, delay=0.01): """Yield n items with a delay between each""" for i in range(n): if delay: await asyncio.sleep(delay) - await yield_(i) + yield i def schedule_future(io_loop, *, delay, result=None): @@ -50,10 +47,9 @@ async def test_iterate_until(io_loop, deadline, n, delay, expected): async def test_iterate_until_ready_after_deadline(io_loop): f = schedule_future(io_loop, delay=0) - @async_generator async def gen(): for i in range(5): - await yield_(i) + yield i yielded = [] async with aclosing(iterate_until(f, gen())) as items: diff --git a/jupyterhub/utils.py b/jupyterhub/utils.py index 9499feeb..d40b513f 100644 --- a/jupyterhub/utils.py +++ b/jupyterhub/utils.py @@ -21,9 +21,6 @@ from hmac import compare_digest from operator import itemgetter from async_generator import aclosing -from async_generator import async_generator -from async_generator import asynccontextmanager -from async_generator import yield_ from tornado import ioloop from tornado import web from tornado.httpclient import AsyncHTTPClient @@ -512,22 +509,6 @@ def maybe_future(obj): return f -@asynccontextmanager -@async_generator -async def not_aclosing(coro): - """An empty context manager for Python < 3.5.2 - which lacks the `aclose` method on async iterators - """ - await yield_(await coro) - - -if sys.version_info < (3, 5, 2): - # Python 3.5.1 is missing the aclose method on async iterators, - # so we can't close them - aclosing = not_aclosing - - -@async_generator async def iterate_until(deadline_future, generator): """An async generator that yields items from a generator until a deadline future resolves @@ -552,7 +533,7 @@ async def iterate_until(deadline_future, generator): ) if item_future.done(): try: - await yield_(item_future.result()) + yield item_future.result() except (StopAsyncIteration, asyncio.CancelledError): break elif deadline_future.done(): From fca2528332365cc7d5258f82a9e06f358d82a603 Mon Sep 17 00:00:00 2001 From: Erik Sundell Date: Sun, 8 Nov 2020 21:54:02 +0100 Subject: [PATCH 09/57] Retain explicit pytest mark asyncio of our coroutines --- jupyterhub/tests/conftest.py | 7 +++++++ 1 file changed, 7 insertions(+) diff --git a/jupyterhub/tests/conftest.py b/jupyterhub/tests/conftest.py index 0e8fa04c..c4c08b90 100644 --- a/jupyterhub/tests/conftest.py +++ b/jupyterhub/tests/conftest.py @@ -54,6 +54,13 @@ from .utils import ssl_setup _db = None +def pytest_collection_modifyitems(items): + """add asyncio marker to all async tests""" + for item in items: + if inspect.iscoroutinefunction(item.obj): + item.add_marker('asyncio') + + @fixture(scope='module') def ssl_tmpdir(tmpdir_factory): return tmpdir_factory.mktemp('ssl') From d581cf54cbb1df39e4dd846a01ab2f3dbe262f27 Mon Sep 17 00:00:00 2001 From: Erik Sundell Date: Wed, 11 Nov 2020 15:40:54 +0100 Subject: [PATCH 10/57] Retain an assertion and update comments --- jupyterhub/app.py | 2 +- jupyterhub/tests/conftest.py | 8 +++++++- 2 files changed, 8 insertions(+), 2 deletions(-) diff --git a/jupyterhub/app.py b/jupyterhub/app.py index af394990..676198a6 100644 --- a/jupyterhub/app.py +++ b/jupyterhub/app.py @@ -930,7 +930,7 @@ class JupyterHub(Application): with an :meth:`authenticate` method that: - - is a coroutine (asyncio) + - is a coroutine (asyncio or tornado) - returns username on success, None on failure - takes two arguments: (handler, data), where `handler` is the calling web.RequestHandler, diff --git a/jupyterhub/tests/conftest.py b/jupyterhub/tests/conftest.py index c4c08b90..34c94e4f 100644 --- a/jupyterhub/tests/conftest.py +++ b/jupyterhub/tests/conftest.py @@ -55,10 +55,16 @@ _db = None def pytest_collection_modifyitems(items): - """add asyncio marker to all async tests""" + """This function is automatically run by pytest passing all collected test + functions. + + We use it to add asyncio marker to all async tests and assert we don't use + test functions that are async generators which wouldn't make sense. + """ for item in items: if inspect.iscoroutinefunction(item.obj): item.add_marker('asyncio') + assert not inspect.isasyncgenfunction(item.obj) @fixture(scope='module') From 9da4aa236ec88660771efdc28b6fa16397eec470 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?F=C3=A9lix-Antoine=20Fortin?= Date: Wed, 11 Nov 2020 13:01:14 -0500 Subject: [PATCH 11/57] Standardize Sign in capitalization on the login page --- share/jupyterhub/templates/login.html | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/share/jupyterhub/templates/login.html b/share/jupyterhub/templates/login.html index 5c00d31d..9359d9ec 100644 --- a/share/jupyterhub/templates/login.html +++ b/share/jupyterhub/templates/login.html @@ -61,7 +61,7 @@ id="login_submit" type="submit" class='btn btn-jupyter' - value='Sign In' + value='Sign in' tabindex="3" />