crystal-lang / crystal

The Crystal Programming Language
https://crystal-lang.org
Apache License 2.0
19.2k stars 1.61k forks source link

`close_on_exec` on Windows #14717

Open straight-shoota opened 2 weeks ago

straight-shoota commented 2 weeks ago

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.

ysbaddaden commented 2 weeks ago

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

ysbaddaden commented 2 weeks ago

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= :+1: