Repository navigation
[FYI] Child processes inherit open file handles from parent #51
Description
Activity
Personally I haven't had any issues with it, but would love more exact details on situations where this causes issues so we can add that to the README note
This can be avoided at the C level using the
FD_CLOEXECflag (this came up at phpnw in relation to an old xdebug bug report).With file handles there's no way to set this from userland that I'm aware of, but with sockets it can done with
socket_import_stream()/socket_set_option()- it may require passing the option number directly, I don't know if the relevant constant is defined, I haven't checked.I could look at exposing this via a stream context parameter if there's any interest in it? Likely would not be available until 7.3 though.
@DaveRandom For PHP that's probably good enough, because it's single threaded, but it requires the
socketextension then. For files there's the'e'flag, but probably nobody uses that.@kelunik I feel like
$ctx['fd']['inheritable'] = falseis the way to go with this, personally (which affects fds so will include sockets)Sounds good to me. Maybe we can even backport that to 7.1 / 7.0. Honestly, I don't see many reasons why it shouldn't be the default, because PHP doesn't expose the raw FDs anywhere to let a child use them.
@kelunik I would also be OK with that but it would probably need to be done in 8 because semver. For the time being it can default to current (arguably but not necessarily) broken behaviour.
The nice thing about context options is that setting non-existent ones doesn't break anything, so at least existing code can be modified to set it without worrying about whether it will actually have any effect (i.e. no need for a version check, react/amp can just set it by default).
It is possible to get at raw fds in PHP btw, http://php.net/manual/en/wrappers.php.php#wrappers.php.fd
That's not the only consideration though - the child is not necessarily a PHP process, so it's theoretically possible to do interesting/useful stuff with inheritable fds across exec boundaries.
Yes, it's possible to access the FD, I know, but if you open a stream, you can't get the FD number to let the child process use that (inherited) FD.
@kelunik I will look at exposing that via
stream_get_meta_data()while I'm at it. May as well make this into a potentially useful (albeit niche) feature.@DaveRandom Thanks for chiming in, I agree that adding a context option to set this mode on creation is the way forward here 👍
Whether this should be the future default is debatable (I agree), taking a look at how others handle this (such as https://www.python.org/dev/peps/pep-0446/ and https://bugs.ruby-lang.org/issues/5041) suggests that they figured it's worth introducing this as a fix even in a minor version. Either way, this would probably be best discussed with php-internals instead of this issue, so I'm all for pushing this upstream 👍
Exposing access to FDs is only partly related to this issue, but I agree that exposing it via
stream_get_meta_data()looks sane to me 👍Arguably, this is not really an issue in this repository, but more like how underlying Unix system work. Given the limited number of options currently, I would still suggest documenting this as a known issue in the README of this repository. We should still look into providing better defaults in a future version 👍
Just confirmed this has been solved with #65 by handling it the following way: WyriHaximus/reactphp-child-process-messenger@9995a03#diff-8157a0d28a58887c0293ce14b4b0281fR68
Closing this issue as I believe this has been solved and will be released soon™.
Reacted by Oleksandr
Opening this as a FYI only, note that child processes inherit open file handles from the parent process.
https://bugs.php.net/bug.php?id=70932
Depending on how child processes are used this may or may not cause issues. I'm currently not aware of any issues in the ReactPHP ecosystem, so I'm merely opening this as a reminder and to gather some input on this.
One option would be adding a heads up to the README and consider this issue resolved for now. Thanks to @kelunik for making us aware of this issue!