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

bugfix: shutdown_server returns True when pid exists #4674

Merged
merged 1 commit into from
Jun 25, 2019

Conversation

asreimer
Copy link
Contributor

The Bug

check_pid returns True if the PID for a notebook server still exists. Therefore, the if check_pid(pid): statements here and here evaluate to True even though the notebook server is still running. This is not the expected behaviour, since this means that the shutdown_server function will return and return True even though the notebook server is still in the process of shutting down.

Reproducing the Bug

The following procedure was tested in a python3.7 virtual environment with jupyter 4.4.0

  1. in a terminal, start a notebook server: jupyter notebook --port=8888
  2. open a jupyter notebook and start an infinite loop, such as: while True: pass
  3. in another terminal run the following: python -c 'from notebook.notebookapp import shutdown_server, list_running_servers; port=8888; servers = [x for x in list_running_servers() if x["port"] == port]; status = True if len(servers) == 0 else shutdown_server(servers[0]); print(status)' && bash -c 'ps -ef | grep "jupyter-notebook"'
  4. Notice that the output of step 3) shows that:
    a) shutdown_server returns True and then,
    b) the jupyter-notebook process is still running`

In step 4, the expected behaviour would be for the output of the bash -c 'ps -ef | grep "jupyter-notebook"' command to not show a jupyter notebook server running.

Note: the while loop helps make this bug more reproducible. It's intermittent without the loop due to factors such as system load and machine specs. Also, that big command in step 3 is importing shutdown_server and then running it to kill the notebook server we started in step 1, that's all.

For step 3, here's the output I get on my system:

True
asreimer 21687 11734  8 15:56 pts/4    00:00:01 /home/asreimer/virtualenvs/env3.7/bin/python3.7 /home/asreimer/virtualenvs/env3.7/bin/jupyter-notebook --port=8888
asreimer 21794 15361  0 15:57 pts/5    00:00:00 bash -c ps -ef | grep "jupyter-notebook"
asreimer 21803 21794  0 15:57 pts/5    00:00:00 grep jupyter-notebook

Notice that the jupyter-notebook --port=8888 process is still running, despite shutdown_server telling us that the server was shutdown. Also notice that step 3 executes very fast, yet the jupyter notebook app will take several seconds to shutdown.

Note: the notebook server does eventually shutdown, [due to the api call on these] lines(https://github.com/jupyter/notebook/blob/master/notebook/notebookapp.py#L413-L420) succeeding. The bug here is that the shutdown happens after the shutdown_server function returns, instead of happening before shudown_server returns.

Who Cares?

I'm working on a project where we are running jupyter notebooks in docker containers. We need to be able to shutdown the notebooks by sending a shutdown command of some kind to the notebook server running in the docker container. Using the shutdown_server function didn't result in the expected behaviour, so I started digging in to why and found this bug.

The Fix

This commit adds a not to each line: if not check_pid(pid): so that the conditional only evaluates to True if check_pid returns False, which happens when the notebook server has shutdown, as expected.

After applying the fix, the output of steps 1 through 3 detailed above is as expected:

True
asreimer 21627 15361  0 16:08 pts/5    00:00:00 bash -c ps -ef | grep "jupyter-notebook"
asreimer 21636 21627  0 16:08 pts/5    00:00:00 grep jupyter-notebook

you will also notice that it step 3 will take as much time to execute as it takes for the jupyter notebook server to shutdown.

`check_pid` returns `True` if the PID for a notebook server still exists. Therefore, the `if check_pid(pid):` statements on lines 424 and 437 evaluate to `True` even though the notebook server is still running.

This commit simply adds a `not` to each line: `if not check_pid(pid):` so that the conditional only evaluates to `True` if `check_pid` returns `False`, which happens when the notebook server has shutdown, as expected.
@takluyver
Copy link
Member

Thanks, this looks totally sensible. Thanks for the detailed description!

@takluyver takluyver merged commit 3c9cad6 into jupyter:master Jun 25, 2019
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Mar 27, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants