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

std.child_process: enable using non-standard pipes #11701

Closed
wants to merge 8 commits into from

Conversation

matu3ba
Copy link
Contributor

@matu3ba matu3ba commented May 23, 2022

The user provides an array of struct with info for direction for the pipe.
Each struct also contains the space for the input and output end of pipe,
which simplifies handling.
Typically a low number of non-standard pipes are used, so no premature optimization
and complicating the usage.
Also, it is expected that the upper bound of used pipes per process is comptime-known,
so slicing a continuous chunk would be most efficient.

  • provide user with function pointer to enable parsing of necessary file descriptor
    on Linux and file handles on Windows in the child process
    • They can not be comptime-set, because the Kernel or user expects the program to
      work in all cases.
  • keep logic the same as for posix_spawn
  • use fcntl and SetHandleInformation to prevent file leakage into
    subsequently created children
  • test if both fcntl and HandleInformation were correctly set
  • test non-standard stream with 1 pipe type

Note: Standard streams are expected to be inheritable by child processes, so those
streams are "intentionally leaked to the child".

Benchmarking with realistic workloads is needed to decide the final design.
Especially, since storing space for one additionally unused pipe end can be wasteful.

@matu3ba matu3ba force-pushed the childproc branch 2 times, most recently from fa27d15 to 79a7afb Compare May 24, 2022 11:34
@matu3ba

This comment was marked as resolved.

@matu3ba
Copy link
Contributor Author

matu3ba commented May 24, 2022

Solution is to add those https://comp.os.ms-windows.programmer.win32.narkive.com/a4d3CdvQ/convert-handle-to-file-descriptor-works-in-vc-not-in-cygwin and while we are it these very nice functions to get the type of an underlying file descriptor:
https://jakash3.wordpress.com/2012/12/19/getting-file-descriptor-type-in-windows/

Andrew: "Libc is to be entirely avoided on Windows"

Thus: Use @intToPtr and @ptrToInt for Windows.

Reminder for me to implement eventually
https://www.youtube.com/watch?v=uhRWMGBjlO8&t=621s
haskell/directory#110

@matu3ba

This comment was marked as resolved.

The user provides an array of struct with info for direction for the pipe.
Each struct also contains the space for the input and output end of pipe,
which simplifies handling.
Typically a low number of non-standard pipes are used, so no premature optimization
and complicating the usage.
Also, it is expected that the upper bound of used pipes per process is comptime-known,
so slicing a continuous chunk would be most efficient.

* provide user with function pointer to enable parsing of necessary file descriptor
  on Linux and file handles on Windows in the child process
  - They can not be comptime-set, because the Kernel or user expects the program to
    work in all cases.
* keep logic the same as for posix_spawn
* use fcntl and SetHandleInformation to prevent file leakage into
  subsequently created children
* test if both fcntl and HandleInformation were correctly set
* test non-standard stream with 1 pipe type

Note: Standard streams are expected to be inheritable by child processes, so those
streams are "intentionally leaked to the child".

Benchmarking with realistic workloads is needed to decide the final design.
Especially, since storing space for one additionally unused pipe end can be wasteful.
@matu3ba matu3ba changed the title Childproc std.child_process: enable using non-standard pipes May 27, 2022
@matu3ba
Copy link
Contributor Author

matu3ba commented May 27, 2022

I would like to have some feedback from @marler8997 or another person that did more stuff with IPC on better benchmarks or to have some insight in use cases for the fast path aka posix_spawn.

So far I have this https://gist.github.com/matu3ba/c643b26b5e18a2aff20ef75468aa088d with a minimal printing hello world as child process and no user-defined function what to do in the parent right before spawn().

On Arch Linux I get round about these values, if repeated sufficiently often ./time parent:

real    0m0,263s
user    0m0,483s
sys     0m0,332s

With the changes applies (null inserted into spawn function):

real    0m0,255s
user    0m0,471s
sys     0m0,327s

@matu3ba matu3ba marked this pull request as ready for review May 27, 2022 15:20
@@ -127,25 +163,33 @@ pub const ChildProcess = struct {
self.gid = user_info.gid;
}

const ExPipeInfoProto = switch (builtin.zig_backend) {
Copy link
Contributor

Choose a reason for hiding this comment

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

I don't think this is a great type name

Copy link
Contributor Author

Choose a reason for hiding this comment

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

What about ExtraPipeProto? How would you call writing the pipe information to the child process?

self.stderr = File{ .handle = stderr_pipe[0] };
} else {
self.stderr = null;
// user must communicate extra pipes to child process either via
Copy link
Contributor

Choose a reason for hiding this comment

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

Or just convention. e.g. SD_LISTEN_FDS_START is compile-time known to be 3

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I may understand it wrong, what you are suggesting. As I understand it, you want to hardcode the file descriptor numbers.

Doing this for file descriptors other than 0,1,2 is error prone, even if we can assume that 3 is the next free one for the current process.
Take as example that the user has as additional requirement to open a file before using 3 or does other stuff, which uses file descriptors. Now 3 might be taken and the user introduced an error.

Another example, which is more of the worst case to debug: Another process uses file descriptor 3 already to do IPC with the one that now wants to spawn a child process and uses file descriptor 3 to do IPC.

@andrewrk
Copy link
Member

andrewrk commented Sep 1, 2022

I have attempted to rebase this against master branch on your behalf in order to re-run the failing CI checks. However there are conflicts. If you would like to continue working on this, please open a new PR against master branch.

@andrewrk andrewrk closed this Sep 1, 2022
matu3ba added a commit to matu3ba/zig that referenced this pull request Sep 5, 2022
The user provides an array of struct with info for direction for the pipe.
Each struct also contains the space for the input and output end of pipe,
which simplifies handling.
Typically a low number of non-standard pipes are used, so no premature optimization
and complicating the usage.
Also, it is expected that the upper bound of used pipes per process is comptime-known,
so slicing a continuous chunk would be most efficient.

* provide user with function pointer to enable parsing of necessary file descriptor
  on Linux and file handles on Windows in the child process
  - They can not be comptime-set, because the Kernel or user expects the program to
    work in all cases.
* keep logic the same as for posix_spawn
* use fcntl and SetHandleInformation to prevent file leakage into
  subsequently created children
* test if both fcntl and HandleInformation were correctly set
* test non-standard stream with 1 pipe type

Note: Standard streams are expected to be inheritable by child processes, so those
streams are "intentionally leaked to the child".

Benchmarking with realistic workloads is needed to decide the final design.
Especially, since storing space for one additionally unused pipe end can be wasteful.

This is ziglang#11701 with conflicts fixed and will be used for the panic test runner.
matu3ba added a commit to matu3ba/zig that referenced this pull request Sep 18, 2022
The user provides an array of struct with info for direction for the pipe.
Each struct also contains the space for the input and output end of pipe,
which simplifies handling.
Typically a low number of non-standard pipes are used, so no premature optimization
and complicating the usage.
Also, it is expected that the upper bound of used pipes per process is comptime-known,
so slicing a continuous chunk would be most efficient.

* provide user with function pointer to enable parsing of necessary file descriptor
  on Linux and file handles on Windows in the child process
  - They can not be comptime-set, because the Kernel or user expects the program to
    work in all cases.
* keep logic the same as for posix_spawn
* use fcntl and SetHandleInformation to prevent file leakage into
  subsequently created children
* test if both fcntl and HandleInformation were correctly set
* test non-standard stream with 1 pipe type

Note: Standard streams are expected to be inheritable by child processes, so those
streams are "intentionally leaked to the child".

Benchmarking with realistic workloads is needed to decide the final design.
Especially, since storing space for one additionally unused pipe end can be wasteful.

This is ziglang#11701 with conflicts fixed and will be used for the panic test runner.
matu3ba added a commit to matu3ba/zig that referenced this pull request Sep 18, 2022
The user provides an array of struct with info for direction for the pipe.
Each struct also contains the space for the input and output end of pipe,
which simplifies handling.
Typically a low number of non-standard pipes are used, so no premature optimization
and complicating the usage.
Also, it is expected that the upper bound of used pipes per process is comptime-known,
so slicing a continuous chunk would be most efficient.

* provide user with function pointer to enable parsing of necessary file descriptor
  on Linux and file handles on Windows in the child process
  - They can not be comptime-set, because the Kernel or user expects the program to
    work in all cases.
* keep logic the same as for posix_spawn
* use fcntl and SetHandleInformation to prevent file leakage into
  subsequently created children
* test if both fcntl and HandleInformation were correctly set
* test non-standard stream with 1 pipe type

Note: Standard streams are expected to be inheritable by child processes, so those
streams are "intentionally leaked to the child".

Benchmarking with realistic workloads is needed to decide the final design.
Especially, since storing space for one additionally unused pipe end can be wasteful.

This is ziglang#11701 with conflicts fixed and will be used for the panic test runner.
matu3ba added a commit to matu3ba/zig that referenced this pull request Nov 27, 2022
The user provides an array of struct with info for direction for the pipe.
Each struct also contains the space for the input and output end of pipe,
which simplifies handling.
Typically a low number of non-standard pipes are used, so no premature optimization
and complicating the usage.
Also, it is expected that the upper bound of used pipes per process is comptime-known,
so slicing a continuous chunk would be most efficient.

* provide user with function pointer to enable parsing of necessary file descriptor
  on Linux and file handles on Windows in the child process
  - They can not be comptime-set, because the Kernel or user expects the program to
    work in all cases.
* keep logic the same as for posix_spawn
* use fcntl and SetHandleInformation to prevent file leakage into
  subsequently created children
* test if both fcntl and HandleInformation were correctly set
* test non-standard stream with 1 pipe type

Note: Standard streams are expected to be inheritable by child processes, so those
streams are "intentionally leaked to the child".

Benchmarking with realistic workloads is needed to decide the final design.
Especially, since storing space for one additionally unused pipe end can be wasteful.

This is ziglang#11701 with conflicts fixed and will be used for the panic test runner.
matu3ba added a commit to matu3ba/zig that referenced this pull request Dec 4, 2022
The user provides an array of struct with info for direction for the pipe.
Each struct also contains the space for the input and output end of pipe,
which simplifies handling.
Typically a low number of non-standard pipes are used, so no premature optimization
and complicating the usage.
Also, it is expected that the upper bound of used pipes per process is comptime-known,
so slicing a continuous chunk would be most efficient.

* provide user with function pointer to enable parsing of necessary file descriptor
  on Linux and file handles on Windows in the child process
  - They can not be comptime-set, because the Kernel or user expects the program to
    work in all cases.
* keep logic the same as for posix_spawn
* use fcntl and SetHandleInformation to prevent file leakage into
  subsequently created children
* test if both fcntl and HandleInformation were correctly set
* test non-standard stream with 1 pipe type

Note: Standard streams are expected to be inheritable by child processes, so those
streams are "intentionally leaked to the child".

Benchmarking with realistic workloads is needed to decide the final design.
Especially, since storing space for one additionally unused pipe end can be wasteful.

This is ziglang#11701 with conflicts fixed and will be used for the panic test runner.
matu3ba added a commit to matu3ba/zig that referenced this pull request Dec 11, 2022
The user provides an array of struct with info for direction for the pipe.
Each struct also contains the space for the input and output end of pipe,
which simplifies handling.
Typically a low number of non-standard pipes are used, so no premature optimization
and complicating the usage.
Also, it is expected that the upper bound of used pipes per process is comptime-known,
so slicing a continuous chunk would be most efficient.

* provide user with function pointer to enable parsing of necessary file descriptor
  on Linux and file handles on Windows in the child process
  - They can not be comptime-set, because the Kernel or user expects the program to
    work in all cases.
* keep logic the same as for posix_spawn
* use fcntl and SetHandleInformation to prevent file leakage into
  subsequently created children
* test if both fcntl and HandleInformation were correctly set
* test non-standard stream with 1 pipe type

Note: Standard streams are expected to be inheritable by child processes, so those
streams are "intentionally leaked to the child".

Benchmarking with realistic workloads is needed to decide the final design.
Especially, since storing space for one additionally unused pipe end can be wasteful.

This is ziglang#11701 with conflicts fixed and will be used for the panic test runner.
matu3ba added a commit to matu3ba/zig that referenced this pull request Dec 22, 2022
The user provides an array of struct with info for direction for the pipe.
Each struct also contains the space for the input and output end of pipe,
which simplifies handling.
Typically a low number of non-standard pipes are used, so no premature optimization
and complicating the usage.
Also, it is expected that the upper bound of used pipes per process is comptime-known,
so slicing a continuous chunk would be most efficient.

* provide user with function pointer to enable parsing of necessary file descriptor
  on Linux and file handles on Windows in the child process
  - They can not be comptime-set, because the Kernel or user expects the program to
    work in all cases.
* keep logic the same as for posix_spawn
* use fcntl and SetHandleInformation to prevent file leakage into
  subsequently created children
* test if both fcntl and HandleInformation were correctly set
* test non-standard stream with 1 pipe type

Note: Standard streams are expected to be inheritable by child processes, so those
streams are "intentionally leaked to the child".

Benchmarking with realistic workloads is needed to decide the final design.
Especially, since storing space for one additionally unused pipe end can be wasteful.

This is ziglang#11701 with conflicts fixed and will be used for the panic test runner.
matu3ba added a commit to matu3ba/zig that referenced this pull request Dec 27, 2022
The user provides an array of struct with info for direction for the pipe.
Each struct also contains the space for the input and output end of pipe,
which simplifies handling.
Typically a low number of non-standard pipes are used, so no premature optimization
and complicating the usage.
Also, it is expected that the upper bound of used pipes per process is comptime-known,
so slicing a continuous chunk would be most efficient.

* provide user with function pointer to enable parsing of necessary file descriptor
  on Linux and file handles on Windows in the child process
  - They can not be comptime-set, because the Kernel or user expects the program to
    work in all cases.
* keep logic the same as for posix_spawn
* use fcntl and SetHandleInformation to prevent file leakage into
  subsequently created children
* test if both fcntl and HandleInformation were correctly set
* test non-standard stream with 1 pipe type

Note: Standard streams are expected to be inheritable by child processes, so those
streams are "intentionally leaked to the child".

Benchmarking with realistic workloads is needed to decide the final design.
Especially, since storing space for one additionally unused pipe end can be wasteful.

This is ziglang#11701 with conflicts fixed and will be used for the panic test runner.
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.

3 participants