0.9.0b3

Rollback database sessions on SQLAlchemy errors

ci/docker-db.sh

@@ -8,7 +8,7 @@ export MYSQL_HOST=127.0.0.1
 export MYSQL_TCP_PORT=${MYSQL_TCP_PORT:-13306}
 export PGHOST=127.0.0.1
 NAME="hub-test-$DB"
-DOCKER_RUN="docker run --rm -d --name $NAME"
+DOCKER_RUN="docker run -d --name $NAME"
 
 docker rm -f "$NAME" 2>/dev/null || true
 
@@ -47,4 +47,4 @@ Set these environment variables:
     export MYSQL_HOST=127.0.0.1
     export MYSQL_TCP_PORT=$MYSQL_TCP_PORT
     export PGHOST=127.0.0.1
-"
+"

jupyterhub/_version.py

@@ -7,8 +7,8 @@ version_info = (
     0,
     9,
     0,
-    'b2',  # release
-    # 'dev',  # dev
+    "b3",  # release (b1, rc1)
+    # "dev",  # dev
 )
 
 # pep 440 version: no dot before beta/rc, but before .dev

jupyterhub/apihandlers/base.py

@@ -6,6 +6,7 @@ import json
 
 from http.client import responses
 
+from sqlalchemy.exc import SQLAlchemyError
 from tornado import web
 
 from .. import orm
@@ -87,6 +88,10 @@ class APIHandler(BaseHandler):
             if reason:
                 status_message = reason
 
+        if exception and isinstance(exception, SQLAlchemyError):
+            self.log.warning("Rolling back session due to database error %s", exception)
+            self.db.rollback()
+
         self.set_header('Content-Type', 'application/json')
         # allow setting headers from exceptions
         # since exception handler clears headers

jupyterhub/app.py

@@ -27,7 +27,7 @@ if sys.version_info[:2] < (3, 3):
 
 from dateutil.parser import parse as parse_date
 from jinja2 import Environment, FileSystemLoader, PrefixLoader, ChoiceLoader
-from sqlalchemy.exc import OperationalError
+from sqlalchemy.exc import OperationalError, SQLAlchemyError
 
 from tornado.httpclient import AsyncHTTPClient
 import tornado.httpserver
@@ -358,7 +358,7 @@ class JupyterHub(Application):
             self.bind_url = bind_url
 
     bind_url = Unicode(
-        "http://127.0.0.1:8000",
+        "http://:8000",
         help="""The public facing URL of the whole JupyterHub application.
 
         This is the address on which the proxy will bind.
@@ -1493,10 +1493,16 @@ class JupyterHub(Application):
                 oauth_client_ids.add(spawner.oauth_client_id)
 
         client_store = self.oauth_provider.client_authenticator.client_store
-        for oauth_client in self.db.query(orm.OAuthClient):
+        for i, oauth_client in enumerate(self.db.query(orm.OAuthClient)):
             if oauth_client.identifier not in oauth_client_ids:
                 self.log.warning("Deleting OAuth client %s", oauth_client.identifier)
                 self.db.delete(oauth_client)
+                # Some deployments that create temporary users may have left *lots*
+                # of entries here.
+                # Don't try to delete them all in one transaction,
+                # commit at most 100 deletions at a time.
+                if i % 100 == 0:
+                    self.db.commit()
         self.db.commit()
 
     def init_proxy(self):
@@ -1621,6 +1627,33 @@ class JupyterHub(Application):
             cfg.JupyterHub.merge(cfg.JupyterHubApp)
             self.update_config(cfg)
         self.write_pid_file()
+
+        def _log_cls(name, cls):
+            """Log a configured class
+
+            Logs the class and version (if found) of Authenticator
+            and Spawner
+            """
+            # try to guess the version from the top-level module
+            # this will work often enough to be useful.
+            # no need to be perfect.
+            if cls.__module__:
+                mod = sys.modules.get(cls.__module__.split('.')[0])
+                version = getattr(mod, '__version__', '')
+                if version:
+                    version = '-{}'.format(version)
+            else:
+                version = ''
+            self.log.info(
+                "Using %s: %s.%s%s",
+                name,
+                cls.__module__ or '',
+                cls.__name__,
+                version,
+            )
+        _log_cls("Authenticator", self.authenticator_class)
+        _log_cls("Spawner", self.spawner_class)
+
         self.init_pycurl()
         self.init_secrets()
         self.init_db()
@@ -1759,7 +1792,13 @@ class JupyterHub(Application):
         self.statsd.gauge('users.running', users_count)
         self.statsd.gauge('users.active', active_users_count)
 
-        self.db.commit()
+        try:
+            self.db.commit()
+        except SQLAlchemyError:
+            self.log.exception("Rolling back session due to database error")
+            self.db.rollback()
+            return
+
         await self.proxy.check_routes(self.users, self._service_map, routes)
 
     async def start(self):

jupyterhub/auth.py

@@ -49,7 +49,7 @@ class Authenticator(LoggingConfigurable):
 
         Encrypting auth_state requires the cryptography package.
 
-        Additionally, the JUPYTERHUB_CRYPTO_KEY envirionment variable must
+        Additionally, the JUPYTERHUB_CRYPT_KEY environment variable must
         contain one (or more, separated by ;) 32B encryption keys.
         These can be either base64 or hex-encoded.
 

jupyterhub/handlers/base.py

@@ -15,6 +15,7 @@ import uuid
 
 from jinja2 import TemplateNotFound
 
+from sqlalchemy.exc import SQLAlchemyError
 from tornado.log import app_log
 from tornado.httputil import url_concat, HTTPHeaders
 from tornado.ioloop import IOLoop
@@ -264,10 +265,17 @@ class BaseHandler(RequestHandler):
 
     def get_current_user(self):
         """get current username"""
-        user = self.get_current_user_token()
-        if user is not None:
-            return user
-        return self.get_current_user_cookie()
+        if not hasattr(self, '_jupyterhub_user'):
+            try:
+                user = self.get_current_user_token()
+                if user is None:
+                    user = self.get_current_user_cookie()
+                self._jupyterhub_user = user
+            except Exception:
+                # don't let errors here raise more than once
+                self._jupyterhub_user = None
+                raise
+        return self._jupyterhub_user
 
     def find_user(self, name):
         """Get a user by name
@@ -417,10 +425,20 @@ class BaseHandler(RequestHandler):
         - else: /hub/home
         """
         next_url = self.get_argument('next', default='')
-        if (next_url + '/').startswith('%s://%s/' % (self.request.protocol, self.request.host)):
+        if (next_url + '/').startswith(
+            (
+                '%s://%s/' % (self.request.protocol, self.request.host),
+                '//%s/' % self.request.host,
+            )
+        ):
             # treat absolute URLs for our host as absolute paths:
-            next_url = urlparse(next_url).path
-        if next_url and not next_url.startswith('/'):
+            parsed = urlparse(next_url)
+            next_url = parsed.path
+            if parsed.query:
+                next_url = next_url + '?' + parsed.query
+            if parsed.hash:
+                next_url = next_url + '#' + parsed.hash
+        if next_url and (urlparse(next_url).netloc or not next_url.startswith('/')):
             self.log.warning("Disallowing redirect outside JupyterHub: %r", next_url)
             next_url = ''
         if next_url and next_url.startswith(url_path_join(self.base_url, 'user/')):
@@ -794,6 +812,10 @@ class BaseHandler(RequestHandler):
             if reason:
                 message = reasons.get(reason, reason)
 
+        if exception and isinstance(exception, SQLAlchemyError):
+            self.log.warning("Rolling back session due to database error %s", exception)
+            self.db.rollback()
+
         # build template namespace
         ns = dict(
             status_code=status_code,

jupyterhub/orm.py

@@ -14,7 +14,7 @@ from tornado.log import app_log
 
 from sqlalchemy.types import TypeDecorator, TEXT, LargeBinary
 from sqlalchemy import (
-    create_engine, event, inspect, or_,
+    create_engine, event, exc, inspect, or_, select,
     Column, Integer, ForeignKey, Unicode, Boolean,
     DateTime, Enum, Table,
 )
@@ -575,7 +575,7 @@ def _expire_relationship(target, relationship_prop):
 def _notify_deleted_relationships(session, obj):
     """Expire relationships when an object becomes deleted
 
-    Needed for
+    Needed to keep relationships up to date.
     """
     mapper = inspect(obj).mapper
     for prop in mapper.relationships:
@@ -583,6 +583,52 @@ def _notify_deleted_relationships(session, obj):
             _expire_relationship(obj, prop)
 
 
+def register_ping_connection(engine):
+    """Check connections before using them.
+
+    Avoids database errors when using stale connections.
+
+    From SQLAlchemy docs on pessimistic disconnect handling:
+
+    https://docs.sqlalchemy.org/en/rel_1_1/core/pooling.html#disconnect-handling-pessimistic
+    """
+    @event.listens_for(engine, "engine_connect")
+    def ping_connection(connection, branch):
+        if branch:
+            # "branch" refers to a sub-connection of a connection,
+            # we don't want to bother pinging on these.
+            return
+
+        # turn off "close with result".  This flag is only used with
+        # "connectionless" execution, otherwise will be False in any case
+        save_should_close_with_result = connection.should_close_with_result
+        connection.should_close_with_result = False
+
+        try:
+            # run a SELECT 1.   use a core select() so that
+            # the SELECT of a scalar value without a table is
+            # appropriately formatted for the backend
+            connection.scalar(select([1]))
+        except exc.DBAPIError as err:
+            # catch SQLAlchemy's DBAPIError, which is a wrapper
+            # for the DBAPI's exception.  It includes a .connection_invalidated
+            # attribute which specifies if this connection is a "disconnect"
+            # condition, which is based on inspection of the original exception
+            # by the dialect in use.
+            if err.connection_invalidated:
+                app_log.error("Database connection error, attempting to reconnect: %s", err)
+                # run the same SELECT again - the connection will re-validate
+                # itself and establish a new connection.  The disconnect detection
+                # here also causes the whole connection pool to be invalidated
+                # so that all stale connections are discarded.
+                connection.scalar(select([1]))
+            else:
+                raise
+        finally:
+            # restore "close with result"
+            connection.should_close_with_result = save_should_close_with_result
+
+
 def check_db_revision(engine):
     """Check the JupyterHub database revision
 
@@ -661,10 +707,12 @@ def mysql_large_prefix_check(engine):
     else:
         return False
 
+
 def add_row_format(base):
     for t in  base.metadata.tables.values():
         t.dialect_kwargs['mysql_ROW_FORMAT'] = 'DYNAMIC'
 
+
 def new_session_factory(url="sqlite:///:memory:",
                         reset=False,
                         expire_on_commit=False,
@@ -684,6 +732,9 @@ def new_session_factory(url="sqlite:///:memory:",
         kwargs.setdefault('poolclass', StaticPool)
 
     engine = create_engine(url, **kwargs)
+    # enable pessimistic disconnect handling
+    register_ping_connection(engine)
+
     if reset:
         Base.metadata.drop_all(engine)
 

jupyterhub/tests/test_app.py

@@ -242,6 +242,16 @@ def test_resume_spawners(tmpdir, request):
             {'bind_url': 'http://0.0.0.0:12345/sub'},
             {'base_url': '/sub/'},
         ),
+        (
+            # no config, test defaults
+            {},
+            {
+                'base_url': '/',
+                'bind_url': 'http://:8000',
+                'ip': '',
+                'port': 8000,
+                },
+        ),
     ]
 )
 def test_url_config(hub_config, expected):

jupyterhub/tests/test_pages.py

@@ -3,6 +3,7 @@
 from urllib.parse import urlencode, urlparse
 
 from tornado import gen
+from tornado.httputil import url_concat
 
 from ..handlers import BaseHandler
 from ..utils import url_path_join as ujoin
@@ -366,29 +367,50 @@ def test_login_strip(app):
     assert called_with == [form_data]
 
 
+@pytest.mark.parametrize(
+    'running, next_url, location',
+    [
+        # default URL if next not specified, for both running and not
+        (True, '', ''),
+        (False, '', ''),
+        # next_url is respected
+        (False, '/hub/admin', '/hub/admin'),
+        (False, '/user/other', '/hub/user/other'),
+        (False, '/absolute', '/absolute'),
+        (False, '/has?query#andhash', '/has?query#andhash'),
+
+        # next_url outside is not allowed
+        (False, 'https://other.domain', ''),
+        (False, 'ftp://other.domain', ''),
+        (False, '//other.domain', ''),
+    ]
+)
 @pytest.mark.gen_test
-def test_login_redirect(app):
+def test_login_redirect(app, running, next_url, location):
     cookies = yield app.login_user('river')
     user = app.users['river']
-    # no next_url, server running
-    yield user.spawn()
-    r = yield get_page('login', app, cookies=cookies, allow_redirects=False)
-    r.raise_for_status()
-    assert r.status_code == 302
-    assert '/user/river' in r.headers['Location']
+    if location:
+        location = ujoin(app.base_url, location)
+    else:
+        # use default url
+        location = user.url
 
-    # no next_url, server not running
-    yield user.stop()
-    r = yield get_page('login', app, cookies=cookies, allow_redirects=False)
-    r.raise_for_status()
-    assert r.status_code == 302
-    assert '/user/river' in r.headers['Location']
+    url = 'login'
+    if next_url:
+        if '//' not in next_url:
+            next_url = ujoin(app.base_url, next_url, '')
+        url = url_concat(url, dict(next=next_url))
 
-    # next URL given, use it
-    r = yield get_page('login?next=/hub/admin', app, cookies=cookies, allow_redirects=False)
+    if running and not user.active:
+        # ensure running
+        yield user.spawn()
+    elif user.active and not running:
+        # ensure not running
+        yield user.stop()
+    r = yield get_page(url, app, cookies=cookies, allow_redirects=False)
     r.raise_for_status()
     assert r.status_code == 302
-    assert r.headers['Location'].endswith('/hub/admin')
+    assert location == r.headers['Location']
 
 
 @pytest.mark.gen_test