Skip to content

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.

# 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.

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