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

Overwrite all open file descriptors with /dev/null streams #22

Merged
merged 23 commits into from
Jan 17, 2019

Conversation

WyriHaximus
Copy link
Owner

This also gets rid of STD* streams so we can fully support windows again

Refs reactphp/child-process#65

This also gets rid of STD* streams so we can fully support windows again
@kelunik
Copy link

kelunik commented Dec 3, 2018

Does that work fine on OS X? I think there are portability issues with /proc/self for getting all open FDs IIRC. It also isn't a solution that works on Windows.

@WyriHaximus
Copy link
Owner Author

@kelunik I'm open for suggestions for windows and OS X 👍

@NanoSector
Copy link

Using /dev/fd seems to be the equivalent for /proc/self/fd on macOS.

@WyriHaximus
Copy link
Owner Author

This doesn't work either:

<?php

foreach (new DirectoryIterator('php://fd/*') as $i => $v) {
    var_export([$i, $v]);
}

Results in:

PHP Fatal error: Uncaught UnexpectedValueException: DirectoryIterator::__construct(php://fd/*): failed to open dir: not implemented in /home/wyrihaximus/Projects/php-file-descriptors/test.php:3

@kelunik
Copy link

kelunik commented Dec 3, 2018

I don't have suggestions, otherwise we would have a solution for Amp already. :-(

@WyriHaximus
Copy link
Owner Author

@kelunik would a RFC for something like new DirectoryIterator('php://fd/*') be doable?

@kelunik
Copy link

kelunik commented Dec 4, 2018

Not sure, I think php://fd is just a mapping. The correct solution would be defaulting every FD to CLOEXEC.

@WyriHaximus
Copy link
Owner Author

Not sure, I think php://fd is just a mapping.

Hmm ok, but is or isn't there an internal list of internal file descriptors?

The correct solution would be defaulting every FD to CLOEXEC.

Would that be hard or easy to do for 7.4 or 8.0? (That wouldn't help us right now, but having that the default in the future would be awesome.)

@kelunik
Copy link

kelunik commented Dec 6, 2018

There's a list of open resources, but not sure whether there's a list of open file descriptors (including inherited ones).

As PHP is single threaded, closing all resources on proc_open after forking might be good enough.

composer.json Outdated Show resolved Hide resolved
@WyriHaximus
Copy link
Owner Author

There's a list of open resources, but not sure whether there's a list of open file descriptors (including inherited ones).

Could you look into that? Mainly interested in getting that list on the parent, so we can null them.

As PHP is single threaded, closing all resources on proc_open after forking might be good enough.

Yeah that is working out really well by doing this: https://github.com/WyriHaximus/reactphp-child-process-messenger/pull/22/files#diff-8157a0d28a58887c0293ce14b4b0281fR83

@kelunik
Copy link

kelunik commented Dec 10, 2018

Not sure whether I'll get to that, don't have that much internals knowledge. Maybe @trowski?

@WyriHaximus WyriHaximus merged commit 562fa19 into master Jan 17, 2019
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.

None yet

3 participants