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

Keep file descriptors inherited by default #65

Draft
wants to merge 2 commits into
base: main
Choose a base branch
from

Conversation

belovdv
Copy link
Contributor

@belovdv belovdv commented Jan 20, 2024

From #64:

Then jobserver is initialized from pipe, it sets cloexec, but doesn't delete environment variable which points to file descriptors. By default (unless Client::configure is used), child process will be instructed (if it uses jobserver) to access file descriptors that was closed and maybe reopened to point to other files.

This seems like a violation of IO-safety.

Either cloexec shouldn't be set or the environment variables should also be removed and only be added back via configure.


This pr is aimed to implement first option.

Concerns:

  • current behavior is how gnu make works,
  • it may be breaking change.

Reasons to choose first option:

  • in a typical case inheritance is what we want - inheritance by default will reduce amount of places in code where jobserver code is called,
  • fifo-style (named pipes) jobserver is inherited by default:
    • it's weird to have different behaviors depending on implementation details,
    • modern posix gnu make uses this fifo style by default, so it may be more common in the future,
  • second option requires changing (more) global things (cloexec, environment variable).

Concerns:

  • with inheritance by default, how to work with multiply jobservers at once, if it would be necessary?
  • with inheritance by default, Client::new in current pr's code changes global state - env var.

Notes:

  • pr is a draft, in particular, documentation wasn't updated,
  • windows implementation creates semaphore without SECURITY_ATTRIBUTES::bInheritHandle and does nothing in Client::configure. According to learn.microsoft.com, the handle cannot be inherited by child processes.

@belovdv belovdv marked this pull request as draft January 20, 2024 17: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.

1 participant