address review in spawn-throttle-retry

- update config to single tuple instead of two integers
- call it spawn_throttle_retry_range
- fix setting Retry-After header without disabling error pages

jupyterhub/apihandlers/base.py

@@ -72,6 +72,7 @@ class APIHandler(BaseHandler):
         """Write JSON errors instead of HTML"""
         exc_info = kwargs.get('exc_info')
         message = ''
+        exception = None
         status_message = responses.get(status_code, 'Unknown Error')
         if exc_info:
             exception = exc_info[1]
@@ -85,6 +86,15 @@ class APIHandler(BaseHandler):
             reason = getattr(exception, 'reason', '')
             if reason:
                 status_message = reason
+
+        self.set_header('Content-Type', 'application/json')
+        # allow setting headers from exceptions
+        # since exception handler clears headers
+        headers = getattr(exception, 'headers', None)
+        if headers:
+            for key, value in headers.items():
+                self.set_header(key, value)
+
         self.write(json.dumps({
             'status': status_code,
             'message': message or status_message,

jupyterhub/app.py

@@ -36,7 +36,7 @@ from tornado.platform.asyncio import AsyncIOMainLoop
 
 from traitlets import (
     Unicode, Integer, Dict, TraitError, List, Bool, Any,
-    Type, Set, Instance, Bytes, Float,
+    Tuple, Type, Set, Instance, Bytes, Float,
     observe, default,
 )
 from traitlets.config import Application, catch_config_error
@@ -616,28 +616,20 @@ class JupyterHub(Application):
         """
     ).tag(config=True)
 
-    throttle_retry_suggest_min = Integer(
-        30,
+    spawn_throttle_retry_range = Tuple(
+        (30, 60),
         help="""
-        Minimum seconds after which we suggest the user retry spawning.
+        (min seconds, max seconds) range to suggest a user wait before retrying.
 
-        When `concurrent_spawn_limit` is exceeded, we recommend users retry
-        after a random period of time, bounded by throttle_retry_suggest_min
-        and throttle_retry_suggest_max.
+        When `concurrent_spawn_limit` is exceeded, spawning is throttled.
+        We suggest users wait random period of time within this range
+        before retrying.
 
-        throttle_retry_suggest_min should ideally be set to the median
-        spawn time of servers in your installation.
-        """
-    )
+        A Retry-After header is set with a random value within this range.
+        Error pages will display a rounded version of this value.
 
-    throttle_retry_suggest_max = Integer(
-        60,
-        help="""
-        Minimum seconds after which we suggest the user retry spawning.
-
-        When `concurrent_spawn_limit` is exceeded, we recommend users retry
-        after a random period of time, bounded by throttle_retry_suggest_min
-        and throttle_retry_suggest_max.
+        The lower bound should ideally be approximately
+        the median spawn time for your deployment.
         """
     )
 
@@ -1448,8 +1440,7 @@ class JupyterHub(Application):
             allow_named_servers=self.allow_named_servers,
             oauth_provider=self.oauth_provider,
             concurrent_spawn_limit=self.concurrent_spawn_limit,
-            throttle_retry_suggest_min=self.throttle_retry_suggest_min,
-            throttle_retry_suggest_max=self.throttle_retry_suggest_max,
+            spawn_throttle_retry_range=self.spawn_throttle_retry_range,
             active_server_limit=self.active_server_limit,
         )
         # allow configured settings to have priority

jupyterhub/handlers/base.py

@@ -4,13 +4,14 @@
 # Distributed under the terms of the Modified BSD License.
 
 import copy
-import re
-import time
 from datetime import datetime, timedelta
 from http.client import responses
+import math
+import random
+import re
+import time
 from urllib.parse import urlparse, urlunparse, parse_qs, urlencode
 import uuid
-import random
 
 from jinja2 import TemplateNotFound
 
@@ -533,18 +534,30 @@ class BaseHandler(RequestHandler):
             # Suggest number of seconds client should wait before retrying
             # This helps prevent thundering herd problems, where users simply
             # immediately retry when we are overloaded.
-            retry_time = int(random.uniform(
-                self.settings['throttle_retry_suggest_min'],
-                self.settings['throttle_retry_suggest_max']
-            ))
-            self.set_header('Retry-After', str(retry_time))
-            self.log.info(
-                '%s pending spawns, throttling. Retry in %s seconds',
-                spawn_pending_count, retry_time
+            retry_range = self.settings['spawn_throttle_retry_range']
+            retry_time = int(random.uniform(*retry_range))
+
+            # round suggestion to nicer human value (nearest 10 seconds or minute)
+            if retry_time <= 90:
+                # round human seconds up to nearest 10
+                human_retry_time = "%i0 seconds" % math.ceil(retry_time / 10.)
+            else:
+                # round number of minutes
+                human_retry_time = "%i minutes" % math.round(retry_time / 60.)
+
+            self.log.warning(
+                '%s pending spawns, throttling. Suggested retry in %s seconds.',
+                spawn_pending_count, retry_time,
             )
-            self.set_status(429, "Too many users trying to log in right now. Try again in a {}s".format(retry_time))
-            # We use set_status and then raise web.Finish, since raising web.HTTPError resets any headers we wanna send.
-            raise web.Finish()
+            err = web.HTTPError(
+                429,
+                "Too many users trying to log in right now. Try again in {}.".format(human_retry_time)
+            )
+            # can't call set_header directly here because it gets ignored
+            # when errors are raised
+            # we handle err.headers ourselves in Handler.write_error
+            err.headers = {'Retry-After': retry_time}
+            raise err
 
         if active_server_limit and active_count >= active_server_limit:
             self.log.info(
@@ -779,6 +792,13 @@ class BaseHandler(RequestHandler):
         )
 
         self.set_header('Content-Type', 'text/html')
+        # allow setting headers from exceptions
+        # since exception handler clears headers
+        headers = getattr(exception, 'headers', None)
+        if headers:
+            for key, value in headers.items():
+                self.set_header(key, value)
+
         # render the template
         try:
             html = self.render_template('%s.html' % status_code, **ns)