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

Reliable list_running_servers(runtime_dir) design #5716

Open
wants to merge 18 commits into
base: 6.4.x
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from 1 commit
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
66 changes: 45 additions & 21 deletions notebook/notebookapp.py
Expand Up @@ -429,20 +429,10 @@ def start(self):
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.

*server_info* should be a dictionary as produced by list_running_servers().

Will first try to request shutdown using /api/shutdown .
On Unix, if the server is still running after *timeout* seconds, it will
send SIGTERM. After another timeout, it escalates to SIGKILL.

Returns True if the server was stopped by any means, False if stopping it
failed (on Windows).
"""
def kernel_request(server_info, path='/login', method='GET', body=None, headers=None, timeout=5, log=None):
Copy link
Member

Choose a reason for hiding this comment

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

This method should be renamed since its unrelated to kernel. Perhaps server_request?

Also, please remove the default value from path since that should be explicitly provided by the caller (which is already the case anyway).

Copy link
Author

@abaelhe abaelhe Sep 1, 2020

Choose a reason for hiding this comment

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

This method should be renamed since its unrelated to kernel. Perhaps server_request?

we creator have this naming right;
with respect to Guido name Python; and their first creator name IPython and Jupyter also Anaconda;
i am working on my IPython Swift Kernel, kernel in my brain;
don't mind, different think lead sparks.

Also, please remove the default value from path since that should be explicitly provided by the caller (which is already the case anyway).

caller will provide their; this default value does not conflict to different path;
we use '/login' as 'default value' to remind caller authentication first.

Copy link
Member

Choose a reason for hiding this comment

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

This method needs to change and the default for path either removed or updated to /api

Copy link
Member

Choose a reason for hiding this comment

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

Yeah, I agree with @kevin-bates here. This method needs to be renamed to something like api_request before we can accept it. It's not requesting a response from a "kernel" (as we define it in the Jupyter ecosystem). It's just a general server API ping.

I also agree, the default path should probably be /api.

Copy link
Author

Choose a reason for hiding this comment

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

who create new who own naming rights.
answered.

Copy link
Member

Choose a reason for hiding this comment

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

The name matters here. It obscures this library's API if function names are not clear.

kernel_request is the wrong name for this function. In the Jupyter ecosystem, kernel refers specifically to the underlying language kernel being executed by calls to the server's kernel API. This method is not making a kernel request. It's making a generic request the server's REST API. Let's generalize this function name to something like server_request or api_request.

particularly because Jupyter has a notion of kernels that is specific.

Copy link
Member

Choose a reason for hiding this comment

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

I'm setting this conversation to unresolved, since this change is necessary.

Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
def kernel_request(server_info, path='/login', method='GET', body=None, headers=None, timeout=5, log=None):
def api_request(server_info, path='/api', method='GET', body=None, headers=None, timeout=5, log=None):

Zsailer marked this conversation as resolved.
Show resolved Hide resolved
"""query a notebook server in a separate process."""
abaelhe marked this conversation as resolved.
Show resolved Hide resolved
from tornado import gen
from tornado.httpclient import AsyncHTTPClient, HTTPClient, HTTPRequest
from tornado.httpclient import AsyncHTTPClient, HTTPClient, HTTPRequest, HTTPError
from tornado.netutil import Resolver
url = server_info['url']
pid = server_info['pid']
abaelhe marked this conversation as resolved.
Show resolved Hide resolved
Expand All @@ -468,12 +458,45 @@ def resolve(self, host, port, *args, **kwargs):

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)
AsyncHTTPClient.configure(None, resolver=resolver)
HTTPClient(AsyncHTTPClient).fetch(req)
fullurl = urljoin(url, path)
headers = dict(headers) if headers is not None else {}
headers.setdefault('Authorization', 'token ' + server_info['token'])
req = HTTPRequest(fullurl,
method=method, body=body, headers=headers,
follow_redirects=True,
decompress_response=True,
allow_nonstandard_methods=False,
validate_cert=False
)
if log: log.debug("%s request to %s", method, fullurl)

savedAsyncHTTPClient = AsyncHTTPClient._save_configuration()
try:
AsyncHTTPClient.configure(None, resolver=resolver)
response = HTTPClient(AsyncHTTPClient).fetch(req)
except HTTPError as e:
if e.response is not None:
response = e.response
else:
raise
abaelhe marked this conversation as resolved.
Show resolved Hide resolved
finally:
AsyncHTTPClient._restore_configuration(savedAsyncHTTPClient)

return response


def shutdown_server(server_info, timeout=5, log=None):
"""Shutdown a notebook server in a separate process.
*server_info* should be a dictionary as produced by list_running_servers().
Will first try to request shutdown using /api/shutdown .
On Unix, if the server is still running after *timeout* seconds, it will
send SIGTERM. After another timeout, it escalates to SIGKILL.
Returns True if the server was stopped by any means, False if stopping it
failed (on Windows).
"""
url = server_info['url']
abaelhe marked this conversation as resolved.
Show resolved Hide resolved
pid = server_info['pid']
kernel_request(server_info, path='api/shutdown', method='POST', body=b'', timeout=timeout, log=log)

# Poll to see if it shut down.
for _ in range(timeout*10):
abaelhe marked this conversation as resolved.
Show resolved Hide resolved
Expand Down Expand Up @@ -2294,9 +2317,10 @@ def list_running_servers(runtime_dir=None):
with io.open(os.path.join(runtime_dir, file_name), encoding='utf-8') as f:
info = json.load(f)

# Simple check whether that process is really still running
# active check whether that process is really available via real HTTP request
abaelhe marked this conversation as resolved.
Show resolved Hide resolved
# Also remove leftover files from IPython 2.x without a pid field
if ('pid' in info) and check_pid(info['pid']):
response = kernel_request(info, path='/login')
if response.body.find(b'<title>Jupyter Notebook</title>') > 0:
abaelhe marked this conversation as resolved.
Show resolved Hide resolved
yield info
else:
# If the process has died, try to delete its info file
Expand Down
41 changes: 30 additions & 11 deletions notebook/tests/test_notebookapp.py
Expand Up @@ -30,21 +30,40 @@ def test_help_output():
check_help_all_output('notebook')

def test_server_info_file():
import threading, tornado.ioloop as iom, tornado.platform.asyncio as torio
td = TemporaryDirectory()

if iom.asyncio is not None:
iom.asyncio.set_event_loop_policy(torio.AnyThreadEventLoopPolicy())
iom.IOLoop.configure("tornado.platform.asyncio.AsyncIOLoop")

nbapp = NotebookApp(runtime_dir=td.name, log=logging.getLogger())
def get_servers():
return list(notebookapp.list_running_servers(nbapp.runtime_dir))
nbapp.initialize(argv=[])
nbapp.io_loop = iom.IOLoop.current()
nbapp.open_browser = False
super(NotebookApp, nbapp).start()
nbapp.write_server_info_file()
servers = get_servers()
nt.assert_equal(len(servers), 1)
nt.assert_equal(servers[0]['port'], nbapp.port)
nt.assert_equal(servers[0]['url'], nbapp.connection_url)
nbapp.remove_server_info_file()
nt.assert_equal(get_servers(), [])

# The ENOENT error should be silenced.
nbapp.remove_server_info_file()
def check_thread():
try:
servers = list(notebookapp.list_running_servers(nbapp.runtime_dir))
nt.assert_equal(len(servers), 1)
nt.assert_equal(servers[0]['port'], nbapp.port)
nt.assert_equal(servers[0]['url'], nbapp.connection_url)
finally:
nbapp.stop()

nbapp.io_loop.add_callback(nbapp.io_loop.run_in_executor, executor=None, func=check_thread)

if sys.platform.startswith("win"):
pc = iom.PeriodicCallback(lambda: None, 5000)
pc.start()
try:
nbapp.io_loop.start()
except KeyboardInterrupt:
print("Interrupted...")
finally:
nbapp.remove_server_info_file()

def test_nb_dir():
with TemporaryDirectory() as td:
Expand Down Expand Up @@ -200,4 +219,4 @@ def test_run(self):
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}
assert self.sock in {info['sock'] for info in servers}