-
Notifications
You must be signed in to change notification settings - Fork 7.9k
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
Conversation
There was a problem hiding this 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".
@cmb69 Do you see any problem with modifying our |
@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 Maybe the code |
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. |
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:
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 |
There was a problem hiding this 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.
@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 |
There was a problem hiding this 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?
Looks good to me. Thanks! |
It seems this wasn't documented or mentioned in the 8.0 changelogs. I'm only just now learning about it. |
… 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.
@dktapps, it's mentioned in the migration guide; the actual |
… 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.
… 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.
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 instream_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 aHANDLE
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 usingsocket()
. We can make use ofWSASocket()
instead and not pass theWSA_FLAG_OVERLAPPED
flag to create a socket that does not make use of overlapped IO. The PR uses a (slightly) modified copy of thesocketpair()
function implementation found inwin32/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.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.