-
Notifications
You must be signed in to change notification settings - Fork 7.9k
Emulate non-blocking pipes in proc_open() on Windows #5864
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
@@ -0,0 +1,69 @@ | |||
--TEST-- |
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.
This is missing a --SKIPIF--
section.
We generally use PHP_OS_FAMILY !== 'Windows'
or this in other tests
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.
I left out SKIPIF because I want the test to verify that pipes behave in exactly the same way on Linux and Windows systems when threads are used on Windows. The threaded_pipes
option has to be ignored for all other builds.
@nikic @cmb69 I forgot to flag the PR as a draft. It needs better error handling (errors in threads need to be propagated to the stream in order to trigger a warning and fail stream operations) and socket / temp buffer sizes will likely need to be adjusted. I created the PR to get some feedback if this feature is something that you would like to see implemented in PHP before I put in more work. |
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.
Thanks for the PR! That's certainly a fascinating solution for a real problem (although I had hoped that would have been completely solved with PR #5777). The "obvious" solution for the issue of blocking pipes would be to use overlapped IO for these in the first place, but that would be way more intrusive, so this solution might be preferable. I'd need more time to come to a well-informed conclusion.
Anyway, @DaveRandom may have some thought about this.
@cmb69 Thanks for lloking into this. I know that there are other ways to implement this. Let me provide some background why I ended up implementing it the way I did: The ultimate goal is to expose process pipes as PHP streams to userland. The solution currently provided by
This is the way the "threaded pipe" (as implemented in this PR) wraps a Windows pipe to make it fit into PHP's concecpt of non-blocking IO. The socket pair acts as a proxy between a PHP stream and the process pipes. The stream supports non-blocking mode and event notification because it wraps one end of the socket pair. A nice property of that design is that there is no need to explicitly synchronize threads because it comes for free with socket IO operations. The obvious downside is the need for a socket pair and a thread for each process pipe and the additional IO performed by the thread that adds IO calls and a little latency.
This illustrates an alternative design that I considered. It involves a single IOCP thread that multiplexes IO calls across all process pipes. While it does save resources by using only a single thread it adds a lot of complexity to the implementation. It would require us to manually create a named pipe pair to pass to the child process (to have the parent end of the pipe support overlapped mode). It would also require one end of the socket pair to support overlapped IO (which is already the case). In addition to that the IOCP thread has to keep state for each socket-pipe pair to alternate between read and write. In addition to that is requires synchronized access to the IOCP thread to manage active pipes. And finally closing a PHP stream requires cancellation of pending IO operations and another sync with the IOCP thread to remove the stream from the overlapped IO set. I wanted to keep things simple and went with the first design for the PR. It is meant to be used as fallback solution in situations where #5777 fails to deliver. I do not know of any way to make this work without putting the socket pair between PHP userland and the pipe. Even with IOCP there is no direct way to integrate a process pipe with If anyone can think of an alternative (or even better way) to achieve this I would be very interested to know. I hope this was not too much information. I just wanted to write it down for reference. Maybe it will be helpful to anyone (my future self included). ;-) |
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.
I like the feature, but can't meaningfully review this as I'm not familiar with the relevant Windows APIs, or even their I/O model.
{ | ||
threaded_pipe *data = (threaded_pipe *) stream->abstract; | ||
|
||
if (threaded_error_check(data, "Pipe write failed: [%d] %s")) { |
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.
I would prefer passing in only "Pipe write failed"
and let the rest of the message be constructed by threaded_error_check
. Separating format string from arguments makes me uncomfortable...
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.
I also don't really get why the error check is performed before the operation?
@@ -420,7 +420,7 @@ SECURITY_ATTRIBUTES php_proc_open_security = { | |||
.bInheritHandle = TRUE | |||
}; | |||
|
|||
# define pipe(pair) (CreatePipe(&pair[0], &pair[1], &php_proc_open_security, 0) ? 0 : -1) | |||
# define pipe(pair) (CreatePipe(&pair[0], &pair[1], &php_proc_open_security, 8192) ? 0 : -1) |
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.
What is the magic 8192 value here?
php_socket_t sock; /* Internal end of the proxy socket pair. */ | ||
php_stream *stream; /* Wraps the exposed end of the socket pair. */ | ||
const char *mode; /* Pipe open mode. */ | ||
int closed; /* Set to 1 on stream close. */ |
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.
int closed; /* Set to 1 on stream close. */ | |
bool closed; /* Set to 1 on stream close. */ |
/* Interrupt IO calls in thread and wait with timeout to prevent race conditions. */ | ||
do { | ||
CancelSynchronousIo(data->thread); | ||
} while (WAIT_TIMEOUT == WaitForSingleObject(data->thread, 5)); |
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.
This looks a bit fishy...
Status Update: I did a lot more testing and while this soultion works for pipes that the child process writes to (STDOUT, STDERR) it fails to work with STDIN in the correct way. My assumption was that I could use the socket pair as a data buffer sitting between userland and the pipe. The important thing is to keep the socket buffer size small to ensure that all buffered data can be written to the pipe without blocking. The only way to do this is to set I also tried with Due to the these issues I lean towards closing this PR because it does not achieve what I intended it to do in a satisfactory way. There seem to be no workarounds for the TCP case and finding any resources related to using AF_UNIX on Windows is close to impossible... |
Bummer! (guess it wouldn't make sense to add general So for solving the blocking pipes issue on Windows, we're likely back to using overlapped IO in the streams layer – fiddly, and likely too late for 8.0. Anyhow, thanks for working on this – highly appreciated! :) |
@cmb69 Do you know of a way to make overlapped IO work with PHP streams? I am (at least a bit) familiar with IOCP but I do not see a way to use it in non-blocking streams. AFAIK overlapped IO just schedules operations that send completion messages to a queue whereas PHP would need a way to know if an operation can be performed without blocking (to be used with |
I am closing this PR due to problems with both TCP and UNIX sockets on Windows that (seemingly) cannot be worked around. Using overlapped IO / IOCP for process pipes is the way to go on Windows, but I cannot see how it could fit PHP's concept of non-blocking streams... |
This PR adds a new option
threaded_pipes
toother_options
inproc_open()
. This feature (like all the other options) is exclusive to Windows. Threaded pipes provide non-blocking streams in situations where the solution implemented in #5777 is not supported by the spawned child process (for example node.js fails to do so). The downside of this approach is the need to spawn a thread for each non-closed process pipe. In addition to that we need an additional pair of sockets for each pipe. The need to tunnel data through threads involves additional system calls (ReadFile()
,WriteFile()
,WSARecv()
,WSASend()
) that add (a little) latency to process IO operations. We do however end up with a solution that can avoid all issues related to child process pipe blocking on Windows reliably.Threaded pipes are backed by a Windows thread, an anonymous pipe (the parent end of a process pipe) and a socket pair. In read mode (STDOUT / STDERR) the thread will do blocking reads from the process pipe and write the received data to one end of the socket pair. The other end of the socket pair is exposed to PHP userland. It can be used to read data from the process pipe, supports non-blocking mode and can be polled for events by
stream_select()
. Write mode (STDIN) works in the same way, the thread reads data from the socket pair and writes it to the process pipe. The other end of the socket pair is exposed to userland and has full support for non-blocking / select.This little example shows how to use threaded pipes. Note that I used the exact same example in #5777 and in the case of
ping
we could usesocket
instead ofpipe
just fine (it would be faster and use fewer system resources). Spawning a node.js proccess works just fine with threaded pipes whereas it would fail with just sockets.It is possible to change the threading model and use a single thread to power all pipes but it will be a lot more complicated to implement and only be beneficial if a lot of processes / pipes are alive at the same time. The only model that can achieve this on Windows is IOCP. Using it would require us to manually create a pair of named pipes and enable overlapped mode / IOCP for the parent end. It would also require the (internal) end of the socket pair to use overlapped mode and the same IOCP. The thread processing all of this would need to call GetQueuedCompletionStatus() and handle results and pipe state. While this model makes sense when all your IO is async / non-blocking I think it will not offer much benefit for the
proc_open()
use case.The basic idea (and maybe even implementation) could also be reused to provide non-blocking access to PHP's STDIN / STDOUT / STDERR when PHP is run from the cli. This is just an idea for a possible addition, none of this is implemented currently.