Skip to content

Commit

Permalink
Improve fix for GH-16889
Browse files Browse the repository at this point in the history
The original patch[1] cared only about pipe handles in the rset, but
would be problematic if there are other handles (e.g. files in the
rset, or pipes/files in the other sets), because `php_select()` would
return immediately, reporting all non read-pipe handles as ready, but
possibly never reporting read-pipe handles.

We fix this by applying different logic for the case where only pipe
handles are supplied in the rset, but no handles in the wset or eset.
In this case `php_select()` only returns when actually one of the
handles is ready, or when the timeout expires.  To avoid busy looping
in this case, we sleep for a short amount of time.  This matches POSIX
behavior.

In all other cases, `php_select()` behaves as before (i.e. prior to the
original fix), that is it returns immediately, reporting all handles as
ready.

We also add a test case that demonstrates multiplexing the output of a
couple of child processes.

See also the discussion on <#16917>.

[1] <b614b4a>

Closes GH-17174.
  • Loading branch information
cmb69 committed Dec 16, 2024
1 parent fadceca commit 6972612
Show file tree
Hide file tree
Showing 3 changed files with 56 additions and 2 deletions.
7 changes: 7 additions & 0 deletions UPGRADING
Original file line number Diff line number Diff line change
Expand Up @@ -189,6 +189,13 @@ PHP 8.5 UPGRADE NOTES
and FFI::load(). However, this convenience feature should not be used in
production.

* Streams:
. If only pipe streams are contained in the $read array, and the $write and
$except arrays are empty, stream_select() now behaves similar to POSIX
systems, i.e. the function only returns if at least one pipe is ready to be
read, or after the timeout expires. Previously, stream_select() returned
immediately, reporting all streams as ready to read.

========================================
13. Other Changes
========================================
Expand Down
37 changes: 37 additions & 0 deletions ext/standard/tests/general_functions/proc_open_multiplex.phpt
Original file line number Diff line number Diff line change
@@ -0,0 +1,37 @@
--TEST--
Multiplexing of child output
--FILE--
<?php
$php = getenv("TEST_PHP_EXECUTABLE");
$desc = [
["null"],
["pipe", "w"],
["null"]
];
$read_pipes = [];
for ($i = 0; $i < 10; $i++) {
$procs[] = proc_open([$php, "-r", "usleep(100000 * (10 - $i)); echo 'hello$i';"], $desc, $pipes);
$read_pipes[] = $pipes[1];
}
$rset = $read_pipes;
$wset = null;
$eset = null;
while (!empty($read_pipes) && stream_select($rset, $wset, $eset, 2) > 0) {
foreach ($rset as $pipe) {
echo fread($pipe, 6), "\n";
unset($read_pipes[array_search($pipe, $read_pipes)]);
}
$rset = $read_pipes;
}
?>
--EXPECT--
hello9
hello8
hello7
hello6
hello5
hello4
hello3
hello2
hello1
hello0
14 changes: 12 additions & 2 deletions win32/select.c
Original file line number Diff line number Diff line change
Expand Up @@ -16,12 +16,15 @@

#include "php.h"
#include "php_network.h"
#include "win32/time.h"

/* Win32 select() will only work with sockets, so we roll our own implementation here.
* - If you supply only sockets, this simply passes through to winsock select().
* - If you supply file handles, there is no way to distinguish between
* ready for read/write or OOB, so any set in which the handle is found will
* be marked as ready. Pipes will be checked if they are ready for read, though.
* be marked as ready.
* - If you supply only pipe handles in rfds, and no handles in wfds or efds,
* the pipes will only be marked as ready if there is data available.
* - If you supply a mixture of handles and sockets, the system will interleave
* calls between select() and WaitForMultipleObjects(). The time slicing may
* cause this function call to take up to 100 ms longer than you specified.
Expand All @@ -34,6 +37,7 @@ PHPAPI int php_select(php_socket_t max_fd, fd_set *rfds, fd_set *wfds, fd_set *e
HANDLE handles[MAXIMUM_WAIT_OBJECTS];
int handle_slot_to_fd[MAXIMUM_WAIT_OBJECTS];
int n_handles = 0, i;
int num_read_pipes = 0;
fd_set sock_read, sock_write, sock_except;
fd_set aread, awrite, aexcept;
int sock_max_fd = -1;
Expand Down Expand Up @@ -78,6 +82,9 @@ PHPAPI int php_select(php_socket_t max_fd, fd_set *rfds, fd_set *wfds, fd_set *e
sock_max_fd = i;
}
} else {
if (SAFE_FD_ISSET(i, rfds) && GetFileType(handles[n_handles]) == FILE_TYPE_PIPE) {
num_read_pipes++;
}
handle_slot_to_fd[n_handles] = i;
n_handles++;
}
Expand Down Expand Up @@ -136,7 +143,7 @@ PHPAPI int php_select(php_socket_t max_fd, fd_set *rfds, fd_set *wfds, fd_set *e
if (WAIT_OBJECT_0 == WaitForSingleObject(handles[i], 0)) {
if (SAFE_FD_ISSET(handle_slot_to_fd[i], rfds)) {
DWORD avail_read = 0;
if (GetFileType(handles[i]) != FILE_TYPE_PIPE
if (num_read_pipes < n_handles
|| !PeekNamedPipe(handles[i], NULL, 0, NULL, &avail_read, NULL)
|| avail_read > 0
) {
Expand All @@ -156,6 +163,9 @@ PHPAPI int php_select(php_socket_t max_fd, fd_set *rfds, fd_set *wfds, fd_set *e
}
}
}
if (retcode == 0 && num_read_pipes == n_handles && sock_max_fd < 0) {
usleep(100);
}
} while (retcode == 0 && (ms_total == INFINITE || GetTickCount64() < limit));

if (rfds) {
Expand Down

2 comments on commit 6972612

@iluuu1994
Copy link
Member

@iluuu1994 iluuu1994 commented on 6972612 Dec 17, 2024

Choose a reason for hiding this comment

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

@cmb69 Not sure if you've seen, but ext/standard/tests/general_functions/proc_open_multiplex.phpt fails in the macOS build.

@cmb69
Copy link
Member Author

@cmb69 cmb69 commented on 6972612 Dec 17, 2024

Choose a reason for hiding this comment

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

Thanks! I have dropped the test for now; will come up with a more solid alternative.

Please sign in to comment.