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

close_on_exec on Windows #14717

Open
straight-shoota opened this issue Jun 15, 2024 · 2 comments
Open

close_on_exec on Windows #14717

straight-shoota opened this issue Jun 15, 2024 · 2 comments
Labels
platform:windows Windows support based on the MSVC toolchain / Win32 API status:discussion topic:stdlib:system

Comments

@straight-shoota
Copy link
Member

Socket#close_on_exec? and FileDescriptor#close_on_exec? both have a fixed value of false on Windows. This was originally introduced in #5339 for file descriptors, and since #14634 also applies to sockets (before that patch, it didn't even compile).

The concept of close_on_exec only exists on Unix systems due to the fork/exec mechanism. This is not a thing on Windows.
We still need to somehow implement the API on Windows, hence it has a fixed value. Though the choice to make it false is unexpected to me.

The underlying concept is about whether child processes inherit a file descriptor (or socket). close_on_exec == true means the file descriptor is closed on exec and child process won't inherit. close_on_exec == false means the opposite: the file descriptor is not closed on exec and child processes inherit.

On Windows, child processes don't inherit handles from the parent process by default. Following from that I would assume close_on_exec == true should hold on Windows.
The inheritance behaviour of IO::FileDescriptor and Socket on Windows is equivalent to the default on Unix systems, which is close_on_exec == true.

We could consider renaming the property to a more abstract name like inherited_by_child_processes to move away from platform-specific terminology and make this equivalence more visible.

Note, there are ways to enable inheritance for handles in Windows as well: https://devblogs.microsoft.com/oldnewthing/20111216-00/?p=8873
We might want to look into implementing that in the future. It's a bit of a rabbit hole, though. A proper, thread-safe implementation as noted in this article needs to specifiy inheritable handles at process creation. So this would rather be an option for Process.new than a property on IO::FileDescriptor and Socket.
I figure similar issues as mentioned for inheritance on Windows apply to Unix systems as well. So perhaps it might really be a good idea to introduce a new mechanism which operates on process creation instead of a global configuration per file descriptor.

@straight-shoota straight-shoota added status:discussion topic:stdlib:system platform:windows Windows support based on the MSVC toolchain / Win32 API and removed platform:windows Windows support based on the MSVC toolchain / Win32 API labels Jun 15, 2024
@ysbaddaden
Copy link
Contributor

Yes, Windows won't pass the socket to sub-processes so #close_on_exec? should be true.

@ysbaddaden
Copy link
Contributor

ysbaddaden commented Jun 17, 2024

Oh, passing the inheritable file descriptors to Process.new sounds good: Windows works this way, while on UNIX we could set FD_CLOEXEC after fork and before exec, so we'd pass the file descriptors to the actual sub-process, without leaking to any other sub-process another thread happens to spawn in parallel, nor using any lock. Nice!

Then we could deprecate the UNIX specific #close_on_exec? and #close_on_exec= 👍

@HertzDevil HertzDevil added the platform:windows Windows support based on the MSVC toolchain / Win32 API label Jun 19, 2024
@HertzDevil HertzDevil moved this to To investigate in Windows Support Sep 4, 2024
@HertzDevil HertzDevil moved this from To investigate to Todo in Windows Support Sep 4, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
platform:windows Windows support based on the MSVC toolchain / Win32 API status:discussion topic:stdlib:system
Projects
Status: Todo
Development

No branches or pull requests

3 participants