From a9ed339d584f8ac0d836adf4af98f3c6a7a43528 Mon Sep 17 00:00:00 2001 From: Kris Wilson Date: Tue, 20 Aug 2019 18:56:34 -0700 Subject: [PATCH 01/18] Add UNIX socket support to notebook server. --- notebook/__init__.py | 2 + notebook/base/handlers.py | 23 ++- notebook/notebookapp.py | 190 +++++++++++++++--- notebook/tests/launchnotebook.py | 64 +++++- notebook/tests/test_notebookapp.py | 14 +- .../tests/test_notebookapp_integration.py | 39 ++++ notebook/utils.py | 15 ++ setup.py | 3 +- 8 files changed, 305 insertions(+), 45 deletions(-) create mode 100644 notebook/tests/test_notebookapp_integration.py diff --git a/notebook/__init__.py b/notebook/__init__.py index f406f3c6eb..c72096f26b 100644 --- a/notebook/__init__.py +++ b/notebook/__init__.py @@ -20,6 +20,8 @@ os.path.join(os.path.dirname(__file__), "templates"), ] +DEFAULT_NOTEBOOK_PORT = 8888 + del os from .nbextensions import install_nbextension diff --git a/notebook/base/handlers.py b/notebook/base/handlers.py index c05d6ce60c..38665b9a63 100755 --- a/notebook/base/handlers.py +++ b/notebook/base/handlers.py @@ -32,7 +32,7 @@ import notebook from notebook._tz import utcnow from notebook.i18n import combine_translations -from notebook.utils import is_hidden, url_path_join, url_is_absolute, url_escape +from notebook.utils import is_hidden, url_path_join, url_is_absolute, url_escape, urldecode_unix_socket_path from notebook.services.security import csp_report_uri #----------------------------------------------------------------------------- @@ -471,13 +471,22 @@ def check_host(self): if host.startswith('[') and host.endswith(']'): host = host[1:-1] - try: - addr = ipaddress.ip_address(host) - except ValueError: - # Not an IP address: check against hostnames - allow = host in self.settings.get('local_hostnames', ['localhost']) + if not PY3: + # ip_address only accepts unicode on Python 2 + host = host.decode('utf8', 'replace') + + # UNIX socket handling + check_host = urldecode_unix_socket_path(host) + if check_host.startswith('/') and os.path.exists(check_host): + allow = True else: - allow = addr.is_loopback + try: + addr = ipaddress.ip_address(host) + except ValueError: + # Not an IP address: check against hostnames + allow = host in self.settings.get('local_hostnames', ['localhost']) + else: + allow = addr.is_loopback if not allow: self.log.warning( diff --git a/notebook/notebookapp.py b/notebook/notebookapp.py index 4fa3925422..5e17f93758 100755 --- a/notebook/notebookapp.py +++ b/notebook/notebookapp.py @@ -66,8 +66,11 @@ from tornado import web from tornado.httputil import url_concat from tornado.log import LogFormatter, app_log, access_log, gen_log +if not sys.platform.startswith('win'): + from tornado.netutil import bind_unix_socket from notebook import ( + DEFAULT_NOTEBOOK_PORT, DEFAULT_STATIC_FILES_PATH, DEFAULT_TEMPLATE_PATH_LIST, __version__, @@ -107,7 +110,17 @@ from notebook._sysinfo import get_sys_info from ._tz import utcnow, utcfromtimestamp -from .utils import url_path_join, check_pid, url_escape, urljoin, pathname2url, run_sync +from .utils import ( + check_pid, + pathname2url, + run_sync, + url_escape, + url_path_join, + urldecode_unix_socket_path, + urlencode_unix_socket, + urlencode_unix_socket_path, + urljoin, +) # Check if we can use async kernel management try: @@ -218,7 +231,7 @@ def init_settings(self, jupyter_app, kernel_manager, contents_manager, warnings.warn(_("The `ignore_minified_js` flag is deprecated and will be removed in Notebook 6.0"), DeprecationWarning) now = utcnow() - + root_dir = contents_manager.root_dir home = py3compat.str_to_unicode(os.path.expanduser('~'), encoding=sys.getfilesystemencoding()) if root_dir.startswith(home + os.path.sep): @@ -403,6 +416,7 @@ def start(self): set_password(config_file=self.config_file) self.log.info("Wrote hashed password to %s" % self.config_file) + def shutdown_server(server_info, timeout=5, log=None): """Shutdown a notebook server in a separate process. @@ -415,14 +429,39 @@ def shutdown_server(server_info, timeout=5, log=None): Returns True if the server was stopped by any means, False if stopping it failed (on Windows). """ - from tornado.httpclient import HTTPClient, HTTPRequest + from tornado import gen + from tornado.httpclient import AsyncHTTPClient, HTTPClient, HTTPRequest + from tornado.netutil import bind_unix_socket, Resolver url = server_info['url'] pid = server_info['pid'] + resolver = None + + # UNIX Socket handling. + if url.startswith('http+unix://'): + # This library doesn't understand our URI form, but it's just HTTP. + url = url.replace('http+unix://', 'http://') + + class UnixSocketResolver(Resolver): + def initialize(self, resolver): + self.resolver = resolver + + def close(self): + self.resolver.close() + + @gen.coroutine + def resolve(self, host, port, *args, **kwargs): + raise gen.Return([ + (socket.AF_UNIX, urldecode_unix_socket_path(host)) + ]) + + resolver = UnixSocketResolver(resolver=Resolver()) + req = HTTPRequest(url + 'api/shutdown', method='POST', body=b'', headers={ 'Authorization': 'token ' + server_info['token'] }) if log: log.debug("POST request to %sapi/shutdown", url) - HTTPClient().fetch(req) + AsyncHTTPClient.configure(None, resolver=resolver) + HTTPClient(AsyncHTTPClient).fetch(req) # Poll to see if it shut down. for _ in range(timeout*10): @@ -453,13 +492,20 @@ class NbserverStopApp(JupyterApp): version = __version__ description="Stop currently running notebook server for a given port" - port = Integer(8888, config=True, - help="Port of the server to be killed. Default 8888") + port = Integer(DEFAULT_NOTEBOOK_PORT, config=True, + help="Port of the server to be killed. Default %s" % DEFAULT_NOTEBOOK_PORT) + + sock = Unicode(u'', config=True, + help="UNIX socket of the server to be killed.") def parse_command_line(self, argv=None): super(NbserverStopApp, self).parse_command_line(argv) if self.extra_args: - self.port=int(self.extra_args[0]) + try: + self.port = int(self.extra_args[0]) + except ValueError: + # self.extra_args[0] was not an int, so it must be a string (unix socket). + self.sock = self.extra_args[0] def shutdown_server(self, server): return shutdown_server(server, log=self.log) @@ -469,16 +515,16 @@ def start(self): if not servers: self.exit("There are no running servers") for server in servers: - if server['port'] == self.port: - print("Shutting down server on port", self.port, "...") + if server.get('sock') == self.sock or server['port'] == self.port: + print("Shutting down server on %s..." % self.sock or self.port) if not self.shutdown_server(server): sys.exit("Could not stop server") return else: print("There is currently no server running on port {}".format(self.port), file=sys.stderr) - print("Ports currently in use:", file=sys.stderr) + print("Ports/sockets currently in use:", file=sys.stderr) for server in servers: - print(" - {}".format(server['port']), file=sys.stderr) + print(" - {}".format(server.get('sock', server['port'])), file=sys.stderr) self.exit(1) @@ -558,6 +604,8 @@ def start(self): 'ip': 'NotebookApp.ip', 'port': 'NotebookApp.port', 'port-retries': 'NotebookApp.port_retries', + 'sock': 'NotebookApp.sock', + 'sock-umask': 'NotebookApp.sock_umask', 'transport': 'KernelManager.transport', 'keyfile': 'NotebookApp.keyfile', 'certfile': 'NotebookApp.certfile', @@ -715,10 +763,18 @@ def _valdate_ip(self, proposal): or containerized setups for example).""") ) - port = Integer(8888, config=True, + port = Integer(DEFAULT_NOTEBOOK_PORT, config=True, help=_("The port the notebook server will listen on.") ) + sock = Unicode(u'', config=True, + help=_("The UNIX socket the notebook server will listen on.") + ) + + sock_umask = Unicode(u'0600', config=True, + help=_("The UNIX socket umask to set on creation (default: 0600).") + ) + port_retries = Integer(50, config=True, help=_("The number of additional ports to try if the specified port is not available.") ) @@ -1469,6 +1525,27 @@ def init_webapp(self): self.log.critical(_("\t$ python -m notebook.auth password")) sys.exit(1) + # Socket options validation. + if self.sock: + if self.port != DEFAULT_NOTEBOOK_PORT: + self.log.critical( + _('Options --port and --sock are mutually exclusive. Aborting.'), + ) + sys.exit(1) + + if self.open_browser: + # If we're bound to a UNIX socket, we can't reliably connect from a browser. + self.log.critical( + _('Options --open-browser and --sock are mutually exclusive. Aborting.'), + ) + sys.exit(1) + + if sys.platform.startswith('win'): + self.log.critical( + _('Option --sock is not supported on Windows, but got value of %s. Aborting.' % self.sock), + ) + sys.exit(1) + self.web_app = NotebookWebApplication( self, self.kernel_manager, self.contents_manager, self.session_manager, self.kernel_spec_manager, @@ -1505,6 +1582,32 @@ def init_webapp(self): max_body_size=self.max_body_size, max_buffer_size=self.max_buffer_size) + success = self._bind_http_server() + if not success: + self.log.critical(_('ERROR: the notebook server could not be started because ' + 'no available port could be found.')) + self.exit(1) + + def _bind_http_server(self): + return self._bind_http_server_unix() if self.sock else self._bind_http_server_tcp() + + def _bind_http_server_unix(self): + try: + sock = bind_unix_socket(self.sock, mode=int(self.sock_umask.encode(), 8)) + self.http_server.add_socket(sock) + except socket.error as e: + if e.errno == errno.EADDRINUSE: + self.log.info(_('The socket %s is already in use.') % self.sock) + return False + elif e.errno in (errno.EACCES, getattr(errno, 'WSAEACCES', errno.EACCES)): + self.log.warning(_("Permission to listen on sock %s denied") % self.sock) + return False + else: + raise + else: + return True + + def _bind_http_server_tcp(self): success = None for port in random_ports(self.port, self.port_retries+1): try: @@ -1533,35 +1636,45 @@ def init_webapp(self): self.log.critical(_('ERROR: the notebook server could not be started because ' 'port %i is not available.') % port) self.exit(1) - + return success + + def _concat_token(self, url): + token = self.token if self._token_generated else '...' + return url_concat(url, {'token': token}) + @property def display_url(self): if self.custom_display_url: url = self.custom_display_url if not url.endswith('/'): url += '/' + elif self.sock: + url = self._unix_sock_url() else: if self.ip in ('', '0.0.0.0'): ip = "%s" % socket.gethostname() else: ip = self.ip - url = self._url(ip) - if self.token: - # Don't log full token if it came from config - token = self.token if self._token_generated else '...' - url = (url_concat(url, {'token': token}) - + '\n or ' - + url_concat(self._url('127.0.0.1'), {'token': token})) + url = self._tcp_url(ip) + if self.token and not self.sock: + url = self._concat_token(url) + url += '\n or %s' % self._concat_token(self._tcp_url('127.0.0.1')) return url @property def connection_url(self): - ip = self.ip if self.ip else 'localhost' - return self._url(ip) + if self.sock: + return self._unix_sock_url() + else: + ip = self.ip if self.ip else 'localhost' + return self._tcp_url(ip) - def _url(self, ip): + def _unix_sock_url(self, token=None): + return '%s%s' % (urlencode_unix_socket(self.sock), self.base_url) + + def _tcp_url(self, ip, port=None): proto = 'https' if self.certfile else 'http' - return "%s://%s:%i%s" % (proto, ip, self.port, self.base_url) + return "%s://%s:%i%s" % (proto, ip, port or self.port, self.base_url) def init_terminals(self): if not self.terminals_enabled: @@ -1825,6 +1938,7 @@ def server_info(self): return {'url': self.connection_url, 'hostname': self.ip if self.ip else 'localhost', 'port': self.port, + 'sock': self.sock, 'secure': bool(self.certfile), 'base_url': self.base_url, 'token': self.token, @@ -1954,19 +2068,31 @@ def start(self): self.write_server_info_file() self.write_browser_open_file() - if self.open_browser or self.file_to_run: + if (self.open_browser or self.file_to_run) and not self.sock: self.launch_browser() if self.token and self._token_generated: # log full URL with generated token, so there's a copy/pasteable link # with auth info. - self.log.critical('\n'.join([ - '\n', - 'To access the notebook, open this file in a browser:', - ' %s' % urljoin('file:', pathname2url(self.browser_open_file)), - 'Or copy and paste one of these URLs:', - ' %s' % self.display_url, - ])) + if self.sock: + self.log.critical('\n'.join([ + '\n', + 'Notebook is listening on %s' % self.display_url, + '', + ( + 'UNIX sockets are not browser-connectable, but you can tunnel to ' + 'the instance via e.g.`ssh -L 8888:%s -N user@this_host` and then ' + 'opening e.g. %s in a browser.' + ) % (self.sock, self._concat_token(self._tcp_url('localhost', 8888))) + ])) + else: + self.log.critical('\n'.join([ + '\n', + 'To access the notebook, open this file in a browser:', + ' %s' % urljoin('file:', pathname2url(self.browser_open_file)), + 'Or copy and paste one of these URLs:', + ' %s' % self.display_url, + ])) self.io_loop = ioloop.IOLoop.current() if sys.platform.startswith('win'): diff --git a/notebook/tests/launchnotebook.py b/notebook/tests/launchnotebook.py index b873457316..1a7f999f37 100644 --- a/notebook/tests/launchnotebook.py +++ b/notebook/tests/launchnotebook.py @@ -16,12 +16,13 @@ from unittest.mock import patch import requests +import requests_unixsocket from tornado.ioloop import IOLoop import zmq import jupyter_core.paths from traitlets.config import Config -from ..notebookapp import NotebookApp +from ..notebookapp import NotebookApp, urlencode_unix_socket from ..utils import url_path_join from ipython_genutils.tempdir import TemporaryDirectory @@ -52,7 +53,7 @@ def wait_until_alive(cls): url = cls.base_url() + 'api/contents' for _ in range(int(MAX_WAITTIME/POLL_INTERVAL)): try: - requests.get(url) + cls.fetch_url(url) except Exception as e: if not cls.notebook_thread.is_alive(): raise RuntimeError("The notebook server failed to start") @@ -76,6 +77,10 @@ def auth_headers(cls): headers['Authorization'] = 'token %s' % cls.token return headers + @staticmethod + def fetch_url(url): + return requests.get(url) + @classmethod def request(cls, verb, path, **kwargs): """Send a request to my server @@ -104,7 +109,11 @@ def get_patch_env(cls): @classmethod def get_argv(cls): return [] - + + @classmethod + def get_bind_args(cls): + return dict(port=cls.port) + @classmethod def setup_class(cls): cls.tmp_dir = TemporaryDirectory() @@ -116,7 +125,7 @@ def tmp(*parts): if e.errno != errno.EEXIST: raise return path - + cls.home_dir = tmp('home') data_dir = cls.data_dir = tmp('data') config_dir = cls.config_dir = tmp('config') @@ -143,6 +152,34 @@ def tmp(*parts): started = Event() def start_thread(): + if 'asyncio' in sys.modules: + import asyncio + asyncio.set_event_loop(asyncio.new_event_loop()) + bind_args = cls.get_bind_args() + app = cls.notebook = NotebookApp( + port_retries=0, + open_browser=False, + config_dir=cls.config_dir, + data_dir=cls.data_dir, + runtime_dir=cls.runtime_dir, + notebook_dir=cls.notebook_dir, + base_url=cls.url_prefix, + config=config, + allow_root=True, + token=cls.token, + **bind_args + ) + # don't register signal handler during tests + app.init_signal = lambda : None + # clear log handlers and propagate to root for nose to capture it + # needs to be redone after initialize, which reconfigures logging + app.log.propagate = True + app.log.handlers = [] + app.initialize(argv=cls.get_argv()) + app.log.propagate = True + app.log.handlers = [] + loop = IOLoop.current() + loop.add_callback(started.set) try: app = cls.notebook = NotebookApp( port=cls.port, @@ -206,6 +243,25 @@ def base_url(cls): return 'http://localhost:%i%s' % (cls.port, cls.url_prefix) +class UNIXSocketNotebookTestBase(NotebookTestBase): + # Rely on `/tmp` to avoid any Linux socket length max buffer + # issues. Key on PID for process-wise concurrency. + sock = '/tmp/.notebook.%i.sock' % os.getpid() + + @classmethod + def get_bind_args(cls): + return dict(sock=cls.sock) + + @classmethod + def base_url(cls): + return '%s%s' % (urlencode_unix_socket(cls.sock), cls.url_prefix) + + @staticmethod + def fetch_url(url): + with requests_unixsocket.monkeypatch(): + return requests.get(url) + + @contextmanager def assert_http_error(status, msg=None): try: diff --git a/notebook/tests/test_notebookapp.py b/notebook/tests/test_notebookapp.py index f3066d9fb7..d8543feed9 100644 --- a/notebook/tests/test_notebookapp.py +++ b/notebook/tests/test_notebookapp.py @@ -22,7 +22,7 @@ from notebook.auth.security import passwd_check NotebookApp = notebookapp.NotebookApp -from .launchnotebook import NotebookTestBase +from .launchnotebook import NotebookTestBase, UNIXSocketNotebookTestBase def test_help_output(): @@ -189,3 +189,15 @@ def test_list_running_servers(self): servers = list(notebookapp.list_running_servers()) assert len(servers) >= 1 assert self.port in {info['port'] for info in servers} + + +# UNIX sockets aren't available on Windows. +if not sys.platform.startswith('win'): + class NotebookUnixSocketTests(UNIXSocketNotebookTestBase): + def test_run(self): + self.fetch_url(self.base_url() + 'api/contents') + + def test_list_running_sock_servers(self): + servers = list(notebookapp.list_running_servers()) + assert len(servers) >= 1 + assert self.sock in {info['sock'] for info in servers} diff --git a/notebook/tests/test_notebookapp_integration.py b/notebook/tests/test_notebookapp_integration.py new file mode 100644 index 0000000000..5a06901321 --- /dev/null +++ b/notebook/tests/test_notebookapp_integration.py @@ -0,0 +1,39 @@ +import os +import stat +import subprocess +import time + +from ipython_genutils.testing.decorators import skip_win32 + +from .launchnotebook import UNIXSocketNotebookTestBase +from ..utils import urlencode_unix_socket, urlencode_unix_socket_path + + +@skip_win32 +def test_shutdown_sock_server_integration(): + sock = UNIXSocketNotebookTestBase.sock + url = urlencode_unix_socket(sock) + encoded_sock_path = urlencode_unix_socket_path(sock) + + p = subprocess.Popen( + ['jupyter', 'notebook', '--no-browser', '--sock=%s' % sock], + stdout=subprocess.PIPE, stderr=subprocess.PIPE + ) + + for line in iter(p.stderr.readline, b''): + if url.encode() in line: + complete = True + break + + assert complete, 'did not find socket URL in stdout when launching notebook' + + assert encoded_sock_path.encode() in subprocess.check_output(['jupyter', 'notebook', 'list']) + + # Ensure default umask is properly applied. + assert stat.S_IMODE(os.lstat(sock).st_mode) == 0o600 + + subprocess.check_output(['jupyter', 'notebook', 'stop', sock]) + + assert encoded_sock_path.encode() not in subprocess.check_output(['jupyter', 'notebook', 'list']) + + p.wait() diff --git a/notebook/utils.py b/notebook/utils.py index 9ec10773fb..47d7a26faf 100644 --- a/notebook/utils.py +++ b/notebook/utils.py @@ -367,3 +367,18 @@ def wrapped(): result = asyncio.ensure_future(maybe_async) return result return wrapped() + + +def urlencode_unix_socket_path(socket_path): + """Encodes a UNIX socket path string from a socket path for the `http+unix` URI form.""" + return socket_path.replace('/', '%2F') + + +def urldecode_unix_socket_path(socket_path): + """Decodes a UNIX sock path string from an encoded sock path for the `http+unix` URI form.""" + return socket_path.replace('%2F', '/') + + +def urlencode_unix_socket(socket_path): + """Encodes a UNIX socket URL from a socket path for the `http+unix` URI form.""" + return 'http+unix://%s' % urlencode_unix_socket_path(socket_path) diff --git a/setup.py b/setup.py index a2f07d0023..c970aac746 100755 --- a/setup.py +++ b/setup.py @@ -115,7 +115,8 @@ ], extras_require = { 'test': ['nose', 'coverage', 'requests', 'nose_warnings_filters', - 'nbval', 'nose-exclude', 'selenium', 'pytest', 'pytest-cov'], + 'nbval', 'nose-exclude', 'selenium', 'pytest', 'pytest-cov', + 'requests-unixsocket'], 'test:sys_platform == "win32"': ['nose-exclude'], }, python_requires = '>=3.5', From 20b5cb8e4de5536c2cec40251d092625b3544950 Mon Sep 17 00:00:00 2001 From: Kris Wilson Date: Tue, 28 Apr 2020 01:00:54 -0700 Subject: [PATCH 02/18] Fixup. --- notebook/tests/launchnotebook.py | 28 ---------------------------- 1 file changed, 28 deletions(-) diff --git a/notebook/tests/launchnotebook.py b/notebook/tests/launchnotebook.py index 1a7f999f37..a7b65a1aac 100644 --- a/notebook/tests/launchnotebook.py +++ b/notebook/tests/launchnotebook.py @@ -152,34 +152,6 @@ def tmp(*parts): started = Event() def start_thread(): - if 'asyncio' in sys.modules: - import asyncio - asyncio.set_event_loop(asyncio.new_event_loop()) - bind_args = cls.get_bind_args() - app = cls.notebook = NotebookApp( - port_retries=0, - open_browser=False, - config_dir=cls.config_dir, - data_dir=cls.data_dir, - runtime_dir=cls.runtime_dir, - notebook_dir=cls.notebook_dir, - base_url=cls.url_prefix, - config=config, - allow_root=True, - token=cls.token, - **bind_args - ) - # don't register signal handler during tests - app.init_signal = lambda : None - # clear log handlers and propagate to root for nose to capture it - # needs to be redone after initialize, which reconfigures logging - app.log.propagate = True - app.log.handlers = [] - app.initialize(argv=cls.get_argv()) - app.log.propagate = True - app.log.handlers = [] - loop = IOLoop.current() - loop.add_callback(started.set) try: app = cls.notebook = NotebookApp( port=cls.port, From 8aad32477304419c7a486e9f3fd8b66fbc94126f Mon Sep 17 00:00:00 2001 From: Kris Wilson Date: Tue, 28 Apr 2020 01:11:24 -0700 Subject: [PATCH 03/18] Fixup. --- notebook/base/handlers.py | 4 ---- 1 file changed, 4 deletions(-) diff --git a/notebook/base/handlers.py b/notebook/base/handlers.py index 38665b9a63..b8d7a3810c 100755 --- a/notebook/base/handlers.py +++ b/notebook/base/handlers.py @@ -471,10 +471,6 @@ def check_host(self): if host.startswith('[') and host.endswith(']'): host = host[1:-1] - if not PY3: - # ip_address only accepts unicode on Python 2 - host = host.decode('utf8', 'replace') - # UNIX socket handling check_host = urldecode_unix_socket_path(host) if check_host.startswith('/') and os.path.exists(check_host): From b9567512f8700f66cd71f0643b5bace0c1de23d5 Mon Sep 17 00:00:00 2001 From: Kris Wilson Date: Tue, 28 Apr 2020 01:55:18 -0700 Subject: [PATCH 04/18] Fixup tests. --- notebook/tests/launchnotebook.py | 3 ++- notebook/tests/test_notebookapp_integration.py | 8 ++++---- 2 files changed, 6 insertions(+), 5 deletions(-) diff --git a/notebook/tests/launchnotebook.py b/notebook/tests/launchnotebook.py index a7b65a1aac..41d0131717 100644 --- a/notebook/tests/launchnotebook.py +++ b/notebook/tests/launchnotebook.py @@ -153,8 +153,8 @@ def tmp(*parts): started = Event() def start_thread(): try: + bind_args = cls.get_bind_args() app = cls.notebook = NotebookApp( - port=cls.port, port_retries=0, open_browser=False, config_dir=cls.config_dir, @@ -165,6 +165,7 @@ def start_thread(): config=config, allow_root=True, token=cls.token, + **bind_args ) if 'asyncio' in sys.modules: app._init_asyncio_patch() diff --git a/notebook/tests/test_notebookapp_integration.py b/notebook/tests/test_notebookapp_integration.py index 5a06901321..c11a8f15e0 100644 --- a/notebook/tests/test_notebookapp_integration.py +++ b/notebook/tests/test_notebookapp_integration.py @@ -16,7 +16,7 @@ def test_shutdown_sock_server_integration(): encoded_sock_path = urlencode_unix_socket_path(sock) p = subprocess.Popen( - ['jupyter', 'notebook', '--no-browser', '--sock=%s' % sock], + ['jupyter-notebook', '--no-browser', '--sock=%s' % sock], stdout=subprocess.PIPE, stderr=subprocess.PIPE ) @@ -27,13 +27,13 @@ def test_shutdown_sock_server_integration(): assert complete, 'did not find socket URL in stdout when launching notebook' - assert encoded_sock_path.encode() in subprocess.check_output(['jupyter', 'notebook', 'list']) + assert encoded_sock_path.encode() in subprocess.check_output(['jupyter-notebook', 'list']) # Ensure default umask is properly applied. assert stat.S_IMODE(os.lstat(sock).st_mode) == 0o600 - subprocess.check_output(['jupyter', 'notebook', 'stop', sock]) + subprocess.check_output(['jupyter-notebook', 'stop', sock]) - assert encoded_sock_path.encode() not in subprocess.check_output(['jupyter', 'notebook', 'list']) + assert encoded_sock_path.encode() not in subprocess.check_output(['jupyter-notebook', 'list']) p.wait() From 9bda4c61b96bb483165d873f083efab5e9a2e516 Mon Sep 17 00:00:00 2001 From: Kris Wilson Date: Tue, 28 Apr 2020 19:04:55 -0700 Subject: [PATCH 05/18] Feedback: Improve stop server description. --- notebook/notebookapp.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/notebook/notebookapp.py b/notebook/notebookapp.py index 5e17f93758..cacc3d5e18 100755 --- a/notebook/notebookapp.py +++ b/notebook/notebookapp.py @@ -490,7 +490,7 @@ def resolve(self, host, port, *args, **kwargs): class NbserverStopApp(JupyterApp): version = __version__ - description="Stop currently running notebook server for a given port" + description="Stop currently running notebook server." port = Integer(DEFAULT_NOTEBOOK_PORT, config=True, help="Port of the server to be killed. Default %s" % DEFAULT_NOTEBOOK_PORT) From 4af22581fc976e83c83adb48d870ba06d432d577 Mon Sep 17 00:00:00 2001 From: Kris Wilson Date: Tue, 28 Apr 2020 23:45:08 -0700 Subject: [PATCH 06/18] Feedback: repair stop command port targeting. --- notebook/notebookapp.py | 34 +++++++--- .../tests/test_notebookapp_integration.py | 68 ++++++++++++++++++- 2 files changed, 92 insertions(+), 10 deletions(-) diff --git a/notebook/notebookapp.py b/notebook/notebookapp.py index cacc3d5e18..07119857e6 100755 --- a/notebook/notebookapp.py +++ b/notebook/notebookapp.py @@ -510,21 +510,36 @@ def parse_command_line(self, argv=None): def shutdown_server(self, server): return shutdown_server(server, log=self.log) + def _shutdown_or_exit(self, target_endpoint, server): + print("Shutting down server on %s..." % target_endpoint) + if not self.shutdown_server(server): + sys.exit("Could not stop server on %s" % target_endpoint) + def start(self): servers = list(list_running_servers(self.runtime_dir)) if not servers: - self.exit("There are no running servers") + self.exit("There are no running servers (per %s)" % self.runtime_dir) + for server in servers: - if server.get('sock') == self.sock or server['port'] == self.port: - print("Shutting down server on %s..." % self.sock or self.port) - if not self.shutdown_server(server): - sys.exit("Could not stop server") - return + if self.sock: + sock = server.get('sock', None) + if sock and sock == self.sock: + self._shutdown_or_exit(sock, server) + return + elif self.port: + port = server.get('port', None) + if port == self.port: + self._shutdown_or_exit(port, server) + return else: - print("There is currently no server running on port {}".format(self.port), file=sys.stderr) + current_endpoint = self.sock or self.port + print( + "There is currently no server running on {}".format(current_endpoint), + file=sys.stderr + ) print("Ports/sockets currently in use:", file=sys.stderr) for server in servers: - print(" - {}".format(server.get('sock', server['port'])), file=sys.stderr) + print(" - {}".format(server.get('sock') or server['port']), file=sys.stderr) self.exit(1) @@ -1532,6 +1547,9 @@ def init_webapp(self): _('Options --port and --sock are mutually exclusive. Aborting.'), ) sys.exit(1) + else: + # Reset the default port if we're using a UNIX socket. + self.port = 0 if self.open_browser: # If we're bound to a UNIX socket, we can't reliably connect from a browser. diff --git a/notebook/tests/test_notebookapp_integration.py b/notebook/tests/test_notebookapp_integration.py index c11a8f15e0..69df0ba70a 100644 --- a/notebook/tests/test_notebookapp_integration.py +++ b/notebook/tests/test_notebookapp_integration.py @@ -4,6 +4,7 @@ import time from ipython_genutils.testing.decorators import skip_win32 +from notebook import DEFAULT_NOTEBOOK_PORT from .launchnotebook import UNIXSocketNotebookTestBase from ..utils import urlencode_unix_socket, urlencode_unix_socket_path @@ -12,7 +13,7 @@ @skip_win32 def test_shutdown_sock_server_integration(): sock = UNIXSocketNotebookTestBase.sock - url = urlencode_unix_socket(sock) + url = urlencode_unix_socket(sock).encode() encoded_sock_path = urlencode_unix_socket_path(sock) p = subprocess.Popen( @@ -21,7 +22,7 @@ def test_shutdown_sock_server_integration(): ) for line in iter(p.stderr.readline, b''): - if url.encode() in line: + if url in line: complete = True break @@ -37,3 +38,66 @@ def test_shutdown_sock_server_integration(): assert encoded_sock_path.encode() not in subprocess.check_output(['jupyter-notebook', 'list']) p.wait() + + + +def _ensure_stopped(check_msg='There are no running servers'): + try: + subprocess.check_output( + ['jupyter-notebook', 'stop'], + stderr=subprocess.STDOUT + ) + except subprocess.CalledProcessError as e: + assert check_msg in e.output.decode() + else: + raise AssertionError('expected all servers to be stopped') + + +@skip_win32 +def test_stop_multi(): + """Tests lifecycle behavior for mixed-mode server types w/ default ports. + + Suitable for local dev testing only due to reliance on default port binding. + """ + TEST_PORT = '9797' + MSG_TMPL = 'Shutting down server on {}...' + + _ensure_stopped() + + # Default port. + p1 = subprocess.Popen( + ['jupyter-notebook', '--no-browser'] + ) + + # Unix socket. + sock = UNIXSocketNotebookTestBase.sock + p2 = subprocess.Popen( + ['jupyter-notebook', '--no-browser', '--sock=%s' % sock] + ) + + # Specified port + p3 = subprocess.Popen( + ['jupyter-notebook', '--no-browser', '--port=%s' % TEST_PORT] + ) + + time.sleep(3) + + assert MSG_TMPL.format(DEFAULT_NOTEBOOK_PORT) in subprocess.check_output( + ['jupyter-notebook', 'stop'] + ).decode() + + _ensure_stopped('There is currently no server running on 8888') + + assert MSG_TMPL.format(sock) in subprocess.check_output( + ['jupyter-notebook', 'stop', sock] + ).decode() + + assert MSG_TMPL.format(TEST_PORT) in subprocess.check_output( + ['jupyter-notebook', 'stop', TEST_PORT] + ).decode() + + _ensure_stopped() + + p1.wait() + p2.wait() + p3.wait() From 07deb1d865918632e6d0d6cfa0415aaf1f96ef95 Mon Sep 17 00:00:00 2001 From: Kris Wilson Date: Tue, 28 Apr 2020 23:48:59 -0700 Subject: [PATCH 07/18] Feedback: remove error on --sock and --open-browser. --- notebook/notebookapp.py | 5 ++--- 1 file changed, 2 insertions(+), 3 deletions(-) diff --git a/notebook/notebookapp.py b/notebook/notebookapp.py index 07119857e6..726ec64912 100755 --- a/notebook/notebookapp.py +++ b/notebook/notebookapp.py @@ -1553,10 +1553,9 @@ def init_webapp(self): if self.open_browser: # If we're bound to a UNIX socket, we can't reliably connect from a browser. - self.log.critical( - _('Options --open-browser and --sock are mutually exclusive. Aborting.'), + self.log.warning( + _('Ignoring --NotebookApp.open_browser due to --sock being used.'), ) - sys.exit(1) if sys.platform.startswith('win'): self.log.critical( From 328dca54969130397f7a3cf59c0777baecb557f4 Mon Sep 17 00:00:00 2001 From: Kris Wilson Date: Tue, 28 Apr 2020 23:53:20 -0700 Subject: [PATCH 08/18] Feedback: file_to_run and sock mutual exclusion. --- notebook/notebookapp.py | 6 ++++++ 1 file changed, 6 insertions(+) diff --git a/notebook/notebookapp.py b/notebook/notebookapp.py index 726ec64912..100dfa1c9b 100755 --- a/notebook/notebookapp.py +++ b/notebook/notebookapp.py @@ -1557,6 +1557,12 @@ def init_webapp(self): _('Ignoring --NotebookApp.open_browser due to --sock being used.'), ) + if self.file_to_run: + self.log.critical( + _('Options --NotebookApp.file_to_run and --sock are mutually exclusive.'), + ) + sys.exit(1) + if sys.platform.startswith('win'): self.log.critical( _('Option --sock is not supported on Windows, but got value of %s. Aborting.' % self.sock), From 592af9a52681f46b68ce99debe7d856e82589907 Mon Sep 17 00:00:00 2001 From: Kris Wilson Date: Tue, 28 Apr 2020 23:53:33 -0700 Subject: [PATCH 09/18] Feedback: opening. --- notebook/notebookapp.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/notebook/notebookapp.py b/notebook/notebookapp.py index 100dfa1c9b..37e0b3ee04 100755 --- a/notebook/notebookapp.py +++ b/notebook/notebookapp.py @@ -2105,7 +2105,7 @@ def start(self): ( 'UNIX sockets are not browser-connectable, but you can tunnel to ' 'the instance via e.g.`ssh -L 8888:%s -N user@this_host` and then ' - 'opening e.g. %s in a browser.' + 'open e.g. %s in a browser.' ) % (self.sock, self._concat_token(self._tcp_url('localhost', 8888))) ])) else: From bbf6c77227310633dd1b103f3ba2c4f82d9916a1 Mon Sep 17 00:00:00 2001 From: Kris Wilson Date: Tue, 28 Apr 2020 23:55:50 -0700 Subject: [PATCH 10/18] Feedback: umask -> mode. --- notebook/notebookapp.py | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/notebook/notebookapp.py b/notebook/notebookapp.py index 37e0b3ee04..d63c383f15 100755 --- a/notebook/notebookapp.py +++ b/notebook/notebookapp.py @@ -620,7 +620,7 @@ def start(self): 'port': 'NotebookApp.port', 'port-retries': 'NotebookApp.port_retries', 'sock': 'NotebookApp.sock', - 'sock-umask': 'NotebookApp.sock_umask', + 'sock-mode': 'NotebookApp.sock_mode', 'transport': 'KernelManager.transport', 'keyfile': 'NotebookApp.keyfile', 'certfile': 'NotebookApp.certfile', @@ -786,8 +786,8 @@ def _valdate_ip(self, proposal): help=_("The UNIX socket the notebook server will listen on.") ) - sock_umask = Unicode(u'0600', config=True, - help=_("The UNIX socket umask to set on creation (default: 0600).") + sock_mode = Unicode(u'0600', config=True, + help=_("The permissions mode/umask for UNIX socket creation (default: 0600).") ) port_retries = Integer(50, config=True, @@ -1616,7 +1616,7 @@ def _bind_http_server(self): def _bind_http_server_unix(self): try: - sock = bind_unix_socket(self.sock, mode=int(self.sock_umask.encode(), 8)) + sock = bind_unix_socket(self.sock, mode=int(self.sock_mode.encode(), 8)) self.http_server.add_socket(sock) except socket.error as e: if e.errno == errno.EADDRINUSE: From 8d9b8c6f06f6463926275a05b0040c6e6dd0c36a Mon Sep 17 00:00:00 2001 From: Kris Wilson Date: Tue, 28 Apr 2020 23:56:47 -0700 Subject: [PATCH 11/18] Cleanup. --- notebook/tests/test_notebookapp_integration.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/notebook/tests/test_notebookapp_integration.py b/notebook/tests/test_notebookapp_integration.py index 69df0ba70a..e07c7073b2 100644 --- a/notebook/tests/test_notebookapp_integration.py +++ b/notebook/tests/test_notebookapp_integration.py @@ -57,7 +57,7 @@ def _ensure_stopped(check_msg='There are no running servers'): def test_stop_multi(): """Tests lifecycle behavior for mixed-mode server types w/ default ports. - Suitable for local dev testing only due to reliance on default port binding. + Mostly suitable for local dev testing due to reliance on default port binding. """ TEST_PORT = '9797' MSG_TMPL = 'Shutting down server on {}...' From 3e28cfd603c8898560088531844f771f7c8e8cfe Mon Sep 17 00:00:00 2001 From: Kris Wilson Date: Wed, 29 Apr 2020 00:06:55 -0700 Subject: [PATCH 12/18] Feedback: improve test coverage. --- notebook/tests/test_notebookapp_integration.py | 15 ++++++++++++--- 1 file changed, 12 insertions(+), 3 deletions(-) diff --git a/notebook/tests/test_notebookapp_integration.py b/notebook/tests/test_notebookapp_integration.py index e07c7073b2..4a132261b4 100644 --- a/notebook/tests/test_notebookapp_integration.py +++ b/notebook/tests/test_notebookapp_integration.py @@ -17,7 +17,7 @@ def test_shutdown_sock_server_integration(): encoded_sock_path = urlencode_unix_socket_path(sock) p = subprocess.Popen( - ['jupyter-notebook', '--no-browser', '--sock=%s' % sock], + ['jupyter-notebook', '--sock=%s' % sock], stdout=subprocess.PIPE, stderr=subprocess.PIPE ) @@ -33,6 +33,15 @@ def test_shutdown_sock_server_integration(): # Ensure default umask is properly applied. assert stat.S_IMODE(os.lstat(sock).st_mode) == 0o600 + try: + subprocess.check_output(['jupyter-notebook', 'stop'], stderr=subprocess.STDOUT) + except subprocess.CalledProcessError as e: + assert 'There is currently no server running on' in e.output.decode() + else: + raise AssertionError('expected stop command to fail due to target mis-match') + + assert encoded_sock_path.encode() in subprocess.check_output(['jupyter-notebook', 'list']) + subprocess.check_output(['jupyter-notebook', 'stop', sock]) assert encoded_sock_path.encode() not in subprocess.check_output(['jupyter-notebook', 'list']) @@ -54,7 +63,7 @@ def _ensure_stopped(check_msg='There are no running servers'): @skip_win32 -def test_stop_multi(): +def test_stop_multi_integration(): """Tests lifecycle behavior for mixed-mode server types w/ default ports. Mostly suitable for local dev testing due to reliance on default port binding. @@ -72,7 +81,7 @@ def test_stop_multi(): # Unix socket. sock = UNIXSocketNotebookTestBase.sock p2 = subprocess.Popen( - ['jupyter-notebook', '--no-browser', '--sock=%s' % sock] + ['jupyter-notebook', '--sock=%s' % sock] ) # Specified port From 1a5eed86783588c0f15d2670aa821abc035c9c68 Mon Sep 17 00:00:00 2001 From: Kris Wilson Date: Wed, 29 Apr 2020 00:22:36 -0700 Subject: [PATCH 13/18] Make integration test conditional for CI compatibility. --- notebook/tests/test_notebookapp_integration.py | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/notebook/tests/test_notebookapp_integration.py b/notebook/tests/test_notebookapp_integration.py index 4a132261b4..632dd3c751 100644 --- a/notebook/tests/test_notebookapp_integration.py +++ b/notebook/tests/test_notebookapp_integration.py @@ -3,7 +3,7 @@ import subprocess import time -from ipython_genutils.testing.decorators import skip_win32 +from ipython_genutils.testing.decorators import skip_win32, onlyif from notebook import DEFAULT_NOTEBOOK_PORT from .launchnotebook import UNIXSocketNotebookTestBase @@ -62,7 +62,7 @@ def _ensure_stopped(check_msg='There are no running servers'): raise AssertionError('expected all servers to be stopped') -@skip_win32 +@onlyif(bool(os.environ.get('RUN_NB_INTEGRATION_TESTS', False)), 'for local testing') def test_stop_multi_integration(): """Tests lifecycle behavior for mixed-mode server types w/ default ports. From ee509ad2d9e9edc86626fff69360b085765f8fd1 Mon Sep 17 00:00:00 2001 From: Kris Wilson Date: Tue, 12 May 2020 22:12:20 -0700 Subject: [PATCH 14/18] Feedback: Demote log level on --open_browser and --sock. --- notebook/notebookapp.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/notebook/notebookapp.py b/notebook/notebookapp.py index d63c383f15..6aab61bca7 100755 --- a/notebook/notebookapp.py +++ b/notebook/notebookapp.py @@ -1553,7 +1553,7 @@ def init_webapp(self): if self.open_browser: # If we're bound to a UNIX socket, we can't reliably connect from a browser. - self.log.warning( + self.log.info( _('Ignoring --NotebookApp.open_browser due to --sock being used.'), ) From 1af8283afa5d046daa473d466bd92d9209b6fd1b Mon Sep 17 00:00:00 2001 From: Kris Wilson Date: Wed, 13 May 2020 00:01:52 -0700 Subject: [PATCH 15/18] Feedback: validate sock-mode. --- notebook/notebookapp.py | 27 +++++++++++++--- .../tests/test_notebookapp_integration.py | 31 +++++++++++++++++-- 2 files changed, 51 insertions(+), 7 deletions(-) diff --git a/notebook/notebookapp.py b/notebook/notebookapp.py index 6aab61bca7..87ba0ee9f0 100755 --- a/notebook/notebookapp.py +++ b/notebook/notebookapp.py @@ -26,6 +26,7 @@ import select import signal import socket +import stat import sys import tempfile import threading @@ -759,7 +760,7 @@ def _default_ip(self): return 'localhost' @validate('ip') - def _valdate_ip(self, proposal): + def _validate_ip(self, proposal): value = proposal['value'] if value == u'*': value = u'' @@ -786,10 +787,28 @@ def _valdate_ip(self, proposal): help=_("The UNIX socket the notebook server will listen on.") ) - sock_mode = Unicode(u'0600', config=True, - help=_("The permissions mode/umask for UNIX socket creation (default: 0600).") + sock_mode = Integer(int('0600', 8), config=True, + help=_("The permissions mode for UNIX socket creation (default: 0600).") ) + @validate('sock_mode') + def _validate_sock_mode(self, proposal): + value = proposal['value'] + try: + converted_value = int(value.encode(), 8) + # Ensure the mode is at least user readable/writable. + assert all(( + bool(converted_value & stat.S_IRUSR), + bool(converted_value & stat.S_IWUSR), + )) + except ValueError: + raise TraitError('invalid --sock-mode value: %s' % value) + except AssertionError: + raise TraitError( + 'invalid --sock-mode value: %s, must have u+rw (0600) at a minimum' % value + ) + return converted_value + port_retries = Integer(50, config=True, help=_("The number of additional ports to try if the specified port is not available.") ) @@ -1616,7 +1635,7 @@ def _bind_http_server(self): def _bind_http_server_unix(self): try: - sock = bind_unix_socket(self.sock, mode=int(self.sock_mode.encode(), 8)) + sock = bind_unix_socket(self.sock, mode=self.sock_mode) self.http_server.add_socket(sock) except socket.error as e: if e.errno == errno.EADDRINUSE: diff --git a/notebook/tests/test_notebookapp_integration.py b/notebook/tests/test_notebookapp_integration.py index 632dd3c751..3a5b6b2bff 100644 --- a/notebook/tests/test_notebookapp_integration.py +++ b/notebook/tests/test_notebookapp_integration.py @@ -17,11 +17,13 @@ def test_shutdown_sock_server_integration(): encoded_sock_path = urlencode_unix_socket_path(sock) p = subprocess.Popen( - ['jupyter-notebook', '--sock=%s' % sock], + ['jupyter-notebook', '--sock=%s' % sock, '--sock-mode=0700'], stdout=subprocess.PIPE, stderr=subprocess.PIPE ) + complete = False for line in iter(p.stderr.readline, b''): + print(line) if url in line: complete = True break @@ -30,8 +32,8 @@ def test_shutdown_sock_server_integration(): assert encoded_sock_path.encode() in subprocess.check_output(['jupyter-notebook', 'list']) - # Ensure default umask is properly applied. - assert stat.S_IMODE(os.lstat(sock).st_mode) == 0o600 + # Ensure umask is properly applied. + assert stat.S_IMODE(os.lstat(sock).st_mode) == 0o700 try: subprocess.check_output(['jupyter-notebook', 'stop'], stderr=subprocess.STDOUT) @@ -49,6 +51,29 @@ def test_shutdown_sock_server_integration(): p.wait() +def test_sock_server_validate_sockmode_type(): + try: + subprocess.check_output( + ['jupyter-notebook', '--sock=/tmp/nonexistent', '--sock-mode=badbadbad'], + stderr=subprocess.STDOUT + ) + except subprocess.CalledProcessError as e: + assert 'badbadbad' in e.output.decode() + else: + raise AssertionError('expected execution to fail due to validation of --sock-mode param') + + +def test_sock_server_validate_sockmode_accessible(): + try: + subprocess.check_output( + ['jupyter-notebook', '--sock=/tmp/nonexistent', '--sock-mode=0444'], + stderr=subprocess.STDOUT + ) + except subprocess.CalledProcessError as e: + assert '0444' in e.output.decode() + else: + raise AssertionError('expected execution to fail due to validation of --sock-mode param') + def _ensure_stopped(check_msg='There are no running servers'): try: From 02cd1fa8a00a97f531845ad7533aaf25130a8772 Mon Sep 17 00:00:00 2001 From: Kris Wilson Date: Wed, 13 May 2020 00:34:23 -0700 Subject: [PATCH 16/18] Traitlets type handling fixup. --- notebook/notebookapp.py | 14 +++++++++----- notebook/tests/test_notebookapp_integration.py | 2 +- 2 files changed, 10 insertions(+), 6 deletions(-) diff --git a/notebook/notebookapp.py b/notebook/notebookapp.py index 87ba0ee9f0..62f0e76581 100755 --- a/notebook/notebookapp.py +++ b/notebook/notebookapp.py @@ -787,7 +787,7 @@ def _validate_ip(self, proposal): help=_("The UNIX socket the notebook server will listen on.") ) - sock_mode = Integer(int('0600', 8), config=True, + sock_mode = Unicode('0600', config=True, help=_("The permissions mode for UNIX socket creation (default: 0600).") ) @@ -796,18 +796,22 @@ def _validate_sock_mode(self, proposal): value = proposal['value'] try: converted_value = int(value.encode(), 8) - # Ensure the mode is at least user readable/writable. assert all(( + # Ensure the mode is at least user readable/writable. bool(converted_value & stat.S_IRUSR), bool(converted_value & stat.S_IWUSR), + # And isn't out of bounds. + converted_value <= 2 ** 12 )) except ValueError: - raise TraitError('invalid --sock-mode value: %s' % value) + raise TraitError( + 'invalid --sock-mode value: %s, please specify as e.g. "0600"' % value + ) except AssertionError: raise TraitError( 'invalid --sock-mode value: %s, must have u+rw (0600) at a minimum' % value ) - return converted_value + return value port_retries = Integer(50, config=True, help=_("The number of additional ports to try if the specified port is not available.") @@ -1635,7 +1639,7 @@ def _bind_http_server(self): def _bind_http_server_unix(self): try: - sock = bind_unix_socket(self.sock, mode=self.sock_mode) + sock = bind_unix_socket(self.sock, mode=int(self.sock_mode.encode(), 8)) self.http_server.add_socket(sock) except socket.error as e: if e.errno == errno.EADDRINUSE: diff --git a/notebook/tests/test_notebookapp_integration.py b/notebook/tests/test_notebookapp_integration.py index 3a5b6b2bff..1ae7e35367 100644 --- a/notebook/tests/test_notebookapp_integration.py +++ b/notebook/tests/test_notebookapp_integration.py @@ -23,7 +23,7 @@ def test_shutdown_sock_server_integration(): complete = False for line in iter(p.stderr.readline, b''): - print(line) + print(line.decode()) if url in line: complete = True break From 5aa6e0dfd5af4abd04fa8c7ae79db6c3e0919701 Mon Sep 17 00:00:00 2001 From: Kris Wilson Date: Wed, 13 May 2020 01:53:28 -0700 Subject: [PATCH 17/18] Feedback: add socket usage check on bind and test. --- notebook/notebookapp.py | 7 ++++- .../tests/test_notebookapp_integration.py | 29 +++++++++++++++++++ notebook/utils.py | 18 ++++++++++++ 3 files changed, 53 insertions(+), 1 deletion(-) diff --git a/notebook/notebookapp.py b/notebook/notebookapp.py index 62f0e76581..5ba9605ecd 100755 --- a/notebook/notebookapp.py +++ b/notebook/notebookapp.py @@ -115,6 +115,7 @@ check_pid, pathname2url, run_sync, + unix_socket_in_use, url_escape, url_path_join, urldecode_unix_socket_path, @@ -1638,12 +1639,16 @@ def _bind_http_server(self): return self._bind_http_server_unix() if self.sock else self._bind_http_server_tcp() def _bind_http_server_unix(self): + if unix_socket_in_use(self.sock): + self.log.warning(_('The socket %s is already in use.') % self.sock) + return False + try: sock = bind_unix_socket(self.sock, mode=int(self.sock_mode.encode(), 8)) self.http_server.add_socket(sock) except socket.error as e: if e.errno == errno.EADDRINUSE: - self.log.info(_('The socket %s is already in use.') % self.sock) + self.log.warning(_('The socket %s is already in use.') % self.sock) return False elif e.errno in (errno.EACCES, getattr(errno, 'WSAEACCES', errno.EACCES)): self.log.warning(_("Permission to listen on sock %s denied") % self.sock) diff --git a/notebook/tests/test_notebookapp_integration.py b/notebook/tests/test_notebookapp_integration.py index 1ae7e35367..9af505342b 100644 --- a/notebook/tests/test_notebookapp_integration.py +++ b/notebook/tests/test_notebookapp_integration.py @@ -135,3 +135,32 @@ def test_stop_multi_integration(): p1.wait() p2.wait() p3.wait() + + +@skip_win32 +def test_launch_socket_collision(): + """Tests UNIX socket in-use detection for lifecycle correctness.""" + sock = UNIXSocketNotebookTestBase.sock + check_msg = 'socket %s is already in use' % sock + + _ensure_stopped() + + # Start a server. + cmd = ['jupyter-notebook', '--sock=%s' % sock] + p1 = subprocess.Popen(cmd) + time.sleep(3) + + # Try to start a server bound to the same UNIX socket. + try: + subprocess.check_output(cmd, stderr=subprocess.STDOUT) + except subprocess.CalledProcessError as e: + assert check_msg in e.output.decode() + else: + raise AssertionError('expected error, instead got %s' % e.output.decode()) + + # Stop the background server, ensure it's stopped and wait on the process to exit. + subprocess.check_call(['jupyter-notebook', 'stop', sock]) + + _ensure_stopped() + + p1.wait() diff --git a/notebook/utils.py b/notebook/utils.py index 47d7a26faf..f0ad0e26c6 100644 --- a/notebook/utils.py +++ b/notebook/utils.py @@ -11,6 +11,7 @@ import errno import inspect import os +import socket import stat import sys from distutils.version import LooseVersion @@ -22,6 +23,7 @@ # in tornado >=5 with Python 3 from tornado.concurrent import Future as TornadoFuture from tornado import gen +import requests_unixsocket from ipython_genutils import py3compat # UF_HIDDEN is a stat flag not defined in the stat module. @@ -382,3 +384,19 @@ def urldecode_unix_socket_path(socket_path): def urlencode_unix_socket(socket_path): """Encodes a UNIX socket URL from a socket path for the `http+unix` URI form.""" return 'http+unix://%s' % urlencode_unix_socket_path(socket_path) + + +def unix_socket_in_use(socket_path): + """Checks whether a UNIX socket path on disk is in use by attempting to connect to it.""" + if not os.path.exists(socket_path): + return False + + try: + sock = socket.socket(socket.AF_UNIX, socket.SOCK_STREAM) + sock.connect(socket_path) + except socket.error: + return False + else: + return True + finally: + sock.close() From dcc8874daa780a6cc39c5dd1584b2c5735e96a66 Mon Sep 17 00:00:00 2001 From: Kris Wilson Date: Wed, 13 May 2020 02:07:00 -0700 Subject: [PATCH 18/18] Feedback: remove socket on stop. --- notebook/notebookapp.py | 9 +++++++++ 1 file changed, 9 insertions(+) diff --git a/notebook/notebookapp.py b/notebook/notebookapp.py index 5ba9605ecd..5433f2b2bc 100755 --- a/notebook/notebookapp.py +++ b/notebook/notebookapp.py @@ -517,6 +517,13 @@ def _shutdown_or_exit(self, target_endpoint, server): if not self.shutdown_server(server): sys.exit("Could not stop server on %s" % target_endpoint) + @staticmethod + def _maybe_remove_unix_socket(socket_path): + try: + os.unlink(socket_path) + except (OSError, IOError): + pass + def start(self): servers = list(list_running_servers(self.runtime_dir)) if not servers: @@ -527,6 +534,8 @@ def start(self): sock = server.get('sock', None) if sock and sock == self.sock: self._shutdown_or_exit(sock, server) + # Attempt to remove the UNIX socket after stopping. + self._maybe_remove_unix_socket(sock) return elif self.port: port = server.get('port', None)