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

Detect parent change in more cases on unix #1271

Merged
merged 1 commit into from
Oct 15, 2024

Conversation

bluss
Copy link
Contributor

@bluss bluss commented Oct 5, 2024

New behaviour: poll getppid and exit if this value is no longer equal to the original parent pid.

For backwards compatibility: keep old behaviour when parent_pid is 0 or not given.

Parentpoller only watches for parent change to pid 1 (init), but in modern linux environments processes are usually reparented to the systemd user session process. In this situation, ipykernel never noticies that its parent has exited.

jupyter-client already sets JPY_PARENT_PID with the pid of the parent process. We could trust this environment variable directly, but in this change, it is only used if it is equal to the actual parent id when the poller starts. (A side effect: if the parent dies quickly enough, ipykernel does not notice.)

Fixes #517

New behaviour: poll getppid and exit if this value is no longer equal to
the original parent pid.

For backwards compatibility: keep old behaviour when parent_pid is 0 or
not given.

Parentpoller only watches for parent change to pid 1 (init), but in
modern linux environments processes are usually reparented to the
systemd user session process. In this situation, ipykernel never
noticies that its parent has exited.

jupyter-client already sets JPY_PARENT_PID with the pid of the parent
process. We could trust this environment variable directly, but in this
change, it is only used if it is equal to the actual parent id when the
poller starts. (A side effect: if the parent dies quickly enough,
ipykernel does not notice.)
@bluss
Copy link
Contributor Author

bluss commented Oct 5, 2024

First PR, tell me if I did something wrong

@bluss
Copy link
Contributor Author

bluss commented Oct 11, 2024

The ci failures are similar to failures on other PRs here, so it's possible - not certain - that they are unrelated errors.

self.daemon = True

def run(self):
"""Run the poller."""
# We cannot use os.waitpid because it works only for child processes.
from errno import EINTR

# before start, check if the passed-in parent pid is valid
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Maybe this whole thing is overly complicated? We could also elect to just trust JPY_PARENT_PID directly - the only problem is if it's a stale environment variable from somewhere.

Copy link
Contributor Author

@bluss bluss Oct 16, 2024

Choose a reason for hiding this comment

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

Thanks for merging!

My updated thoughts on this comment here is that it's good that we check if JPY_PARENT_PID is the actual parent before using it. It's actually possible (is it common?) to have situations like papermill -> bash -> ipykernel or with similar intermediate processes, where JPY_PARENT_PID will point to papermill but the ipykernel ppid will be bash.

A kernel provisioner could cause such situations (I'm guilty of that..)

@Carreau
Copy link
Member

Carreau commented Oct 15, 2024

That looks reasonable to me.

@Carreau
Copy link
Member

Carreau commented Oct 15, 2024

Downstream failures seem unrelated. Merging.

@Carreau Carreau merged commit 314cc49 into ipython:main Oct 15, 2024
26 of 34 checks passed
@bluss bluss deleted the ipykernel-parent branch October 15, 2024 13:51
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

PPID is not necessarily 1 when parent frontend is killed
2 participants