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

Fix PR_SET_PDEATHSIG results in Broken pipe (#2395) #3353

Merged
merged 1 commit into from Feb 19, 2020

Conversation

@tbsmoest
Copy link
Contributor

@tbsmoest tbsmoest commented Feb 10, 2020

Fixes: #2395

The ssh client is lazily started by the first worker thread, that
requires a ssh connection. To avoid the ssh client to be killed, when
the worker process is stopped, start the ssh client in a long-lived
synchronous worker thread.

Add the class SyncWorker, that provides a single thread in which tasks
can be executed synchronously.

@tbsmoest tbsmoest requested a review from grahamc Feb 10, 2020
@edolstra
Copy link
Member

@edolstra edolstra commented Feb 10, 2020

Hm, can't we just not set PR_SET_PDEATHSIG?

@tbsmoest
Copy link
Contributor Author

@tbsmoest tbsmoest commented Feb 11, 2020

We could and it would solve the ssh issue. However, I'm not sure whether the child cleanup will work in all cases then. As the PR_SET_PDEATHSIG was explicitly introduced to guarantee the proper cleanup, I was assuming it is still needed. I will check whether it has become redundant in the meantime.

The ssh client is lazily started by the first worker thread, that
requires a ssh connection. To avoid the ssh client to be killed, when
the worker process is stopped, do not set PR_SET_PDEATHSIG.
@tbsmoest tbsmoest force-pushed the tbsmoest:priv_tobias_pr_set_deathsig-1.4 branch from f174188 to 3e34722 Feb 14, 2020
@tbsmoest
Copy link
Contributor Author

@tbsmoest tbsmoest commented Feb 14, 2020

It seems, that it is safe to not set PR_SET_PDEATHSIG for the SSH fork. The signals SIGINT and SIGTERM are forwarded. SIGKILL is not a problem either, as all children are killed, too. SIGSTOP causes issues with orphaned children when it is used in combination with SIGINT, but that is not solved by PR_SET_PDEATHSIG either.

I have updated the PR accordingly.

@tbsmoest
Copy link
Contributor Author

@tbsmoest tbsmoest commented Feb 19, 2020

@edolstra, I have removed my initial solution and now simply deactivated the PR_SET_PDEATHSIG for ssh. I this ok with you?

@edolstra edolstra merged commit c4d3674 into NixOS:master Feb 19, 2020
@edolstra
Copy link
Member

@edolstra edolstra commented Feb 19, 2020

Thanks!

@nh2
Copy link
Contributor

@nh2 nh2 commented Apr 9, 2020

@chpatrick made a backport of this fix for nix-2.3.2 which is on NixOS 20.03: chpatrick@ebaba52

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

3 participants
You can’t perform that action at this time.