From 4f78cbbd1bd1350ca60332b1ea26bad98bcbdab6 Mon Sep 17 00:00:00 2001 From: Min RK Date: Wed, 4 Apr 2018 13:47:57 +0200 Subject: [PATCH 01/26] implement progress on spawn_pending page - add Spawner.progress method. Must be an async generator of JSON-able progress events - add /api/users/:user/server-progress eventstream endpoint - use eventstream to fill progress bar on the spawn pending page --- jupyterhub/apihandlers/base.py | 5 +- jupyterhub/apihandlers/users.py | 105 +++++++++++++++++- jupyterhub/handlers/base.py | 18 ++- jupyterhub/spawner.py | 69 +++++++++++- requirements.txt | 1 + share/jupyterhub/static/less/page.less | 14 +++ share/jupyterhub/templates/spawn_pending.html | 68 +++++++++++- 7 files changed, 266 insertions(+), 14 deletions(-) diff --git a/jupyterhub/apihandlers/base.py b/jupyterhub/apihandlers/base.py index 2a91d04e..a34e2088 100644 --- a/jupyterhub/apihandlers/base.py +++ b/jupyterhub/apihandlers/base.py @@ -18,9 +18,12 @@ class APIHandler(BaseHandler): def content_security_policy(self): return '; '.join([super().content_security_policy, "default-src 'none'"]) + def get_content_type(self): + return 'application/json' + def set_default_headers(self): super().set_default_headers() - self.set_header('Content-Type', 'application/json') + self.set_header('Content-Type', self.get_content_type()) def check_referer(self): """Check Origin for cross-site API requests. diff --git a/jupyterhub/apihandlers/users.py b/jupyterhub/apihandlers/users.py index 9247835e..a1f5e9f3 100644 --- a/jupyterhub/apihandlers/users.py +++ b/jupyterhub/apihandlers/users.py @@ -5,10 +5,11 @@ import json -from tornado import gen, web +from tornado import web +from tornado.iostream import StreamClosedError from .. import orm -from ..utils import admin_only, maybe_future +from ..utils import admin_only, maybe_future, url_path_join from .base import APIHandler @@ -17,6 +18,7 @@ class SelfAPIHandler(APIHandler): Based on the authentication info. Acts as a 'whoami' for auth tokens. """ + async def get(self): user = self.get_current_user() if user is None: @@ -102,6 +104,7 @@ def admin_or_self(method): return method(self, name, *args, **kwargs) return m + class UserAPIHandler(APIHandler): @admin_or_self @@ -269,10 +272,108 @@ class UserAdminAccessAPIHandler(APIHandler): raise web.HTTPError(404) +class SpawnProgressAPIHandler(APIHandler): + """EventStream handler for pending spawns""" + def get_content_type(self): + return 'text/event-stream' + + async def send_event(self, event): + try: + self.write('data: {}\n\n'.format(json.dumps(event))) + await self.flush() + except StreamClosedError: + self.log.warning("Stream closed while handling %s", self.request.uri) + # raise Finish to halt the handler + raise web.Finish() + + @admin_or_self + async def get(self, username, server_name=''): + self.set_header('Cache-Control', 'no-cache') + if server_name is None: + server_name = '' + user = self.find_user(username) + if user is None: + # no such user + raise web.HTTPError(404) + if server_name not in user.spawners: + # user has no such server + raise web.HTTPError(404) + spawner = user.spawners[server_name] + # cases: + # - spawner already started and ready + # - spawner not running at all + # - spawner failed + # - spawner pending start (what we expect) + url = url_path_join(user.url, server_name, '/') + ready_event = { + 'progress': 100, + 'ready': True, + 'message': "Server ready at {}".format(url), + 'html_message': 'Server ready at {0}'.format(url), + 'url': url, + } + failed_event = { + 'progress': 100, + 'failed': True, + 'message': "Spawn failed", + } + + if spawner.ready: + # spawner already ready. Trigger progress-completion immediately + self.log.info("Server %s is already started", spawner._log_name) + await self.send_event(ready_event) + return + + if not spawner._spawn_pending: + # not pending, no progress to fetch + # check if spawner has just failed + f = spawner._spawn_future + if f and f.done() and f.exception(): + failed_event['message'] = "Spawn failed: %s" % f.exception() + await self.send_event(failed_event) + return + else: + raise web.HTTPError(400, "%s is not starting...", spawner._log_name) + + # retrieve progress events from the Spawner + async for event in spawner._generate_progress(): + # don't allow events to sneakily set the 'ready flag' + if 'ready' in event: + event.pop('ready', None) + await self.send_event(event) + + # events finished, check if we are still pending + if spawner._spawn_pending: + try: + await spawner._spawn_future + except Exception: + # suppress exceptions in spawn future, + # which will be logged elsewhere + pass + + # progress finished, check if we are done + if spawner.ready: + # spawner is ready, signal completion and redirect + self.log.info("Server %s is ready", spawner._log_name) + await self.send_event(ready_event) + return + else: + # what happened? Maybe spawn failed? + f = spawner._spawn_future + if f and f.done() and f.exception(): + failed_event['message'] = "Spawn failed: %s" % f.exception() + else: + self.log.warning("Server %s didn't start for unknown reason", spawner._log_name) + await self.send_event(failed_event) + return + + default_handlers = [ (r"/api/user", SelfAPIHandler), (r"/api/users", UserListAPIHandler), (r"/api/users/([^/]+)", UserAPIHandler), + (r"/api/users/([^/]+)/server-progress", SpawnProgressAPIHandler), + (r"/api/users/([^/]+)/server-progress/([^/]*)", SpawnProgressAPIHandler), (r"/api/users/([^/]+)/server", UserServerAPIHandler), (r"/api/users/([^/]+)/servers/([^/]*)", UserServerAPIHandler), (r"/api/users/([^/]+)/admin-access", UserAdminAccessAPIHandler), diff --git a/jupyterhub/handlers/base.py b/jupyterhub/handlers/base.py index fdd01e86..7e61fc65 100644 --- a/jupyterhub/handlers/base.py +++ b/jupyterhub/handlers/base.py @@ -887,11 +887,21 @@ class UserSpawnHandler(BaseHandler): pass # we may have waited above, check pending again: + progress_url = url_path_join( + self.hub.base_url, 'api/users', + user.escaped_name, 'server-progress', spawner.name, + ) + if spawner.pending: self.log.info("%s is pending %s", spawner._log_name, spawner.pending) # spawn has started, but not finished self.statsd.incr('redirects.user_spawn_pending', 1) - html = self.render_template("spawn_pending.html", user=user) + url_parts = [] + html = self.render_template( + "spawn_pending.html", + user=user, + progress_url=progress_url, + ) self.finish(html) return @@ -919,7 +929,11 @@ class UserSpawnHandler(BaseHandler): self.log.info("%s is pending %s", spawner._log_name, spawner.pending) # spawn has started, but not finished self.statsd.incr('redirects.user_spawn_pending', 1) - html = self.render_template("spawn_pending.html", user=user) + html = self.render_template( + "spawn_pending.html", + user=user, + progress_url=progress_url, + ) self.finish(html) return diff --git a/jupyterhub/spawner.py b/jupyterhub/spawner.py index 35b625ba..01df8183 100644 --- a/jupyterhub/spawner.py +++ b/jupyterhub/spawner.py @@ -16,6 +16,9 @@ import warnings from subprocess import Popen from tempfile import mkdtemp +# FIXME: remove when we drop Python 3.5 support +from async_generator import isasyncgenfunction, async_generator, yield_ + from sqlalchemy import inspect from tornado import gen, concurrent @@ -47,7 +50,7 @@ class Spawner(LoggingConfigurable): is created for each user. If there are 20 JupyterHub users, there will be 20 instances of the subclass. """ - + # private attributes for tracking status _spawn_pending = False _start_pending = False @@ -264,11 +267,10 @@ class Spawner(LoggingConfigurable): """Get the options form Returns: - (Future(str)): the content of the options form presented to the user + Future (str): the content of the options form presented to the user prior to starting a Spawner. - .. versionadded:: 0.9.0 - Introduced. + .. versionadded:: 0.9 """ if callable(self.options_form): options_form = await maybe_future(self.options_form(self)) @@ -690,6 +692,65 @@ class Spawner(LoggingConfigurable): if self.pre_spawn_hook: return self.pre_spawn_hook(self) + @async_generator + async def _generate_progress(self): + """Private wrapper of progress generator + + This method is always an async generator and will always yield at least one event. + + Calls self._default_progress if self.progress is not an async generator + """ + if not self._spawn_pending: + raise RuntimeError("Spawn not pending, can't generate progress") + await yield_({ + "progress": 0, + "message": "Server requested", + }) + if isasyncgenfunction(self.progress): + progress = self.progress + else: + progress = self._default_progress + + # TODO: stop when spawn is ready, even if progress isn't + async for event in progress(): + await yield_(event) + + @async_generator + async def _default_progress(self): + """The default progress events generator + + Yields just one generic event for 50% progress + """ + await yield_({ + "progress": 50, + "message": "Spawning server...", + }) + + 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: + + :: + + { + "progress": 80, # integer, out of 100 + "message": text, # text message (will be escaped for HTML) + "html_message": html_text, # optional html-formatted message (may have links) + } + + In HTML contexts, html_message will be displayed instead of message if present. + Progress will be updated if defined. + To update messages without progress omit the progress field. + + .. versionadded:: 0.9 + """ + pass + async def start(self): """Start the single-user server diff --git a/requirements.txt b/requirements.txt index a3a47065..9c75ebd1 100644 --- a/requirements.txt +++ b/requirements.txt @@ -1,4 +1,5 @@ alembic +async_generator>=1.8 traitlets>=4.3.2 tornado>=4.1 jinja2 diff --git a/share/jupyterhub/static/less/page.less b/share/jupyterhub/static/less/page.less index 0ffdeab9..6193b357 100644 --- a/share/jupyterhub/static/less/page.less +++ b/share/jupyterhub/static/less/page.less @@ -12,3 +12,17 @@ .hidden { display: none; } + +#progress-log { + margin-top: 8px; +} + +.progress-log-event { + border-top: 1px solid #e7e7e7; + padding: 8px; +} + +// hover-highlight on log events? +// .progress-log-event:hover { +// background: rgba(66, 165, 245, 0.2); +// } diff --git a/share/jupyterhub/templates/spawn_pending.html b/share/jupyterhub/templates/spawn_pending.html index 71c88d08..a255f63a 100644 --- a/share/jupyterhub/templates/spawn_pending.html +++ b/share/jupyterhub/templates/spawn_pending.html @@ -10,9 +10,20 @@

You will be redirected automatically when it's ready for you.

{% endblock %}

- refresh - +
+
+ 0% Complete +
+
+

+
+
+
+ Event log +
+
+
{% endblock %} @@ -24,9 +35,56 @@ require(["jquery"], function ($) { $("#refresh").click(function () { window.location.reload(); }) - setTimeout(function () { - window.location.reload(); - }, 5000); + + // hook up event-stream for progress + var evtSource = new EventSource("{{ progress_url }}"); + var progressMessage = $("#progress-message"); + var progressBar = $("#progress-bar"); + var srProgress = $("#sr-progress"); + var progressLog = $("#progress-log"); + + evtSource.onmessage = function(e) { + var evt = JSON.parse(e.data); + console.log(evt); + if (evt.progress !== undefined) { + // update progress + var progText = evt.progress.toString(); + progressBar.attr('aria-valuenow', progText); + srProgress.text(progText + '%'); + progressBar.css('width', progText + '%'); + } + // update message + var html_message; + if (evt.html_message !== undefined) { + progressMessage.html(evt.html_message); + html_message = evt.html_message; + } else if (evt.message !== undefined) { + progressMessage.text(evt.message); + html_message = progressMessage.html(); + } + if (html_message) { + progressLog.append( + $("
") + .addClass('progress-log-event') + .html(html_message) + ); + } + + if (evt.ready) { + // reload the current page + // which should result in a redirect to the running server + window.location.reload(); + } + + if (evt.failed) { + evtSource.close(); + // turn progress bar red + progressBar.addClass('progress-bar-danger'); + // open event log for debugging + $('progress-details').prop('open', true); + } + } + }); {% endblock %} From 4eb07f9d4892c9faca9bd733b75b24f632469853 Mon Sep 17 00:00:00 2001 From: Min RK Date: Wed, 4 Apr 2018 13:48:05 +0200 Subject: [PATCH 02/26] stop spinner on failure --- share/jupyterhub/templates/spawn_pending.html | 2 ++ 1 file changed, 2 insertions(+) diff --git a/share/jupyterhub/templates/spawn_pending.html b/share/jupyterhub/templates/spawn_pending.html index a255f63a..291004c7 100644 --- a/share/jupyterhub/templates/spawn_pending.html +++ b/share/jupyterhub/templates/spawn_pending.html @@ -78,6 +78,8 @@ require(["jquery"], function ($) { if (evt.failed) { evtSource.close(); + // stop spinner + $(".fa-pulse").removeClass("fa-pulse"); // turn progress bar red progressBar.addClass('progress-bar-danger'); // open event log for debugging From 9b7186e9b8ccd4150353ae97e7327107755f3549 Mon Sep 17 00:00:00 2001 From: Min RK Date: Wed, 4 Apr 2018 15:25:49 +0200 Subject: [PATCH 03/26] close eventstream on success --- share/jupyterhub/templates/spawn_pending.html | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/share/jupyterhub/templates/spawn_pending.html b/share/jupyterhub/templates/spawn_pending.html index 291004c7..1ec87a73 100644 --- a/share/jupyterhub/templates/spawn_pending.html +++ b/share/jupyterhub/templates/spawn_pending.html @@ -71,6 +71,7 @@ require(["jquery"], function ($) { } if (evt.ready) { + evtSource.close(); // reload the current page // which should result in a redirect to the running server window.location.reload(); @@ -83,7 +84,7 @@ require(["jquery"], function ($) { // turn progress bar red progressBar.addClass('progress-bar-danger'); // open event log for debugging - $('progress-details').prop('open', true); + $('#progress-details').prop('open', true); } } From c9e12182a2eda0c908254e0c45ef192a3ede915e Mon Sep 17 00:00:00 2001 From: Min RK Date: Wed, 4 Apr 2018 15:26:27 +0200 Subject: [PATCH 04/26] halt progress iteration on completed spawn requires calling `__aiter__` and `__anext__` instead of `async for` --- jupyterhub/apihandlers/users.py | 43 +++++++++++++++++++++-------- jupyterhub/spawner.py | 48 ++++++++++++++++++--------------- 2 files changed, 58 insertions(+), 33 deletions(-) diff --git a/jupyterhub/apihandlers/users.py b/jupyterhub/apihandlers/users.py index a1f5e9f3..1598770a 100644 --- a/jupyterhub/apihandlers/users.py +++ b/jupyterhub/apihandlers/users.py @@ -3,6 +3,7 @@ # Copyright (c) Jupyter Development Team. # Distributed under the terms of the Modified BSD License. +import asyncio import json from tornado import web @@ -324,10 +325,12 @@ class SpawnProgressAPIHandler(APIHandler): await self.send_event(ready_event) return + spawn_future = spawner._spawn_future + if not spawner._spawn_pending: # not pending, no progress to fetch # check if spawner has just failed - f = spawner._spawn_future + f = spawn_future if f and f.done() and f.exception(): failed_event['message'] = "Spawn failed: %s" % f.exception() await self.send_event(failed_event) @@ -336,22 +339,40 @@ class SpawnProgressAPIHandler(APIHandler): raise web.HTTPError(400, "%s is not starting...", spawner._log_name) # retrieve progress events from the Spawner - async for event in spawner._generate_progress(): - # don't allow events to sneakily set the 'ready flag' + progress_iter = spawner._generate_progress().__aiter__() + while True: + event_future = asyncio.ensure_future(progress_iter.__anext__()) + await asyncio.wait( + [event_future, spawn_future], + return_when=asyncio.FIRST_COMPLETED) + if event_future.done(): + try: + event = event_future.result() + except StopAsyncIteration: + break + elif spawn_future.done(): + # spawn is done *and* event is not ready + # cancel event future to avoid warnings about + # unawaited tasks + if not event_future.cancelled(): + event_future.cancel() + break + else: + # neither is done, this shouldn't be possible + continue + + # don't allow events to sneakily set the 'ready' flag if 'ready' in event: event.pop('ready', None) await self.send_event(event) - # events finished, check if we are still pending + # progress finished, check if we are still pending if spawner._spawn_pending: - try: - await spawner._spawn_future - except Exception: - # suppress exceptions in spawn future, - # which will be logged elsewhere - pass + # wait for spawn_future to complete + # (ignore errors, which will be logged elsewhere) + await asyncio.wait(spawn_future) - # progress finished, check if we are done + # progress and spawn finished, check if spawn succeeded if spawner.ready: # spawner is ready, signal completion and redirect self.log.info("Server %s is ready", spawner._log_name) diff --git a/jupyterhub/spawner.py b/jupyterhub/spawner.py index 01df8183..8453915a 100644 --- a/jupyterhub/spawner.py +++ b/jupyterhub/spawner.py @@ -5,6 +5,7 @@ Contains base Spawner class & default implementation # Copyright (c) Jupyter Development Team. # Distributed under the terms of the Modified BSD License. +import asyncio import errno import json import os @@ -17,11 +18,10 @@ from subprocess import Popen from tempfile import mkdtemp # FIXME: remove when we drop Python 3.5 support -from async_generator import isasyncgenfunction, async_generator, yield_ +from async_generator import async_generator, yield_ from sqlalchemy import inspect -from tornado import gen, concurrent from tornado.ioloop import PeriodicCallback from traitlets.config import LoggingConfigurable @@ -697,35 +697,36 @@ class Spawner(LoggingConfigurable): """Private wrapper of progress generator This method is always an async generator and will always yield at least one event. - - Calls self._default_progress if self.progress is not an async generator """ if not self._spawn_pending: raise RuntimeError("Spawn not pending, can't generate progress") + + spawn_future = self._spawn_future + await yield_({ "progress": 0, "message": "Server requested", }) - if isasyncgenfunction(self.progress): - progress = self.progress - else: - progress = self._default_progress - # TODO: stop when spawn is ready, even if progress isn't - async for event in progress(): - await yield_(event) + progress_iter = self.progress().__aiter__() + while True: + f = asyncio.ensure_future(progress_iter.__anext__()) + await asyncio.wait( + [f, spawn_future], + return_when=asyncio.FIRST_COMPLETED) + if f.done(): + try: + await yield_(f.result()) + except StopAsyncIteration: + break + elif spawn_future.done(): + # cancel event future to avoid warnings about + # unawaited tasks + if not f.cancelled(): + f.cancel() + break @async_generator - async def _default_progress(self): - """The default progress events generator - - Yields just one generic event for 50% progress - """ - await yield_({ - "progress": 50, - "message": "Spawning server...", - }) - async def progress(self): """Async generator for progress events @@ -749,7 +750,10 @@ class Spawner(LoggingConfigurable): .. versionadded:: 0.9 """ - pass + await yield_({ + "progress": 50, + "message": "Spawning server...", + }) async def start(self): """Start the single-user server From 707b300bd6c0147d5dba7fbe5d0bf7b27c161b5c Mon Sep 17 00:00:00 2001 From: Min RK Date: Thu, 5 Apr 2018 10:56:00 +0200 Subject: [PATCH 05/26] add iterate_until utility allows iterating through an async generator, yielding items until another Future resolves if/when that deadline Future resolves, ready items will continue to be yielded until there is one that actually needs to wait at which point the iteration will halt --- jupyterhub/apihandlers/users.py | 25 ++------------- jupyterhub/spawner.py | 23 +++---------- jupyterhub/tests/test_utils.py | 57 +++++++++++++++++++++++++++++++++ jupyterhub/utils.py | 41 ++++++++++++++++++++++++ 4 files changed, 104 insertions(+), 42 deletions(-) create mode 100644 jupyterhub/tests/test_utils.py diff --git a/jupyterhub/apihandlers/users.py b/jupyterhub/apihandlers/users.py index 1598770a..069f02ad 100644 --- a/jupyterhub/apihandlers/users.py +++ b/jupyterhub/apihandlers/users.py @@ -10,7 +10,7 @@ from tornado import web from tornado.iostream import StreamClosedError from .. import orm -from ..utils import admin_only, maybe_future, url_path_join +from ..utils import admin_only, iterate_until, maybe_future, url_path_join from .base import APIHandler @@ -339,28 +339,7 @@ class SpawnProgressAPIHandler(APIHandler): raise web.HTTPError(400, "%s is not starting...", spawner._log_name) # retrieve progress events from the Spawner - progress_iter = spawner._generate_progress().__aiter__() - while True: - event_future = asyncio.ensure_future(progress_iter.__anext__()) - await asyncio.wait( - [event_future, spawn_future], - return_when=asyncio.FIRST_COMPLETED) - if event_future.done(): - try: - event = event_future.result() - except StopAsyncIteration: - break - elif spawn_future.done(): - # spawn is done *and* event is not ready - # cancel event future to avoid warnings about - # unawaited tasks - if not event_future.cancelled(): - event_future.cancel() - break - else: - # neither is done, this shouldn't be possible - continue - + async for event in iterate_until(spawn_future, spawner._generate_progress()): # don't allow events to sneakily set the 'ready' flag if 'ready' in event: event.pop('ready', None) diff --git a/jupyterhub/spawner.py b/jupyterhub/spawner.py index 8453915a..822617d9 100644 --- a/jupyterhub/spawner.py +++ b/jupyterhub/spawner.py @@ -32,7 +32,7 @@ from traitlets import ( from .objects import Server from .traitlets import Command, ByteSpecification, Callable -from .utils import maybe_future, random_port, url_path_join, exponential_backoff +from .utils import iterate_until, maybe_future, random_port, url_path_join, exponential_backoff class Spawner(LoggingConfigurable): @@ -706,25 +706,10 @@ class Spawner(LoggingConfigurable): await yield_({ "progress": 0, "message": "Server requested", - }) + }) - progress_iter = self.progress().__aiter__() - while True: - f = asyncio.ensure_future(progress_iter.__anext__()) - await asyncio.wait( - [f, spawn_future], - return_when=asyncio.FIRST_COMPLETED) - if f.done(): - try: - await yield_(f.result()) - except StopAsyncIteration: - break - elif spawn_future.done(): - # cancel event future to avoid warnings about - # unawaited tasks - if not f.cancelled(): - f.cancel() - break + async for event in iterate_until(spawn_future, self.progress()): + await yield_(event) @async_generator async def progress(self): diff --git a/jupyterhub/tests/test_utils.py b/jupyterhub/tests/test_utils.py new file mode 100644 index 00000000..d2f1bfa6 --- /dev/null +++ b/jupyterhub/tests/test_utils.py @@ -0,0 +1,57 @@ +"""Tests for utilities""" + +import asyncio +import pytest + +from async_generator import async_generator, 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) + + +def schedule_future(io_loop, *, delay, result=None): + """Construct a Future that will resolve after a delay""" + f = asyncio.Future() + if delay: + io_loop.call_later(delay, lambda: f.set_result(result)) + else: + f.set_result(result) + return f + + +@pytest.mark.gen_test +@pytest.mark.parametrize("deadline, n, delay, expected", [ + (0, 3, 1, []), + (0, 3, 0, [0, 1, 2]), + (5, 3, 0.01, [0, 1, 2]), + (0.5, 10, 0.2, [0, 1]), +]) +async def test_iterate_until(io_loop, deadline, n, delay, expected): + f = schedule_future(io_loop, delay=deadline) + + yielded = [] + async for item in iterate_until(f, yield_n(n, delay=delay)): + yielded.append(item) + assert yielded == expected + + +@pytest.mark.gen_test +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) + + yielded = [] + async for item in iterate_until(f, gen()): + yielded.append(item) + assert yielded == list(range(5)) diff --git a/jupyterhub/utils.py b/jupyterhub/utils.py index 6545ba6d..4745aabb 100644 --- a/jupyterhub/utils.py +++ b/jupyterhub/utils.py @@ -19,6 +19,7 @@ import threading import uuid import warnings +from async_generator import async_generator, yield_ from tornado import gen, ioloop, web from tornado.platform.asyncio import to_asyncio_future from tornado.httpclient import AsyncHTTPClient, HTTPError @@ -445,3 +446,43 @@ def maybe_future(obj): return asyncio.wrap_future(obj) else: return to_asyncio_future(gen.maybe_future(obj)) + + +@async_generator +async def iterate_until(deadline_future, generator): + """An async generator that yields items from a generator + until a deadline future resolves + + This could *almost* be implemented as a context manager + like asyncio_timeout with a Future for the cutoff. + + However, we want one distinction: continue yielding items + after the future is complete, as long as the are already finished. + + Usage:: + + async for item in iterate_until(some_future, some_async_generator()): + print(item) + + """ + aiter = generator.__aiter__() + while True: + item_future = asyncio.ensure_future(aiter.__anext__()) + await asyncio.wait( + [item_future, deadline_future], + return_when=asyncio.FIRST_COMPLETED) + if item_future.done(): + try: + await yield_(item_future.result()) + except StopAsyncIteration: + break + elif deadline_future.done(): + # deadline is done *and* next item is not ready + # cancel item future to avoid warnings about + # unawaited tasks + if not item_future.cancelled(): + item_future.cancel() + break + else: + # neither is done, this shouldn't happen + continue From ee1a86d192435dc652c4237f11be2e497abcbb45 Mon Sep 17 00:00:00 2001 From: Min RK Date: Thu, 5 Apr 2018 11:05:53 +0200 Subject: [PATCH 06/26] progress url is at server/progress instead of server-progress --- jupyterhub/apihandlers/users.py | 4 ++-- jupyterhub/handlers/base.py | 10 ++++++---- 2 files changed, 8 insertions(+), 6 deletions(-) diff --git a/jupyterhub/apihandlers/users.py b/jupyterhub/apihandlers/users.py index 069f02ad..7da87562 100644 --- a/jupyterhub/apihandlers/users.py +++ b/jupyterhub/apihandlers/users.py @@ -372,9 +372,9 @@ default_handlers = [ (r"/api/user", SelfAPIHandler), (r"/api/users", UserListAPIHandler), (r"/api/users/([^/]+)", UserAPIHandler), - (r"/api/users/([^/]+)/server-progress", SpawnProgressAPIHandler), - (r"/api/users/([^/]+)/server-progress/([^/]*)", SpawnProgressAPIHandler), (r"/api/users/([^/]+)/server", UserServerAPIHandler), + (r"/api/users/([^/]+)/server/progress", SpawnProgressAPIHandler), (r"/api/users/([^/]+)/servers/([^/]*)", UserServerAPIHandler), + (r"/api/users/([^/]+)/servers/([^/]*)/progress", SpawnProgressAPIHandler), (r"/api/users/([^/]+)/admin-access", UserAdminAccessAPIHandler), ] diff --git a/jupyterhub/handlers/base.py b/jupyterhub/handlers/base.py index 7e61fc65..769f504c 100644 --- a/jupyterhub/handlers/base.py +++ b/jupyterhub/handlers/base.py @@ -887,10 +887,12 @@ class UserSpawnHandler(BaseHandler): pass # we may have waited above, check pending again: - progress_url = url_path_join( - self.hub.base_url, 'api/users', - user.escaped_name, 'server-progress', spawner.name, - ) + url_parts = [self.hub.base_url, 'api/users', user.escaped_name] + if spawner.name: + url_parts.extend(['servers', spawner.name, 'progress']) + else: + url_parts.extend(['server/progress']) + progress_url = url_path_join(*url_parts) if spawner.pending: self.log.info("%s is pending %s", spawner._log_name, spawner.pending) From 6f8a34127bf53b90bcf8e9cf7e85642a6fff9db9 Mon Sep 17 00:00:00 2001 From: Min RK Date: Thu, 5 Apr 2018 11:20:16 +0200 Subject: [PATCH 07/26] consolidate progress url and include it in server models --- jupyterhub/apihandlers/base.py | 38 ++++++++++++++++++++-------------- jupyterhub/handlers/base.py | 11 ++-------- jupyterhub/spawner.py | 4 ++++ jupyterhub/user.py | 9 ++++++++ 4 files changed, 37 insertions(+), 25 deletions(-) diff --git a/jupyterhub/apihandlers/base.py b/jupyterhub/apihandlers/base.py index a34e2088..5d63f532 100644 --- a/jupyterhub/apihandlers/base.py +++ b/jupyterhub/apihandlers/base.py @@ -97,6 +97,22 @@ class APIHandler(BaseHandler): 'message': message or status_message, })) + def server_model(self, spawner): + """Get the JSON model for a Spawner""" + last_activity = spawner.orm_spawner.last_activity + # don't call isoformat if last_activity is None + if last_activity: + last_activity = isoformat(last_activity) + + return { + 'name': spawner.name, + 'last_activity': last_activity, + 'started': isoformat(spawner.orm_spawner.started), + 'pending': spawner.pending, + 'url': url_path_join(spawner.user.url, spawner.name, '/'), + 'progress_url': spawner._progress_url, + } + def user_model(self, user): """Get the JSON model for a User object""" if isinstance(user, orm.User): @@ -114,33 +130,23 @@ class APIHandler(BaseHandler): 'admin': user.admin, 'groups': [ g.name for g in user.groups ], 'server': user.url if user.running else None, + 'progress_url': user.spawner._progress_url if user.active else None, 'pending': None, 'created': isoformat(user.created), 'last_activity': last_activity, 'started': None, } if '' in user.spawners: - spawner = user.spawners[''] - model['pending'] = spawner.pending or None - if spawner.active and spawner.orm_spawner.started: - model['started'] = isoformat(spawner.orm_spawner.started) + server_model = self.server_model(user.spawners['']) + # copy some values from the default server to the user model + for key in ('started', 'pending', 'progress_url'): + model[key] = server_model[key] if self.allow_named_servers: servers = model['servers'] = {} for name, spawner in user.spawners.items(): if spawner.ready: - last_activity = spawner.orm_spawner.last_activity - if last_activity: - last_activity = isoformat(last_activity) - servers[name] = s = { - 'name': name, - 'last_activity': last_activity, - 'started': isoformat(spawner.orm_spawner.started), - } - if spawner.pending: - s['pending'] = spawner.pending - if spawner.server: - s['url'] = url_path_join(user.url, name, '/') + servers[name] = self.server_model(spawner) return model def group_model(self, group): diff --git a/jupyterhub/handlers/base.py b/jupyterhub/handlers/base.py index 769f504c..764db373 100644 --- a/jupyterhub/handlers/base.py +++ b/jupyterhub/handlers/base.py @@ -887,13 +887,6 @@ class UserSpawnHandler(BaseHandler): pass # we may have waited above, check pending again: - url_parts = [self.hub.base_url, 'api/users', user.escaped_name] - if spawner.name: - url_parts.extend(['servers', spawner.name, 'progress']) - else: - url_parts.extend(['server/progress']) - progress_url = url_path_join(*url_parts) - if spawner.pending: self.log.info("%s is pending %s", spawner._log_name, spawner.pending) # spawn has started, but not finished @@ -902,7 +895,7 @@ class UserSpawnHandler(BaseHandler): html = self.render_template( "spawn_pending.html", user=user, - progress_url=progress_url, + progress_url=spawner._progress_url, ) self.finish(html) return @@ -934,7 +927,7 @@ class UserSpawnHandler(BaseHandler): html = self.render_template( "spawn_pending.html", user=user, - progress_url=progress_url, + progress_url=spawner._progress_url, ) self.finish(html) return diff --git a/jupyterhub/spawner.py b/jupyterhub/spawner.py index 822617d9..73182c22 100644 --- a/jupyterhub/spawner.py +++ b/jupyterhub/spawner.py @@ -692,6 +692,10 @@ class Spawner(LoggingConfigurable): if self.pre_spawn_hook: return self.pre_spawn_hook(self) + @property + def _progress_url(self): + return self.user.progress_url(self.name) + @async_generator async def _generate_progress(self): """Private wrapper of progress generator diff --git a/jupyterhub/user.py b/jupyterhub/user.py index 513c74f7..917817ae 100644 --- a/jupyterhub/user.py +++ b/jupyterhub/user.py @@ -320,6 +320,15 @@ class User: else: return self.base_url + def progress_url(self, server_name=''): + """API URL for progress endpoint for a server with a given name""" + url_parts = [self.settings['hub'].base_url, 'api/users', self.escaped_name] + if server_name: + url_parts.extend(['servers', server_name, 'progress']) + else: + url_parts.extend(['server/progress']) + return url_path_join(*url_parts) + async def spawn(self, server_name='', options=None): """Start the user's spawner From 31d3f7a20baa8c961e026e4df5aeddeb75a7dddb Mon Sep 17 00:00:00 2001 From: Min RK Date: Thu, 5 Apr 2018 11:31:17 +0200 Subject: [PATCH 08/26] allow isoformat(None) simplifies "if timestamp is None" cases when we are just using it to serialize nullable timestamps to JSON --- jupyterhub/apihandlers/base.py | 15 ++------------- jupyterhub/utils.py | 4 ++++ 2 files changed, 6 insertions(+), 13 deletions(-) diff --git a/jupyterhub/apihandlers/base.py b/jupyterhub/apihandlers/base.py index 5d63f532..536dbbff 100644 --- a/jupyterhub/apihandlers/base.py +++ b/jupyterhub/apihandlers/base.py @@ -99,14 +99,9 @@ class APIHandler(BaseHandler): def server_model(self, spawner): """Get the JSON model for a Spawner""" - last_activity = spawner.orm_spawner.last_activity - # don't call isoformat if last_activity is None - if last_activity: - last_activity = isoformat(last_activity) - return { 'name': spawner.name, - 'last_activity': last_activity, + 'last_activity': isoformat(spawner.orm_spawner.last_activity), 'started': isoformat(spawner.orm_spawner.started), 'pending': spawner.pending, 'url': url_path_join(spawner.user.url, spawner.name, '/'), @@ -118,12 +113,6 @@ class APIHandler(BaseHandler): if isinstance(user, orm.User): user = self.users[user.id] - - last_activity = user.last_activity - # don't call isoformat if last_activity is None - if last_activity: - last_activity = isoformat(last_activity) - model = { 'kind': 'user', 'name': user.name, @@ -133,8 +122,8 @@ class APIHandler(BaseHandler): 'progress_url': user.spawner._progress_url if user.active else None, 'pending': None, 'created': isoformat(user.created), - 'last_activity': last_activity, 'started': None, + 'last_activity': isoformat(user.last_activity), } if '' in user.spawners: server_model = self.server_model(user.spawners['']) diff --git a/jupyterhub/utils.py b/jupyterhub/utils.py index 4745aabb..cf158ad4 100644 --- a/jupyterhub/utils.py +++ b/jupyterhub/utils.py @@ -45,6 +45,10 @@ def isoformat(dt): Naïve datetime objects are assumed to be UTC """ + # allow null timestamps to remain None without + # having to check if isoformat should be called + if dt is None: + return None if dt.tzinfo: dt = dt.astimezone(timezone.utc).replace(tzinfo=None) return dt.isoformat() + 'Z' From 97cdb1a5d8e8e832575b35214da12858d6154e8c Mon Sep 17 00:00:00 2001 From: Min RK Date: Thu, 5 Apr 2018 11:43:39 +0200 Subject: [PATCH 09/26] handle progress_url in user model tests --- jupyterhub/apihandlers/base.py | 6 +++--- jupyterhub/tests/test_api.py | 8 ++++++++ jupyterhub/tests/test_named_servers.py | 13 ++++++------- 3 files changed, 17 insertions(+), 10 deletions(-) diff --git a/jupyterhub/apihandlers/base.py b/jupyterhub/apihandlers/base.py index 536dbbff..7b622606 100644 --- a/jupyterhub/apihandlers/base.py +++ b/jupyterhub/apihandlers/base.py @@ -103,7 +103,7 @@ class APIHandler(BaseHandler): 'name': spawner.name, 'last_activity': isoformat(spawner.orm_spawner.last_activity), 'started': isoformat(spawner.orm_spawner.started), - 'pending': spawner.pending, + 'pending': spawner.pending or None, 'url': url_path_join(spawner.user.url, spawner.name, '/'), 'progress_url': spawner._progress_url, } @@ -119,7 +119,7 @@ class APIHandler(BaseHandler): 'admin': user.admin, 'groups': [ g.name for g in user.groups ], 'server': user.url if user.running else None, - 'progress_url': user.spawner._progress_url if user.active else None, + 'progress_url': user.progress_url(''), 'pending': None, 'created': isoformat(user.created), 'started': None, @@ -128,7 +128,7 @@ class APIHandler(BaseHandler): if '' in user.spawners: server_model = self.server_model(user.spawners['']) # copy some values from the default server to the user model - for key in ('started', 'pending', 'progress_url'): + for key in ('started', 'pending'): model[key] = server_model[key] if self.allow_named_servers: diff --git a/jupyterhub/tests/test_api.py b/jupyterhub/tests/test_api.py index 5123236d..defc5e28 100644 --- a/jupyterhub/tests/test_api.py +++ b/jupyterhub/tests/test_api.py @@ -201,10 +201,17 @@ def normalize_user(user): """ for key in ('created', 'last_activity', 'started'): user[key] = normalize_timestamp(user[key]) + if user['progress_url']: + user['progress_url'] = re.sub( + r'.*/hub/api', + 'PREFIX/hub/api', + user['progress_url'], + ) if 'servers' in user: for server in user['servers'].values(): for key in ('started', 'last_activity'): server[key] = normalize_timestamp(server[key]) + server['progress_url'] = re.sub(r'.*/hub/api', 'PREFIX/hub/api', server['progress_url']) return user def fill_user(model): @@ -221,6 +228,7 @@ def fill_user(model): model.setdefault('created', TIMESTAMP) model.setdefault('last_activity', TIMESTAMP) model.setdefault('started', None) + model.setdefault('progress_url', 'PREFIX/hub/api/users/{name}/server/progress'.format(**model)) return model diff --git a/jupyterhub/tests/test_named_servers.py b/jupyterhub/tests/test_named_servers.py index 78b1aa51..f6c1e757 100644 --- a/jupyterhub/tests/test_named_servers.py +++ b/jupyterhub/tests/test_named_servers.py @@ -41,6 +41,8 @@ def test_default_server(app, named_servers): 'started': TIMESTAMP, 'last_activity': TIMESTAMP, 'url': user.url, + 'pending': None, + 'progress_url': 'PREFIX/hub/api/users/{}/server/progress'.format(username), }, }, }) @@ -96,6 +98,9 @@ def test_create_named_server(app, named_servers): 'started': TIMESTAMP, 'last_activity': TIMESTAMP, 'url': url_path_join(user.url, name, '/'), + 'pending': None, + 'progress_url': 'PREFIX/hub/api/users/{}/servers/{}/progress'.format( + username, servername), } for name in [servername] }, @@ -124,13 +129,7 @@ def test_delete_named_server(app, named_servers): assert user_model == fill_user({ 'name': username, 'auth_state': None, - 'servers': { - name: { - 'name': name, - 'url': url_path_join(user.url, name, '/'), - } - for name in [] - }, + 'servers': {}, }) @pytest.mark.gen_test From beedc9417902a00626b599d247478bb81284a4ec Mon Sep 17 00:00:00 2001 From: Min RK Date: Mon, 9 Apr 2018 10:32:28 +0200 Subject: [PATCH 10/26] delete the spinner no need for a spinner when we have a progress bar --- share/jupyterhub/templates/spawn_pending.html | 3 --- 1 file changed, 3 deletions(-) diff --git a/share/jupyterhub/templates/spawn_pending.html b/share/jupyterhub/templates/spawn_pending.html index 1ec87a73..29f73ad7 100644 --- a/share/jupyterhub/templates/spawn_pending.html +++ b/share/jupyterhub/templates/spawn_pending.html @@ -9,7 +9,6 @@

Your server is starting up.

You will be redirected automatically when it's ready for you.

{% endblock %} -

0% Complete @@ -79,8 +78,6 @@ require(["jquery"], function ($) { if (evt.failed) { evtSource.close(); - // stop spinner - $(".fa-pulse").removeClass("fa-pulse"); // turn progress bar red progressBar.addClass('progress-bar-danger'); // open event log for debugging From a3ed3874559cfa551a2f0a1f915496210ed22f4f Mon Sep 17 00:00:00 2001 From: Min RK Date: Mon, 9 Apr 2018 10:38:02 +0200 Subject: [PATCH 11/26] move get_content_type up one level to BaseHandler so all handlers get it --- jupyterhub/apihandlers/base.py | 7 ------- jupyterhub/handlers/base.py | 5 +++++ 2 files changed, 5 insertions(+), 7 deletions(-) diff --git a/jupyterhub/apihandlers/base.py b/jupyterhub/apihandlers/base.py index 7b622606..b3252173 100644 --- a/jupyterhub/apihandlers/base.py +++ b/jupyterhub/apihandlers/base.py @@ -18,13 +18,6 @@ class APIHandler(BaseHandler): def content_security_policy(self): return '; '.join([super().content_security_policy, "default-src 'none'"]) - def get_content_type(self): - return 'application/json' - - def set_default_headers(self): - super().set_default_headers() - self.set_header('Content-Type', self.get_content_type()) - def check_referer(self): """Check Origin for cross-site API requests. diff --git a/jupyterhub/handlers/base.py b/jupyterhub/handlers/base.py index 764db373..c25ffcd5 100644 --- a/jupyterhub/handlers/base.py +++ b/jupyterhub/handlers/base.py @@ -135,11 +135,15 @@ class BaseHandler(RequestHandler): "report-uri " + self.csp_report_uri, ]) + def get_content_type(self): + return 'text/html' + def set_default_headers(self): """ Set any headers passed as tornado_settings['headers']. By default sets Content-Security-Policy of frame-ancestors 'self'. + Also responsible for setting content-type header """ # wrap in HTTPHeaders for case-insensitivity headers = HTTPHeaders(self.settings.get('headers', {})) @@ -152,6 +156,7 @@ class BaseHandler(RequestHandler): self.set_header('Access-Control-Allow-Headers', 'accept, content-type, authorization') if 'Content-Security-Policy' not in headers: self.set_header('Content-Security-Policy', self.content_security_policy) + self.set_header('Content-Type', self.get_content_type()) #--------------------------------------------------------------- # Login and cookie-related From c0f37c48a13e92e719dfa201b982748215a9d19e Mon Sep 17 00:00:00 2001 From: Min RK Date: Mon, 9 Apr 2018 11:16:17 +0200 Subject: [PATCH 12/26] fix wait for spawn future asyncio.wait takes a list --- jupyterhub/apihandlers/users.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/jupyterhub/apihandlers/users.py b/jupyterhub/apihandlers/users.py index 7da87562..6811bf38 100644 --- a/jupyterhub/apihandlers/users.py +++ b/jupyterhub/apihandlers/users.py @@ -349,7 +349,7 @@ class SpawnProgressAPIHandler(APIHandler): if spawner._spawn_pending: # wait for spawn_future to complete # (ignore errors, which will be logged elsewhere) - await asyncio.wait(spawn_future) + await asyncio.wait([spawn_future]) # progress and spawn finished, check if spawn succeeded if spawner.ready: From e56d416210c303275cec3a118b0075c5b5847a65 Mon Sep 17 00:00:00 2001 From: Min RK Date: Mon, 9 Apr 2018 11:38:29 +0200 Subject: [PATCH 13/26] Don't delete failed spawners They preserve error messages that are useful only delete spawners that shutdown cleanly --- jupyterhub/user.py | 6 ++++-- 1 file changed, 4 insertions(+), 2 deletions(-) diff --git a/jupyterhub/user.py b/jupyterhub/user.py index 917817ae..de0b613a 100644 --- a/jupyterhub/user.py +++ b/jupyterhub/user.py @@ -554,5 +554,7 @@ class User: except Exception: self.log.exception("Error in Authenticator.post_spawn_stop for %s", self) spawner._stop_pending = False - # pop the Spawner object - self.spawners.pop(server_name) + if not (spawner._spawn_future and spawner._spawn_future.exception()): + # pop Spawner *unless* it's stopping due to an error + # because some pages serve latest-spawn error messages + self.spawners.pop(server_name) From d30e62a205cb367f9d827febc22a4f47549f2efa Mon Sep 17 00:00:00 2001 From: Min RK Date: Mon, 9 Apr 2018 11:39:07 +0200 Subject: [PATCH 14/26] test spawn progress --- jupyterhub/tests/test_api.py | 157 ++++++++++++++++++++++++++++++++++- 1 file changed, 156 insertions(+), 1 deletion(-) diff --git a/jupyterhub/tests/test_api.py b/jupyterhub/tests/test_api.py index defc5e28..b63489f9 100644 --- a/jupyterhub/tests/test_api.py +++ b/jupyterhub/tests/test_api.py @@ -8,9 +8,9 @@ import sys from unittest import mock from urllib.parse import urlparse, quote import uuid +from async_generator import async_generator, yield_ from pytest import mark - from tornado import gen import jupyterhub @@ -684,6 +684,161 @@ def test_slow_bad_spawn(app, no_patience, slow_bad_spawn): assert app.users.count_active_users()['pending'] == 0 +def next_event(it): + """read an event from an eventstream""" + while True: + try: + line = next(it) + except StopIteration: + return + if line.startswith('data:'): + return json.loads(line.split(':', 1)[1]) + + +@mark.gen_test +def test_spawn_progress(request, app, no_patience, slow_spawn): + db = app.db + name = 'martin' + app_user = add_user(db, app=app, name=name) + r = yield api_request(app, 'users', name, 'server', method='post') + r.raise_for_status() + r = yield api_request(app, 'users', name, 'server/progress', stream=True) + r.raise_for_status() + request.addfinalizer(r.close) + ex = async_requests.executor + line_iter = iter(r.iter_lines(decode_unicode=True)) + evt = yield ex.submit(next_event, line_iter) + assert evt == { + 'progress': 0, + 'message': 'Server requested', + } + evt = yield ex.submit(next_event, line_iter) + assert evt == { + 'progress': 50, + 'message': 'Spawning server...', + } + evt = yield ex.submit(next_event, line_iter) + url = app_user.url + assert evt == { + 'progress': 100, + 'message': 'Server ready at {}'.format(url), + 'html_message': 'Server ready at {0}'.format(url), + 'url': url, + 'ready': True, + } + + +@mark.gen_test +def test_spawn_progress_not_started(request, app): + db = app.db + name = 'nope' + app_user = add_user(db, app=app, name=name) + r = yield api_request(app, 'users', name, 'server', method='post') + r.raise_for_status() + r = yield api_request(app, 'users', name, 'server', method='delete') + r.raise_for_status() + r = yield api_request(app, 'users', name, 'server/progress') + assert r.status_code == 404 + + +@mark.gen_test +def test_spawn_progress_not_found(request, app): + db = app.db + name = 'noserver' + r = yield api_request(app, 'users', 'nosuchuser', 'server/progress') + assert r.status_code == 404 + app_user = add_user(db, app=app, name=name) + r = yield api_request(app, 'users', name, 'server/progress') + assert r.status_code == 404 + + +@mark.gen_test +def test_spawn_progress_ready(request, app): + """Test progress API when spawner is already started + + e.g. a race between requesting progress and progress already being complete + """ + db = app.db + name = 'saga' + app_user = add_user(db, app=app, name=name) + r = yield api_request(app, 'users', name, 'server', method='post') + r.raise_for_status() + r = yield api_request(app, 'users', name, 'server/progress', stream=True) + r.raise_for_status() + request.addfinalizer(r.close) + ex = async_requests.executor + line_iter = iter(r.iter_lines(decode_unicode=True)) + evt = yield ex.submit(next_event, line_iter) + assert evt['progress'] == 100 + assert evt['ready'] + assert evt['url'] == app_user.url + + +@mark.gen_test +def test_spawn_progress_bad(request, app, no_patience, bad_spawn): + """Test progress API when spawner is already started + + e.g. a race between requesting progress and progress already being complete + """ + db = app.db + name = 'simon' + app_user = add_user(db, app=app, name=name) + r = yield api_request(app, 'users', name, 'server', method='post') + assert r.status_code == 500 + r = yield api_request(app, 'users', name, 'server/progress', stream=True) + r.raise_for_status() + request.addfinalizer(r.close) + ex = async_requests.executor + line_iter = iter(r.iter_lines(decode_unicode=True)) + evt = yield ex.submit(next_event, line_iter) + assert evt == { + 'progress': 100, + 'failed': True, + 'message': "Spawn failed: I don't work!", + } + + +@mark.gen_test +def test_spawn_progress_cutoff(request, app, no_patience, slow_spawn): + """Progress events stop when Spawner finishes + + even if progress iterator is still going. + """ + + @async_generator + 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) + + db = app.db + name = 'geddy' + app_user = add_user(db, app=app, name=name) + app_user.spawner.progress = progress_forever + app_user.spawner.delay = 1 + + r = yield api_request(app, 'users', name, 'server', method='post') + r.raise_for_status() + r = yield api_request(app, 'users', name, 'server/progress', stream=True) + r.raise_for_status() + request.addfinalizer(r.close) + ex = async_requests.executor + line_iter = iter(r.iter_lines(decode_unicode=True)) + evt = yield ex.submit(next_event, line_iter) + assert evt['progress'] == 0 + evt = yield ex.submit(next_event, line_iter) + assert evt == { + 'progress': 1, + 'message': 'Stage 1', + } + evt = yield ex.submit(next_event, line_iter) + assert evt['progress'] == 100 + + @mark.gen_test def test_spawn_limit(app, no_patience, slow_spawn, request): db = app.db From a1891968555ce26c0a18f6cd44b5044181fe2388 Mon Sep 17 00:00:00 2001 From: Min RK Date: Mon, 9 Apr 2018 13:03:18 +0200 Subject: [PATCH 15/26] ensure async generators are properly closed only terminate with iterate_until in handler, not Spawner._generate_events --- jupyterhub/apihandlers/users.py | 12 +++++---- jupyterhub/spawner.py | 8 +++--- jupyterhub/tests/test_utils.py | 12 +++++---- jupyterhub/utils.py | 47 ++++++++++++++++++--------------- 4 files changed, 44 insertions(+), 35 deletions(-) diff --git a/jupyterhub/apihandlers/users.py b/jupyterhub/apihandlers/users.py index 6811bf38..5b2bfd4c 100644 --- a/jupyterhub/apihandlers/users.py +++ b/jupyterhub/apihandlers/users.py @@ -6,6 +6,7 @@ import asyncio import json +from async_generator import aclosing from tornado import web from tornado.iostream import StreamClosedError @@ -339,11 +340,12 @@ class SpawnProgressAPIHandler(APIHandler): raise web.HTTPError(400, "%s is not starting...", spawner._log_name) # retrieve progress events from the Spawner - async for event in iterate_until(spawn_future, spawner._generate_progress()): - # don't allow events to sneakily set the 'ready' flag - if 'ready' in event: - event.pop('ready', None) - await self.send_event(event) + async with aclosing(iterate_until(spawn_future, spawner._generate_progress())) as events: + async for event in events: + # don't allow events to sneakily set the 'ready' flag + if 'ready' in event: + event.pop('ready', None) + await self.send_event(event) # progress finished, check if we are still pending if spawner._spawn_pending: diff --git a/jupyterhub/spawner.py b/jupyterhub/spawner.py index 73182c22..5284b58e 100644 --- a/jupyterhub/spawner.py +++ b/jupyterhub/spawner.py @@ -705,15 +705,15 @@ class Spawner(LoggingConfigurable): if not self._spawn_pending: raise RuntimeError("Spawn not pending, can't generate progress") - spawn_future = self._spawn_future - await yield_({ "progress": 0, "message": "Server requested", }) + from async_generator import aclosing - async for event in iterate_until(spawn_future, self.progress()): - await yield_(event) + async with aclosing(self.progress()) as progress: + async for event in progress: + await yield_(event) @async_generator async def progress(self): diff --git a/jupyterhub/tests/test_utils.py b/jupyterhub/tests/test_utils.py index d2f1bfa6..418f4795 100644 --- a/jupyterhub/tests/test_utils.py +++ b/jupyterhub/tests/test_utils.py @@ -3,7 +3,7 @@ import asyncio import pytest -from async_generator import async_generator, yield_ +from async_generator import aclosing, async_generator, yield_ from ..utils import iterate_until @@ -37,8 +37,9 @@ async def test_iterate_until(io_loop, deadline, n, delay, expected): f = schedule_future(io_loop, delay=deadline) yielded = [] - async for item in iterate_until(f, yield_n(n, delay=delay)): - yielded.append(item) + async with aclosing(iterate_until(f, yield_n(n, delay=delay))) as items: + async for item in items: + yielded.append(item) assert yielded == expected @@ -52,6 +53,7 @@ async def test_iterate_until_ready_after_deadline(io_loop): await yield_(i) yielded = [] - async for item in iterate_until(f, gen()): - yielded.append(item) + async with aclosing(iterate_until(f, gen())) as items: + async for item in items: + yielded.append(item) assert yielded == list(range(5)) diff --git a/jupyterhub/utils.py b/jupyterhub/utils.py index cf158ad4..dabe96fd 100644 --- a/jupyterhub/utils.py +++ b/jupyterhub/utils.py @@ -19,7 +19,7 @@ import threading import uuid import warnings -from async_generator import async_generator, yield_ +from async_generator import aclosing, async_generator, yield_ from tornado import gen, ioloop, web from tornado.platform.asyncio import to_asyncio_future from tornado.httpclient import AsyncHTTPClient, HTTPError @@ -469,24 +469,29 @@ async def iterate_until(deadline_future, generator): print(item) """ - aiter = generator.__aiter__() - while True: - item_future = asyncio.ensure_future(aiter.__anext__()) - await asyncio.wait( - [item_future, deadline_future], - return_when=asyncio.FIRST_COMPLETED) - if item_future.done(): - try: - await yield_(item_future.result()) - except StopAsyncIteration: + async with aclosing(generator.__aiter__()) as aiter: + while True: + item_future = asyncio.ensure_future(aiter.__anext__()) + await asyncio.wait( + [item_future, deadline_future], + return_when=asyncio.FIRST_COMPLETED) + if item_future.done(): + try: + await yield_(item_future.result()) + except (StopAsyncIteration, asyncio.CancelledError): + break + elif deadline_future.done(): + # deadline is done *and* next item is not ready + # cancel item future to avoid warnings about + # unawaited tasks + if not item_future.cancelled(): + item_future.cancel() + # resolve cancellation to avoid garbage collection issues + try: + await item_future + except asyncio.CancelledError: + pass break - elif deadline_future.done(): - # deadline is done *and* next item is not ready - # cancel item future to avoid warnings about - # unawaited tasks - if not item_future.cancelled(): - item_future.cancel() - break - else: - # neither is done, this shouldn't happen - continue + else: + # neither is done, this shouldn't happen + continue From 6d6e48f434ec86e40e229c4173ab0a4e54c1de8c Mon Sep 17 00:00:00 2001 From: Min RK Date: Mon, 9 Apr 2018 13:03:29 +0200 Subject: [PATCH 16/26] test native async generator on Python 3.6 --- jupyterhub/tests/test_api.py | 45 ++++++++++++++++++++++++++---------- 1 file changed, 33 insertions(+), 12 deletions(-) diff --git a/jupyterhub/tests/test_api.py b/jupyterhub/tests/test_api.py index b63489f9..32ea9f7f 100644 --- a/jupyterhub/tests/test_api.py +++ b/jupyterhub/tests/test_api.py @@ -798,27 +798,48 @@ def test_spawn_progress_bad(request, app, no_patience, 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, + }) + # wait a long time before the next event + await gen.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 gen.sleep(10) +""", globals()) + + @mark.gen_test def test_spawn_progress_cutoff(request, app, no_patience, slow_spawn): """Progress events stop when Spawner finishes even if progress iterator is still going. """ - - @async_generator - 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) - db = app.db name = 'geddy' app_user = add_user(db, app=app, name=name) - app_user.spawner.progress = progress_forever + 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.delay = 1 r = yield api_request(app, 'users', name, 'server', method='post') From 2952f627265ad9156a941640a81fd9cf8f7c9768 Mon Sep 17 00:00:00 2001 From: Min RK Date: Mon, 9 Apr 2018 14:24:29 +0200 Subject: [PATCH 17/26] add cleanup_after fixture function-scoped fixture for shutting down servers avoids servers leaking into neighbor tests without having to teardown the app itself after every test --- jupyterhub/tests/conftest.py | 24 +++++++++++++++++++++++- 1 file changed, 23 insertions(+), 1 deletion(-) diff --git a/jupyterhub/tests/conftest.py b/jupyterhub/tests/conftest.py index 47d97650..d4e8f04c 100644 --- a/jupyterhub/tests/conftest.py +++ b/jupyterhub/tests/conftest.py @@ -3,11 +3,12 @@ # Copyright (c) Jupyter Development Team. # Distributed under the terms of the Modified BSD License. +import asyncio import os import logging from getpass import getuser from subprocess import TimeoutExpired -import time + from unittest import mock from pytest import fixture, raises from tornado import ioloop, gen @@ -25,6 +26,7 @@ import jupyterhub.services.service # global db session object _db = None + @fixture def db(): """Get a db session""" @@ -53,6 +55,26 @@ def io_loop(request): return io_loop +@fixture(autouse=True) +def cleanup_after(request): + """function-scoped fixture to shutdown user servers + + allows cleanup of servers between tests + without having to launch a whole new app + """ + try: + yield + finally: + if not MockHub.initialized(): + return + app = MockHub.instance() + loop = asyncio.new_event_loop() + for uid, user in app.users.items(): + for name, spawner in list(user.spawners.items()): + if spawner.active: + loop.run_until_complete(user.stop(name)) + + @fixture(scope='module') def app(request, io_loop): """Mock a jupyterhub app for testing""" From 307ad636dc62bee123aa3265d55edafc0ab4bcb9 Mon Sep 17 00:00:00 2001 From: Min RK Date: Mon, 9 Apr 2018 14:46:08 +0200 Subject: [PATCH 18/26] test spawner failure mid-progress --- jupyterhub/tests/test_api.py | 40 ++++++++++++++++++++++++++++-------- 1 file changed, 31 insertions(+), 9 deletions(-) diff --git a/jupyterhub/tests/test_api.py b/jupyterhub/tests/test_api.py index 32ea9f7f..0fde40a0 100644 --- a/jupyterhub/tests/test_api.py +++ b/jupyterhub/tests/test_api.py @@ -696,7 +696,7 @@ def next_event(it): @mark.gen_test -def test_spawn_progress(request, app, no_patience, slow_spawn): +def test_progress(request, app, no_patience, slow_spawn): db = app.db name = 'martin' app_user = add_user(db, app=app, name=name) @@ -729,7 +729,7 @@ def test_spawn_progress(request, app, no_patience, slow_spawn): @mark.gen_test -def test_spawn_progress_not_started(request, app): +def test_progress_not_started(request, app): db = app.db name = 'nope' app_user = add_user(db, app=app, name=name) @@ -742,7 +742,7 @@ def test_spawn_progress_not_started(request, app): @mark.gen_test -def test_spawn_progress_not_found(request, app): +def test_progress_not_found(request, app): db = app.db name = 'noserver' r = yield api_request(app, 'users', 'nosuchuser', 'server/progress') @@ -753,7 +753,7 @@ def test_spawn_progress_not_found(request, app): @mark.gen_test -def test_spawn_progress_ready(request, app): +def test_progress_ready(request, app): """Test progress API when spawner is already started e.g. a race between requesting progress and progress already being complete @@ -775,11 +775,8 @@ def test_spawn_progress_ready(request, app): @mark.gen_test -def test_spawn_progress_bad(request, app, no_patience, bad_spawn): - """Test progress API when spawner is already started - - e.g. a race between requesting progress and progress already being complete - """ +def test_progress_bad(request, app, no_patience, bad_spawn): + """Test progress API when spawner has already failed""" db = app.db name = 'simon' app_user = add_user(db, app=app, name=name) @@ -798,6 +795,31 @@ def test_spawn_progress_bad(request, app, no_patience, bad_spawn): } +@mark.gen_test +def test_progress_bad_slow(request, app, no_patience, slow_bad_spawn): + """Test progress API when spawner fails while watching""" + db = app.db + name = 'eugene' + app_user = add_user(db, app=app, name=name) + r = yield api_request(app, 'users', name, 'server', method='post') + assert r.status_code == 202 + r = yield api_request(app, 'users', name, 'server/progress', stream=True) + r.raise_for_status() + request.addfinalizer(r.close) + ex = async_requests.executor + line_iter = iter(r.iter_lines(decode_unicode=True)) + evt = yield ex.submit(next_event, line_iter) + assert evt['progress'] == 0 + evt = yield ex.submit(next_event, line_iter) + assert evt['progress'] == 50 + evt = yield ex.submit(next_event, line_iter) + assert evt == { + 'progress': 100, + 'failed': True, + 'message': "Spawn failed: I don't work!", + } + + @async_generator async def progress_forever(): """progress function that yields messages forever""" From 9b914e8f019f6df303b7d5b14d81ddb60e87b56e Mon Sep 17 00:00:00 2001 From: Min RK Date: Mon, 9 Apr 2018 14:46:28 +0200 Subject: [PATCH 19/26] fix waiting for spawner to fail in progress --- jupyterhub/apihandlers/users.py | 13 +++++-------- 1 file changed, 5 insertions(+), 8 deletions(-) diff --git a/jupyterhub/apihandlers/users.py b/jupyterhub/apihandlers/users.py index 5b2bfd4c..96e807e3 100644 --- a/jupyterhub/apihandlers/users.py +++ b/jupyterhub/apihandlers/users.py @@ -347,27 +347,24 @@ class SpawnProgressAPIHandler(APIHandler): event.pop('ready', None) await self.send_event(event) - # progress finished, check if we are still pending - if spawner._spawn_pending: - # wait for spawn_future to complete - # (ignore errors, which will be logged elsewhere) - await asyncio.wait([spawn_future]) + # progress finished, wait for spawn to actually resolve, + # in case progress finished early + # (ignore errors, which will be logged elsewhere) + await asyncio.wait([spawn_future]) # progress and spawn finished, check if spawn succeeded if spawner.ready: # spawner is ready, signal completion and redirect self.log.info("Server %s is ready", spawner._log_name) await self.send_event(ready_event) - return else: # what happened? Maybe spawn failed? - f = spawner._spawn_future + f = spawn_future if f and f.done() and f.exception(): failed_event['message'] = "Spawn failed: %s" % f.exception() else: self.log.warning("Server %s didn't start for unknown reason", spawner._log_name) await self.send_event(failed_event) - return default_handlers = [ From 955b769d3f87ca14f8c7284a5760c032b0707174 Mon Sep 17 00:00:00 2001 From: Min RK Date: Mon, 9 Apr 2018 14:47:34 +0200 Subject: [PATCH 20/26] add missing commits for deprecated ip/port consider for removal --- jupyterhub/spawner.py | 1 + jupyterhub/user.py | 1 + 2 files changed, 2 insertions(+) diff --git a/jupyterhub/spawner.py b/jupyterhub/spawner.py index 5284b58e..85feeaad 100644 --- a/jupyterhub/spawner.py +++ b/jupyterhub/spawner.py @@ -1096,6 +1096,7 @@ class LocalProcessSpawner(Spawner): if self.ip: self.server.ip = self.ip self.server.port = self.port + self.db.commit() return (self.ip or '127.0.0.1', self.port) async def poll(self): diff --git a/jupyterhub/user.py b/jupyterhub/user.py index de0b613a..bd7683f0 100644 --- a/jupyterhub/user.py +++ b/jupyterhub/user.py @@ -406,6 +406,7 @@ class User: if ip_port: # get ip, port info from return value of start() server.ip, server.port = ip_port + db.commit() else: # prior to 0.7, spawners had to store this info in user.server themselves. # Handle < 0.7 behavior with a warning, assuming info was stored in db by the Spawner. From e962c9993b684faa1f41bcc81456e3e299cd4913 Mon Sep 17 00:00:00 2001 From: Min RK Date: Mon, 9 Apr 2018 14:47:50 +0200 Subject: [PATCH 21/26] don't ask for exception is Future is not done --- jupyterhub/user.py | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-) diff --git a/jupyterhub/user.py b/jupyterhub/user.py index bd7683f0..5385df5e 100644 --- a/jupyterhub/user.py +++ b/jupyterhub/user.py @@ -555,7 +555,10 @@ class User: except Exception: self.log.exception("Error in Authenticator.post_spawn_stop for %s", self) spawner._stop_pending = False - if not (spawner._spawn_future and spawner._spawn_future.exception()): + if not ( + spawner._spawn_future and + (not spawner._spawn_future.done() or spawner._spawn_future.exception()) + ): # pop Spawner *unless* it's stopping due to an error # because some pages serve latest-spawn error messages self.spawners.pop(server_name) From b29110359291ab1f33232e1c6f6733a41dfb740a Mon Sep 17 00:00:00 2001 From: Min RK Date: Mon, 9 Apr 2018 14:51:20 +0200 Subject: [PATCH 22/26] fixup cleanup --- jupyterhub/tests/conftest.py | 1 + jupyterhub/tests/test_pages.py | 3 --- 2 files changed, 1 insertion(+), 3 deletions(-) diff --git a/jupyterhub/tests/conftest.py b/jupyterhub/tests/conftest.py index d4e8f04c..4df40d8e 100644 --- a/jupyterhub/tests/conftest.py +++ b/jupyterhub/tests/conftest.py @@ -72,6 +72,7 @@ def cleanup_after(request): for uid, user in app.users.items(): for name, spawner in list(user.spawners.items()): if spawner.active: + loop.run_until_complete(app.proxy.delete_user(user, name)) loop.run_until_complete(user.stop(name)) diff --git a/jupyterhub/tests/test_pages.py b/jupyterhub/tests/test_pages.py index 5e446cb1..583c3545 100644 --- a/jupyterhub/tests/test_pages.py +++ b/jupyterhub/tests/test_pages.py @@ -110,9 +110,6 @@ def test_spawn_redirect(app): cookies = yield app.login_user(name) u = app.users[orm.User.find(app.db, name)] - # ensure wash's server isn't running: - r = yield api_request(app, 'users', name, 'server', method='delete', cookies=cookies) - r.raise_for_status() status = yield u.spawner.poll() assert status is not None From 23ca2039f69e3545f0a8a3d4a993a27699d9a71a Mon Sep 17 00:00:00 2001 From: Min RK Date: Mon, 9 Apr 2018 15:09:50 +0200 Subject: [PATCH 23/26] run cleanup_after on ioloop instead of directly on asyncio --- jupyterhub/tests/conftest.py | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/jupyterhub/tests/conftest.py b/jupyterhub/tests/conftest.py index 4df40d8e..eeacbcf8 100644 --- a/jupyterhub/tests/conftest.py +++ b/jupyterhub/tests/conftest.py @@ -56,7 +56,7 @@ def io_loop(request): @fixture(autouse=True) -def cleanup_after(request): +def cleanup_after(request, io_loop): """function-scoped fixture to shutdown user servers allows cleanup of servers between tests @@ -68,12 +68,12 @@ def cleanup_after(request): if not MockHub.initialized(): return app = MockHub.instance() - loop = asyncio.new_event_loop() for uid, user in app.users.items(): for name, spawner in list(user.spawners.items()): if spawner.active: - loop.run_until_complete(app.proxy.delete_user(user, name)) - loop.run_until_complete(user.stop(name)) + print('closing', user) + io_loop.run_sync(lambda: app.proxy.delete_user(user, name)) + io_loop.run_sync(lambda: user.stop(name)) @fixture(scope='module') From 151acd5becdce192382c7391e9c40fc892a6d54c Mon Sep 17 00:00:00 2001 From: Min RK Date: Mon, 9 Apr 2018 15:10:58 +0200 Subject: [PATCH 24/26] catch errors in cleanup --- jupyterhub/tests/conftest.py | 8 +++++--- 1 file changed, 5 insertions(+), 3 deletions(-) diff --git a/jupyterhub/tests/conftest.py b/jupyterhub/tests/conftest.py index eeacbcf8..db7e9f13 100644 --- a/jupyterhub/tests/conftest.py +++ b/jupyterhub/tests/conftest.py @@ -3,7 +3,6 @@ # Copyright (c) Jupyter Development Team. # Distributed under the terms of the Modified BSD License. -import asyncio import os import logging from getpass import getuser @@ -12,6 +11,7 @@ from subprocess import TimeoutExpired from unittest import mock from pytest import fixture, raises from tornado import ioloop, gen +from tornado.httpclient import HTTPError from .. import orm from .. import crypto @@ -71,8 +71,10 @@ def cleanup_after(request, io_loop): for uid, user in app.users.items(): for name, spawner in list(user.spawners.items()): if spawner.active: - print('closing', user) - io_loop.run_sync(lambda: app.proxy.delete_user(user, name)) + try: + io_loop.run_sync(lambda: app.proxy.delete_user(user, name)) + except HTTPError: + pass io_loop.run_sync(lambda: user.stop(name)) From 7e3fa8c38d30f83c5e2d1bfbca27d147358879f6 Mon Sep 17 00:00:00 2001 From: Min RK Date: Mon, 9 Apr 2018 15:58:39 +0200 Subject: [PATCH 25/26] Don't double-check _stop_pending flag could cause spurious raises of Timeout errors --- jupyterhub/handlers/base.py | 7 ++----- 1 file changed, 2 insertions(+), 5 deletions(-) diff --git a/jupyterhub/handlers/base.py b/jupyterhub/handlers/base.py index c25ffcd5..8ec720e4 100644 --- a/jupyterhub/handlers/base.py +++ b/jupyterhub/handlers/base.py @@ -697,11 +697,8 @@ class BaseHandler(RequestHandler): try: await gen.with_timeout(timedelta(seconds=self.slow_stop_timeout), stop()) except gen.TimeoutError: - if spawner._stop_pending: - # hit timeout, but stop is still pending - self.log.warning("User %s:%s server is slow to stop", user.name, name) - else: - raise + # hit timeout, but stop is still pending + self.log.warning("User %s:%s server is slow to stop", user.name, name) #--------------------------------------------------------------- # template rendering From 7b5235138f4a39b3a3e911e7221e0bc87c8fa4bd Mon Sep 17 00:00:00 2001 From: Min RK Date: Mon, 9 Apr 2018 16:00:04 +0200 Subject: [PATCH 26/26] commit changes after stopping in cleanup --- jupyterhub/tests/conftest.py | 1 + 1 file changed, 1 insertion(+) diff --git a/jupyterhub/tests/conftest.py b/jupyterhub/tests/conftest.py index db7e9f13..e1b9af48 100644 --- a/jupyterhub/tests/conftest.py +++ b/jupyterhub/tests/conftest.py @@ -76,6 +76,7 @@ def cleanup_after(request, io_loop): except HTTPError: pass io_loop.run_sync(lambda: user.stop(name)) + app.db.commit() @fixture(scope='module')