Skip to content

Add socketpair support to proc_open #5777

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

Closed
wants to merge 2 commits into from
Closed

Add socketpair support to proc_open #5777

wants to merge 2 commits into from

Conversation

kooldev
Copy link
Contributor

@kooldev kooldev commented Jun 29, 2020

Adds support for socket pairs to proc_open(). This is a big improvement over pipes on Windows systems because sockets do support non-blocking mode and can participate in stream_select(). Async PHP frameworks like ReactPHP and Amp have a hard time supporting process spawning on Windows because the created process pipes cannot be polled for events and therefore cannot be integrated with an event loop. Another common problem on Windows is that reads from process pipes will block and it is not always possible to alternate between reading data from a child processes STDOUT / STDERR as it arrives. In some cases this leads to a deadlock situation where a read blocks on one of the pipes and the child process fills the other pipes buffer without a way to drain it. Some libraries (Symfony Process Component for example) try to work around this by redirecting output to files and doing redirection in a shell.

Most of these problems can easily be solved by using a pair of connected sockets. The process calling prop_open() retains one of the sockets, the other one is passed to the child process as a HANDLE on Windows. This works only if the underlying socket does not make use of Overlapped mode, which is the default if a socket is created using socket(). We can make use of WSASocket() instead and not pass the WSA_FLAG_OVERLAPPED flag to create a socket that does not make use of overlapped IO. The PR uses a (slightly) modified copy of the socketpair() function implementation found in win32/sockets.c to achieve this. The change to the function could also be applied to the original implementation instead. This could be a BC break (overlapped mode dropped) but PHP does not use overlapped socket IO anywhere internally.

$spec = [
    0 => ['null'],
    1 => ['socket'], // ['pipe', 'w']
    2 => ['socket'] // ['pipe', 'w']
];

$proc = proc_open(['ping', 'github.com'], $spec, $pipes);

foreach ($pipes as $pipe) {
    var_dump(stream_set_blocking($pipe, false));
}

$buffers = array_fill_keys(array_keys($pipes), '');

while ($pipes) {
    $r = $pipes;
    $w = null;
    $e = null;

    if (!stream_select($r, $w, $e, null, 0)) {
        throw new \Error("Select failed");
    }
    
    foreach ($r as $i => $pipe) {
        if (!is_resource($pipe) || feof($pipe)) {
            $line = trim($buffers[$i]);
            
            if ($line !== '') {
                echo "PIPE {$i} << {$line}\n";
            }
            
            echo "DROP pipe {$i}\n";
            unset($pipes[$i]);
            continue;
        }
        
        $chunk = @fread($pipe, 8192);
        
        if ($chunk === false) {
            throw new \Error("Failed to read: " . (error_get_last()['message'] ?? 'N/A'));
        }
        
        $buffers[$i] .= $chunk;
        
        if (false !== strpos($buffers[$i], "\n")) {
            $lines = array_map('trim', explode("\n", $buffers[$i]));
            $buffers[$i] = array_pop($lines);
            
            foreach ($lines as $line) {
                echo "PIPE {$i} << {$line}\n";
            }
        }
    }
}

The example demonstrates the use of socketpairs with a spawned process. It will show "realtime" data on Windows if sockets are used. If I use pipes instead it will block after the first read and then echo the remaining output once the ping process has finished. This approach might not work for every process but since use of sockets is an opt-in behavior ist should not break any existing code.

@kooldev kooldev marked this pull request as draft June 29, 2020 12:24
Copy link
Member

@nikic nikic left a comment

Choose a reason for hiding this comment

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

Nice! Very much in favor of socketpair support. Somehow it didn't cross my mind that the solution to "pipes on Windows suck balls" is not "fix pipes with separate I/O threads" but rather "just don't use pipes".

@nikic
Copy link
Member

nikic commented Jun 29, 2020

Most of these problems can easily be solved by using a pair of connected sockets. The process calling prop_open() retains one of the sockets, the other one is passed to the child process as a HANDLE on Windows. This works only if the underlying socket does not make use of Overlapped mode, which is the default if a socket is created using socket(). We can make use of WSASocket() instead and not pass the WSA_FLAG_OVERLAPPED flag to create a socket that does not make use of overlapped IO. The PR uses a (slightly) modified copy of the socketpair() function implementation found in win32/sockets.c to achieve this. The change to the function could also be applied to the original implementation instead. This could be a BC break (overlapped mode dropped) but PHP does not use overlapped socket IO anywhere internally.

@cmb69 Do you see any problem with modifying our socketpair() polyfill in this way? It would certainly be nicer to not duplicate the implementation.

@kooldev
Copy link
Contributor Author

kooldev commented Jun 29, 2020

@nikic One thing to watch out for is that only one of the sockets is opened in non-overlapped mode (sock[1]). The reasoning behind this is that it it changes the socket that is passed to the child while the one used in the parent process is identical to what socketpair() usually returns. It is possible to create a socketpair with both ends not using overlapped IO, but it requires using AcceptEx() instead of accept(). Usage is very different from standard accept and it has to be called through a function pointer. In addition to that it requires IOCP. I did that first but went with the copy for now because it is easier to use.

Maybe the code socketpair() can be extracted to win32_socketpair(domain, type, proto, socks[2], overlapped) instead and the original function just call the new one with overlapped = 1? This would avoid all code duplication and we could call the new function in proc_open() with overlapped = 0 to create sock[1] in non-overlapped mode.

@cmb69
Copy link
Member

cmb69 commented Jun 30, 2020

Very nice! That should indeed solve quite some issues regarding the always blocking pipes on Windows.

We should also think about AF_UNIX which is available on recent Windows versions, but this PR is good without that.

@kooldev
Copy link
Contributor Author

kooldev commented Jun 30, 2020

I did some more testing and all programs that do blocking IO worked just fine with sockets. I tried starting a node.js process with sockets and it does not work. I was expecting this because libuv does some magic to detect the pipe type and (re)open it in non-blocking mode. They do not cover the SOCKET case because it is uncommon and instead try to reopen as (named) pipe to read and write with IOCP. You will get output like this if you try:

PIPE 2 << net.js:327
PIPE 2 << err = this._handle.open(fd);
PIPE 2 << ^
PIPE 2 <<
PIPE 2 << Error: EINVAL: invalid argument, uv_pipe_open
PIPE 2 << at new Socket (net.js:327:26)
PIPE 2 << at createWritableStdioStream (internal/bootstrap/switches/is_main_thread.js:67:18)
PIPE 2 << at process.getStdout [as stdout] (internal/bootstrap/switches/is_main_thread.js:117:12)
PIPE 2 << at Object.get (internal/console/constructor.js:171:38)
PIPE 2 << at Object.Console.<computed> (internal/console/constructor.js:298:46)
PIPE 2 << at Object.log (internal/console/constructor.js:309:61)
PIPE 2 << at Object.<anonymous> (C:\php-sdk\examples\console.js:15:9)
PIPE 2 << at Module._compile (internal/modules/cjs/loader.js:1138:30)
PIPE 2 << at Object.Module._extensions..js (internal/modules/cjs/loader.js:1158:10)
PIPE 2 << at Module.load (internal/modules/cjs/loader.js:986:32) {
PIPE 2 << errno: -4071,
PIPE 2 << code: 'EINVAL',
PIPE 2 << syscall: 'uv_pipe_open'
PIPE 2 << }

I think that this is not much of a problem, you can always use a real pipe or redirect to a file instead by passing this to the spec argument of proc_open(). Maybe it can even be supported in node.js / libuv but that is not related to PHP at all. I also did some testing with .bat files that prompt for user input and a socket as STDIN, it works just fine.

@kooldev kooldev marked this pull request as ready for review June 30, 2020 14:24
Copy link
Member

@nikic nikic left a comment

Choose a reason for hiding this comment

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

Looks good, just some coding style nits.

@kooldev
Copy link
Contributor Author

kooldev commented Jul 13, 2020

@nikic This should be ready to merge now. AppVeyor reports 3 failed tests, but they seem to be unrelated to the PR.

I added 2 more test cases that deal with STDIN being set to a socket or a pipe. I needed to make a little adjustment to detect_is_seekable() in plain_wrapper.c in order to make PHP properly read from a socket passed as STDIN. The Windows API function GetFileType() detects pipes, named pipes and sockets as FILE_TYPE_PIPE (documented here). I added a call to GetNamedPipeInfo() to check if the handle is actually a pipe (works with both anonymous and named pipes). If this fails is_pipe is reset to avoid calls to PeekNamedPipe() during read operations. Without this change the peek operation will fail (because it is not supported by sockets) and every read will result in an empty string being returned.

Copy link
Member

@nikic nikic left a comment

Choose a reason for hiding this comment

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

LG. @cmb69 can you please double check the GetNamedPipeInfo part?

@cmb69
Copy link
Member

cmb69 commented Jul 14, 2020

Looks good to me. Thanks!

@dktapps
Copy link
Contributor

dktapps commented Sep 2, 2022

It seems this wasn't documented or mentioned in the 8.0 changelogs. I'm only just now learning about it.

dktapps added a commit to pmmp/PocketMine-MP that referenced this pull request Sep 2, 2022
… to the main server process

Support for this was introduced in PHP 8.0, though not mentioned in any changelog: php/php-src#5777

This simplifies the subprocess handling considerably. However, there is a potential for problems if PHP generates any E_* errors, since these get written to STDOUT as well.
@cmb69
Copy link
Member

cmb69 commented Sep 2, 2022

@dktapps, it's mentioned in the migration guide; the actual proc_open() manpage still needs to be updated, though.

dktapps added a commit to pmmp/PocketMine-MP that referenced this pull request Nov 20, 2024
… to the main server process (#5273)

Support for this was introduced in PHP 8.0, though not mentioned in any changelog: php/php-src#5777

This simplifies the subprocess handling considerably. However, there is a potential for problems if PHP generates any E_* errors, since these get written to STDOUT as well.
To avoid error messages being treated as a command, a hash is attached to each IPC message, seeded with an incrementing counter. This prevents error messages causing command replays or unintended commands.

Unfortunately, PHP doesn't support binding pipes other than stdin/stdout/stderr on Windows for the child process, so we have to use stdout for this. In the future, if it becomes possible, a dedicated pipe for the purpose should be introduced. We'd need something like php://fd/<number> to work on Windows.
AzliRynz added a commit to ClousClouds/XPocketMP that referenced this pull request Feb 19, 2025
… to the main server process

Support for this was introduced in PHP 8.0, though not mentioned in any changelog: php/php-src#5777

This simplifies the subprocess handling considerably. However, there is a potential for problems if PHP generates any E_* errors, since these get written to STDOUT as well.
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.

4 participants