Remove terminals in favor of jupyter_server_terminals extension (#651)

Co-authored-by: Steven Silvester <steven.silvester@ieee.org>

.pre-commit-config.yaml

@@ -44,6 +44,7 @@ repos:
     hooks:
       - id: doc8
         args: [--max-line-length=200]
+        exclude: docs/source/other/full-config.rst
         stages: [manual]
 
   - repo: https://github.com/pycqa/flake8

jupyter_server/extension/application.py

@@ -429,6 +429,10 @@ def start(self):
         # Start the server.
         self.serverapp.start()
 
+    def current_activity(self):
+        """Return a list of activity happening in this extension."""
+        return
+
     async def stop_extension(self):
         """Cleanup any resources managed by this extension."""
 

jupyter_server/extension/handler.py

@@ -28,8 +28,12 @@ class ExtensionHandlerMixin:
     other extensions.
     """
 
-    def initialize(self, name):
+    def initialize(self, name, *args, **kwargs):
         self.name = name
+        try:
+            super().initialize(*args, **kwargs)
+        except TypeError:
+            pass
 
     @property
     def extensionapp(self):

jupyter_server/extension/manager.py

@@ -394,3 +394,9 @@ async def stop_all_extensions(self):
                 for name, apps in sorted(dict(self.extension_apps).items())
             ]
         )
+
+    def any_activity(self):
+        """Check for any activity currently happening across all extension applications."""
+        for _, app in sorted(dict(self.extension_apps).items()):
+            if app.current_activity():
+                return True

jupyter_server/pytest_plugin.py

@@ -127,7 +127,11 @@ def jp_environ(
 @pytest.fixture
 def jp_server_config():
     """Allows tests to setup their specific configuration values."""
-    return {}
+    return Config(
+        {
+            "jpserver_extensions": {"jupyter_server_terminals": True},
+        }
+    )
 
 
 @pytest.fixture
@@ -225,6 +229,13 @@ def my_test(jp_configurable_serverapp):
     """
     ServerApp.clear_instance()
 
+    # Inject jupyter_server_terminals into config unless it was
+    # explicitly put in config.
+    serverapp_config = jp_server_config.setdefault("ServerApp", {})
+    exts = serverapp_config.setdefault("jpserver_extensions", {})
+    if "jupyter_server_terminals" not in exts:
+        exts["jupyter_server_terminals"] = True
+
     def _configurable_serverapp(
         config=jp_server_config,
         base_url=jp_base_url,
@@ -473,7 +484,12 @@ def jp_cleanup_subprocesses(jp_serverapp):
     """Clean up subprocesses started by a Jupyter Server, i.e. kernels and terminal."""
 
     async def _():
-        terminal_cleanup = jp_serverapp.web_app.settings["terminal_manager"].terminate_all
+        term_manager = jp_serverapp.web_app.settings.get("terminal_manager")
+        if term_manager:
+            terminal_cleanup = term_manager.terminate_all
+        else:
+            terminal_cleanup = lambda: None  # noqa
+
         kernel_cleanup = jp_serverapp.kernel_manager.shutdown_all
 
         async def kernel_cleanup_steps():
@@ -496,7 +512,7 @@ async def kernel_cleanup_steps():
                 print(e)
         else:
             try:
-                await terminal_cleanup()
+                terminal_cleanup()
             except Exception as e:
                 print(e)
         if asyncio.iscoroutinefunction(kernel_cleanup):

jupyter_server/serverapp.py

@@ -134,14 +134,6 @@
     urlencode_unix_socket_path,
 )
 
-# Tolerate missing terminado package.
-try:
-    from jupyter_server.terminal import TerminalManager
-
-    terminado_available = True
-except ImportError:
-    terminado_available = False
-
 # -----------------------------------------------------------------------------
 # Module globals
 # -----------------------------------------------------------------------------
@@ -292,7 +284,7 @@ def init_settings(
         env.install_gettext_translations(nbui, newstyle=False)
 
         if sys_info["commit_source"] == "repository":
-            # don't cache (rely on 304) when working from default branch
+            # don't cache (rely on 304) when working from master
             version_hash = ""
         else:
             # reset the cache on server restart
@@ -361,7 +353,6 @@ def init_settings(
             allow_password_change=jupyter_app.allow_password_change,
             server_root_dir=root_dir,
             jinja2_env=env,
-            terminals_available=terminado_available and jupyter_app.terminals_enabled,
             serverapp=jupyter_app,
         )
 
@@ -454,14 +445,12 @@ def last_activity(self):
             self.settings["started"],
             self.settings["kernel_manager"].last_kernel_activity,
         ]
-        try:
-            sources.append(self.settings["api_last_activity"])
-        except KeyError:
-            pass
-        try:
-            sources.append(self.settings["terminal_last_activity"])
-        except KeyError:
-            pass
+        # Any setting that ends with a key that ends with `_last_activity` is
+        # counted here. This provides a hook for extensions to add a last activity
+        # setting to the server.
+        sources.extend(
+            [key for key, val in self.settings.items() if key.endswith("_last_activity")]
+        )
         sources.extend(self.settings["last_activity_times"].values())
         return max(sources)
 
@@ -744,8 +733,6 @@ class ServerApp(JupyterApp):
         GatewayClient,
         Authorizer,
     ]
-    if terminado_available:  # Only necessary when terminado is available
-        classes.append(TerminalManager)
 
     subcommands = dict(
         list=(JupyterServerListApp, JupyterServerListApp.description.splitlines()[0]),
@@ -1713,8 +1700,8 @@ def _update_server_extensions(self, change):
         0,
         config=True,
         help=(
-            "Shut down the server after N seconds with no kernels or "
-            "terminals running and no activity. "
+            "Shut down the server after N seconds with no kernels"
+            "running and no activity. "
             "This can be used together with culling idle kernels "
             "(MappingKernelManager.cull_idle_timeout) to "
             "shutdown the Jupyter server when it's not in use. This is not "
@@ -1724,7 +1711,6 @@ def _update_server_extensions(self, change):
     )
 
     terminals_enabled = Bool(
-        True,
         config=True,
         help=_i18n(
             """Set to False to disable terminals.
@@ -1738,14 +1724,10 @@ def _update_server_extensions(self, change):
         ),
     )
 
-    # 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
+    @default("terminals_enabled")
+    def _default_terminals_enabled(self):
+
+        return True
 
     authenticate_prometheus = Bool(
         True,
@@ -2032,23 +2014,6 @@ def connection_url(self):
         urlparts = self._get_urlparts(path=self.base_url)
         return urlparts.geturl()
 
-    def init_terminals(self):
-        if not self.terminals_enabled:
-            return
-
-        try:
-            from jupyter_server.terminal import initialize
-
-            initialize(
-                self.web_app,
-                self.root_dir,
-                self.connection_url,
-                self.terminado_settings,
-            )
-            self.terminals_available = True
-        except ImportError as e:
-            self.log.warning(_i18n("Terminals not available (error was %s)"), e)
-
     def init_signal(self):
         if not sys.platform.startswith("win") and sys.stdin and sys.stdin.isatty():
             signal.signal(signal.SIGINT, self._handle_sigint)
@@ -2194,24 +2159,22 @@ def shutdown_no_activity(self):
         if len(km) != 0:
             return  # Kernels still running
 
-        if self.terminals_available:
-            term_mgr = self.web_app.settings["terminal_manager"]
-            if term_mgr.terminals:
-                return  # Terminals still running
+        if self.extension_manager.any_activity:
+            return
 
         seconds_since_active = (utcnow() - self.web_app.last_activity()).total_seconds()
         self.log.debug("No activity for %d seconds.", seconds_since_active)
         if seconds_since_active > self.shutdown_no_activity_timeout:
             self.log.info(
-                "No kernels or terminals for %d seconds; shutting down.",
+                "No kernels for %d seconds; shutting down.",
                 seconds_since_active,
             )
             self.stop()
 
     def init_shutdown_no_activity(self):
         if self.shutdown_no_activity_timeout > 0:
             self.log.info(
-                "Will shut down after %d seconds with no kernels or terminals.",
+                "Will shut down after %d seconds with no kernels.",
                 self.shutdown_no_activity_timeout,
             )
             pc = ioloop.PeriodicCallback(self.shutdown_no_activity, 60000)
@@ -2409,7 +2372,6 @@ def initialize(
         self.init_configurables()
         self.init_components()
         self.init_webapp()
-        self.init_terminals()
         self.init_signal()
         self.init_ioloop()
         self.load_server_extensions()
@@ -2431,23 +2393,6 @@ async def cleanup_kernels(self):
         self.log.info(kernel_msg % n_kernels)
         await run_sync_in_loop(self.kernel_manager.shutdown_all())
 
-    async 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)
-        await run_sync_in_loop(terminal_manager.terminate_all())
-
     async def cleanup_extensions(self):
         """Call shutdown hooks in all extensions."""
         n_extensions = len(self.extension_manager.extension_apps)
@@ -2728,7 +2673,6 @@ async def _cleanup(self):
         self.remove_browser_open_files()
         await self.cleanup_extensions()
         await self.cleanup_kernels()
-        await self.cleanup_terminals()
 
     def start_ioloop(self):
         """Start the IO Loop."""

jupyter_server/terminal/__init__.py

@@ -1,52 +1,12 @@
-import os
-import sys
-from shutil import which
-
-import terminado
-
-from ..utils import check_version
-
-if not check_version(terminado.__version__, "0.8.3"):
-    raise ImportError("terminado >= 0.8.3 required, found %s" % terminado.__version__)
-
-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):
-    if os.name == "nt":
-        default_shell = "powershell.exe"
-    else:
-        default_shell = which("sh")
-    shell_override = settings.get("shell_command")
-    shell = [os.environ.get("SHELL") or default_shell] if shell_override is None else shell_override
-    # When the notebook server is not running in a terminal (e.g. when
-    # it's launched by a JupyterHub spawner), it's likely that the user
-    # environment hasn't been fully set up. In that case, run a login
-    # shell to automatically source /etc/profile and the like, unless
-    # 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"] = TerminalManager(
-        shell_command=shell,
-        extra_env={
-            "JUPYTER_SERVER_ROOT": root_dir,
-            "JUPYTER_SERVER_URL": connection_url,
-        },
-        parent=webapp.settings["serverapp"],
-    )
-    terminal_manager.log = webapp.settings["serverapp"].log
-    base_url = webapp.settings["base_url"]
-    handlers = [
-        (
-            ujoin(base_url, r"/terminals/websocket/(\w+)"),
-            TermSocket,
-            {"term_manager": terminal_manager},
-        ),
-        (ujoin(base_url, r"/api/terminals"), api_handlers.TerminalRootHandler),
-        (ujoin(base_url, r"/api/terminals/(\w+)"), api_handlers.TerminalHandler),
-    ]
-    webapp.add_handlers(".*$", handlers)
+import warnings
+
+# Shims
+from jupyter_server_terminals import api_handlers, initialize  # noqa
+from jupyter_server_terminals.handlers import TermSocket  # noqa
+from jupyter_server_terminals.terminalmanager import TerminalManager  # noqa
+
+warnings.warn(
+    "Terminals support has moved to `jupyter_server_terminals`",
+    DeprecationWarning,
+    stacklevel=2,
+)