Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Restore detection of missing terminado package #5465

Merged
merged 6 commits into from
May 25, 2020
Merged
Show file tree
Hide file tree
Changes from 4 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
43 changes: 28 additions & 15 deletions notebook/notebookapp.py
Original file line number Diff line number Diff line change
Expand Up @@ -91,7 +91,6 @@
from .auth.login import LoginHandler
from .auth.logout import LogoutHandler
from .base.handlers import FileFindHandler
from .terminal import TerminalManager

from traitlets.config import Config
from traitlets.config.application import catch_config_error, boolean_flag
Expand Down Expand Up @@ -132,6 +131,13 @@
except ImportError:
async_kernel_mgmt_available = False

# Tolerate missing terminado package.
try:
from .terminal import TerminalManager
terminado_available = True
except ImportError:
terminado_available = False

#-----------------------------------------------------------------------------
# Module globals
#-----------------------------------------------------------------------------
Expand Down Expand Up @@ -300,7 +306,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,
)

# allow custom overrides for the tornado web app.
Expand Down Expand Up @@ -660,9 +666,12 @@ class NotebookApp(JupyterApp):

classes = [
KernelManager, Session, MappingKernelManager, KernelSpecManager,
ContentsManager, FileContentsManager, NotebookNotary, TerminalManager,
ContentsManager, FileContentsManager, NotebookNotary,
GatewayKernelManager, GatewayKernelSpecManager, GatewaySessionManager, GatewayClient,
]
if terminado_available: # Only necessary when terminado is available
classes.append(TerminalManager)

flags = Dict(flags)
aliases = Dict(aliases)

Expand Down Expand Up @@ -1469,6 +1478,12 @@ def _update_server_extensions(self, change):
is not available.
"""))

# Since use of terminals is also a function of whether the terminado package is
# avaialble, 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.
terminals_in_use = False

def parse_command_line(self, argv=None):
super(NotebookApp, self).parse_command_line(argv)

Expand Down Expand Up @@ -1759,7 +1774,7 @@ def init_terminals(self):
try:
from .terminal import initialize
initialize(nb_app=self)
self.web_app.settings['terminals_available'] = True
self.terminals_in_use = True
except ImportError as e:
self.log.warning(_("Terminals not available (error was %s)"), e)

Expand Down Expand Up @@ -1908,13 +1923,12 @@ def shutdown_no_activity(self):
if len(km) != 0:
return # Kernels still running

try:
term_mgr = self.web_app.settings['terminal_manager']
except KeyError:
pass # Terminals not enabled
else:
if term_mgr.terminals:
return # Terminals still running
if not self.terminals_in_use:
return
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This doesn't look quite right. This method is to shut down the server after a certain time with no kernels/terminals. Terminals not in use means no terminals running, but this early return prevents it from ever shutting down.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

😄 Looks like we could do with another pass at naming, as you alluded to in another comment. I was coming to same conclusion about terminals_in_use yesterday (while doing yard work). I think a switch (alignment) to terminals_available might just be the best name:
a. terminado_available = Terminado package is installed
b. terminals_enabled = User has condoned the use of terminals (independent of terminado's presence)
c. terminals_available = User has condoned the use of terminals and terminado is installed

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

To be clear, I think there's an actual logic bug here, regardless of which name we're using. ;-)

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You're absolutely correct. Thank you for spotting that.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks. I think this is in good shape now. :-)


term_mgr = self.web_app.settings['terminal_manager']
if term_mgr.terminals:
return # Terminals still running

seconds_since_active = \
(utcnow() - self.web_app.last_activity()).total_seconds()
Expand Down Expand Up @@ -2000,11 +2014,10 @@ def cleanup_terminals(self):
The terminals will shutdown themselves when this process no longer exists,
but explicit shutdown allows the TerminalManager to cleanup.
"""
try:
terminal_manager = self.web_app.settings['terminal_manager']
except KeyError:
return # Terminals not enabled
if not self.terminals_in_use:
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)
Expand Down
5 changes: 2 additions & 3 deletions notebook/terminal/__init__.py
Original file line number Diff line number Diff line change
Expand Up @@ -3,11 +3,10 @@
import terminado
from ..utils import check_version

if not check_version(terminado.__version__, '0.8.1'):
raise ImportError("terminado >= 0.8.1 required, found %s" % terminado.__version__)
if not check_version(terminado.__version__, '0.8.3'):
raise ImportError("terminado >= 0.8.3 required, found %s" % terminado.__version__)

from ipython_genutils.py3compat import which
from tornado.log import app_log
from notebook.utils import url_path_join as ujoin
from .terminalmanager import TerminalManager
from .handlers import TerminalHandler, TermSocket
Expand Down