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

procutil: use setsid instead of setpgid #6390

Merged
merged 1 commit into from
Jun 3, 2024
Merged

procutil: use setsid instead of setpgid #6390

merged 1 commit into from
Jun 3, 2024

Conversation

nicks
Copy link
Member

@nicks nicks commented May 31, 2024

this starts to stretch my knowledge
of low-level unix process apis, but
my current understanding is:

  • every process has a process group id and a session id
  • all processes in a group belong to the same session
  • setpgid creates a new process group id in the same session
  • setsid creates a new sessions and new process group
  • process groups are used to send signals to sets of processes
  • sessions are used to attach a process to a controlling terminal

using a new session here ensures
that the subprocess can't run something
that closes the terminal controlling tilt,
or put tilt in the background.

fixes #6378
fixes #6387

Signed-off-by: Nick Santos nick.santos@docker.com

this starts to stretch my knowledge
of low-level unix process apis, but
my current understanding is:
- every process has a process group id and a session id
- all processes in a group belong to the same session
- setpgid creates a new process group id in the same session
- setsid creates a new sessions and new process group
- process groups are used to send signals to sets of processes
- sessions are used to attach a process to a controlling terminal

using a new session here ensures
that the subprocess can't run something
that closes the terminal controlling tilt,
or put tilt in the background.

fixes #6378
fixes #6387

Signed-off-by: Nick Santos <nick.santos@docker.com>
@nicks nicks requested a review from lizzthabet May 31, 2024 22:11
Copy link
Contributor

@lizzthabet lizzthabet left a comment

Choose a reason for hiding this comment

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

thanks for the detailed explanation! this change worked for me on macos with a sample project that previously closed tilt when it booted up.

@nicks nicks merged commit c38f0fd into master Jun 3, 2024
9 checks passed
@nicks nicks deleted the nicks/sid branch June 3, 2024 16:14
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

tty in node in local_resource kills tilt tty in local_resource kills tilt
2 participants