Merge pull request #438 from kevin-bates/port-terminal-culling

Port terminal culling from Notebook

Author: Zsailer

docs/environment.yml

@@ -13,4 +13,5 @@ dependencies:
   - sphinxcontrib_github_alt
   - sphinxcontrib-openapi
   - sphinxemoji
+  - terminado
   - git+https://github.com/pandas-dev/pydata-sphinx-theme.git@master

jupyter_server/serverapp.py

@@ -116,6 +116,13 @@
 from jupyter_server.extension.config import ExtensionConfigManager
 from jupyter_server.traittypes import TypeFromClasses
 
+# Tolerate missing terminado package.
+try:
+    from .terminal import TerminalManager
+    terminado_available = True
+except ImportError:
+    terminado_available = False
+
 #-----------------------------------------------------------------------------
 # Module globals
 #-----------------------------------------------------------------------------
@@ -284,7 +291,7 @@ def init_settings(self, jupyter_app, kernel_manager, contents_manager,
             allow_password_change=jupyter_app.allow_password_change,
             server_root_dir=root_dir,
             jinja2_env=env,
-            terminals_available=False,  # Set later if terminals are available
+            terminals_available=terminado_available and jupyter_app.terminals_enabled,
             serverapp=jupyter_app
         )
 
@@ -589,6 +596,8 @@ class ServerApp(JupyterApp):
             ContentsManager, FileContentsManager, AsyncContentsManager, AsyncFileContentsManager, NotebookNotary,
             GatewayKernelManager, GatewayKernelSpecManager, GatewaySessionManager, GatewayClient
         ]
+    if terminado_available:  # Only necessary when terminado is available
+        classes.append(TerminalManager)
 
     subcommands = dict(
         list=(JupyterServerListApp, JupyterServerListApp.description.splitlines()[0]),
@@ -1329,6 +1338,15 @@ def _update_server_extensions(self, change):
          is not available.
          """))
 
+    # Since use of terminals is also a function of whether the terminado package is
+    # available, this variable holds the "final indication" of whether terminal functionality
+    # should be considered (particularly during shutdown/cleanup).  It is enabled only
+    # once both the terminals "service" can be initialized and terminals_enabled is True.
+    # Note: this variable is slightly different from 'terminals_available' in the web settings
+    # in that this variable *could* remain false if terminado is available, yet the terminal
+    # service's initialization still fails.  As a result, this variable holds the truth.
+    terminals_available = False
+
     authenticate_prometheus = Bool(
         True,
         help=""""
@@ -1547,7 +1565,7 @@ def init_terminals(self):
         try:
             from .terminal import initialize
             initialize(self.web_app, self.root_dir, self.connection_url, self.terminado_settings)
-            self.web_app.settings['terminals_available'] = True
+            self.terminals_available = True
         except ImportError as e:
             self.log.warning(_i18n("Terminals not available (error was %s)"), e)
 
@@ -1693,11 +1711,8 @@ def shutdown_no_activity(self):
         if len(km) != 0:
             return   # Kernels still running
 
-        try:
+        if self.terminals_available:
             term_mgr = self.web_app.settings['terminal_manager']
-        except KeyError:
-            pass  # Terminals not enabled
-        else:
             if term_mgr.terminals:
                 return   # Terminals still running
 
@@ -1846,6 +1861,21 @@ def cleanup_kernels(self):
         self.log.info(kernel_msg % n_kernels)
         run_sync(self.kernel_manager.shutdown_all())
 
+    def cleanup_terminals(self):
+        """Shutdown all terminals.
+
+        The terminals will shutdown themselves when this process no longer exists,
+        but explicit shutdown allows the TerminalManager to cleanup.
+        """
+        if not self.terminals_available:
+            return
+
+        terminal_manager = self.web_app.settings['terminal_manager']
+        n_terminals = len(terminal_manager.list())
+        terminal_msg = trans.ngettext('Shutting down %d terminal', 'Shutting down %d terminals', n_terminals)
+        self.log.info(terminal_msg % n_terminals)
+        run_sync(terminal_manager.terminate_all())
+
     def running_server_info(self, kernel_count=True):
         "Return the current working directory and the server url information"
         info = self.contents_manager.info_string() + "\n"
@@ -2076,6 +2106,7 @@ def _cleanup(self):
         self.remove_server_info_file()
         self.remove_browser_open_files()
         self.cleanup_kernels()
+        self.cleanup_terminals()
 
     def start_ioloop(self):
         """Start the IO Loop."""

jupyter_server/services/api/api.yaml

@@ -568,7 +568,7 @@ paths:
           schema:
             type: array
             items:
-              $ref: '#/definitions/Terminal_ID'
+              $ref: '#/definitions/Terminal'
         403:
           description: Forbidden to access
         404:
@@ -582,7 +582,7 @@ paths:
         200:
           description: Succesfully created a new terminal
           schema:
-            $ref: '#/definitions/Terminal_ID'
+            $ref: '#/definitions/Terminal'
         403:
           description: Forbidden to access
         404:
@@ -599,7 +599,7 @@ paths:
         200:
           description: Terminal session with given id
           schema:
-            $ref: '#/definitions/Terminal_ID'
+            $ref: '#/definitions/Terminal'
         403:
           description: Forbidden to access
         404:
@@ -845,12 +845,18 @@ definitions:
         type: string
         description: Last modified timestamp
         format: dateTime
-  Terminal_ID:
-    description: A Terminal_ID object
+  Terminal:
+    description: A Terminal object
     type: object
     required:
       - name
     properties:
       name:
         type: string
-        description: name of terminal ID
+        description: name of terminal
+      last_activity:
+        type: string
+        description: |
+          ISO 8601 timestamp for the last-seen activity on this terminal.  Use
+          this to identify which terminals have been inactive since a given time.
+          Timestamps will be UTC, indicated 'Z' suffix.

jupyter_server/terminal/__init__.py

@@ -8,11 +8,10 @@
     raise ImportError("terminado >= 0.8.3 required, found %s" % terminado.__version__)
 
 from ipython_genutils.py3compat import which
-from terminado import NamedTermManager
-from tornado.log import app_log
 from jupyter_server.utils import url_path_join as ujoin
 from . import api_handlers
 from .handlers import TermSocket
+from .terminalmanager import TerminalManager
 
 
 def initialize(webapp, root_dir, connection_url, settings):
@@ -33,13 +32,14 @@ def initialize(webapp, root_dir, connection_url, settings):
     # the user has specifically set a preferred shell command.
     if os.name != 'nt' and shell_override is None and not sys.stdout.isatty():
         shell.append('-l')
-    terminal_manager = webapp.settings['terminal_manager'] = NamedTermManager(
+    terminal_manager = webapp.settings['terminal_manager'] = TerminalManager(
         shell_command=shell,
         extra_env={'JUPYTER_SERVER_ROOT': root_dir,
                    'JUPYTER_SERVER_URL': connection_url,
                    },
+        parent=webapp.settings['serverapp'],
     )
-    terminal_manager.log = app_log
+    terminal_manager.log = webapp.settings['serverapp'].log
     base_url = webapp.settings['base_url']
     handlers = [
         (ujoin(base_url, r"/terminals/websocket/(\w+)"), TermSocket,

jupyter_server/terminal/api_handlers.py

@@ -1,57 +1,34 @@
 import json
 from tornado import web
 from ..base.handlers import APIHandler
-from ..prometheus.metrics import TERMINAL_CURRENTLY_RUNNING_TOTAL
-
 
 
 class TerminalRootHandler(APIHandler):
 
     @web.authenticated
     def get(self):
-        tm = self.terminal_manager
-        terms = [{'name': name} for name in tm.terminals]
-        self.finish(json.dumps(terms))
-
-        # Update the metric below to the length of the list 'terms'
-        TERMINAL_CURRENTLY_RUNNING_TOTAL.set(
-            len(terms)
-        )
+        models = self.terminal_manager.list()
+        self.finish(json.dumps(models))
 
     @web.authenticated
     def post(self):
         """POST /terminals creates a new terminal and redirects to it"""
         data = self.get_json_body() or {}
 
-        name, _ = self.terminal_manager.new_named_terminal(**data)
-        self.finish(json.dumps({'name': name}))
-
-        # Increase the metric by one because a new terminal was created
-        TERMINAL_CURRENTLY_RUNNING_TOTAL.inc()
+        model = self.terminal_manager.create(**data)
+        self.finish(json.dumps(model))
 
 
 class TerminalHandler(APIHandler):
     SUPPORTED_METHODS = ('GET', 'DELETE')
 
     @web.authenticated
     def get(self, name):
-        tm = self.terminal_manager
-        if name in tm.terminals:
-            self.finish(json.dumps({'name': name}))
-        else:
-            raise web.HTTPError(404, "Terminal not found: %r" % name)
+        model = self.terminal_manager.get(name)
+        self.finish(json.dumps(model))
 
     @web.authenticated
     async def delete(self, name):
-        tm = self.terminal_manager
-        if name in tm.terminals:
-            await tm.terminate(name, force=True)
-            self.set_status(204)
-            self.finish()
-
-            # Decrease the metric below by one
-            # because a terminal has been shutdown
-            TERMINAL_CURRENTLY_RUNNING_TOTAL.dec()
-
-        else:
-            raise web.HTTPError(404, "Terminal not found: %r" % name)
+        await self.terminal_manager.terminate(name, force=True)
+        self.set_status(204)
+        self.finish()

jupyter_server/terminal/handlers.py

@@ -26,8 +26,14 @@ def get(self, *args, **kwargs):
 
     def on_message(self, message):
         super(TermSocket, self).on_message(message)
-        self.application.settings['terminal_last_activity'] = utcnow()
+        self._update_activity()
 
     def write_message(self, message, binary=False):
         super(TermSocket, self).write_message(message, binary=binary)
-        self.application.settings['terminal_last_activity'] = utcnow()
+        self._update_activity()
+
+    def _update_activity(self):
+        self.application.settings['terminal_last_activity'] = utcnow()
+        # terminal may not be around on deletion/cull
+        if self.term_name in self.terminal_manager.terminals:
+            self.terminal_manager.terminals[self.term_name].last_activity = utcnow()