Skip to content

Reintroduce proc_open_multiplex.phpt #17192

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

Merged
merged 2 commits into from
Dec 25, 2024

Conversation

cmb69
Copy link
Member

@cmb69 cmb69 commented Dec 17, 2024

The main intent of the test was to show the changed behavior on Windows; previously, stream_select() would return immediately there, reporting all pipes as ready; now, it only returns if at least one pipe is actually ready.

The original test case was overspecified; of course, we cannot assume that the pipes are ready one after the other; depending on the concrete select(2) implementation and the system scheduler, minor differences are to be expected.

Thus we relax the test expectations, and now require that not all pipes are reported ready after a single stream_select() call, and that the output contains all strings. We also ensure that stream_select() doesn't fail (for whatever reason). And in case of the test expectations not being met, we also output some diagnostics (most notably the output that has already been read).

The main intent of the test was to show the changed behavior on
Windows; previously, `stream_select()` would return immediately there,
reporting all pipes as ready; now, it only returns if at least one pipe
is actually ready.

The original test case was overspecified; of course, we cannot assume
that the pipes are ready one after the other; depending on the concrete
`select(2)` implementation and the system scheduler, minor differences
are to be expected.

Thus we relax the test expectations, and now require that not all pipes
are reported ready after a single `stream_select()` call, and that the
output contains all strings.  We also ensure that `stream_select()`
doesn't fail (for whatever reason).  And in case of the test
expectations not being met, we also output some diagnostics (most
notably the output that has already been read).
@cmb69
Copy link
Member Author

cmb69 commented Dec 17, 2024

I've retriggered both macOS jobs, and they passed again, so this might be good this time.

@cmb69 cmb69 marked this pull request as ready for review December 17, 2024 16:02
@cmb69 cmb69 requested a review from bukka as a code owner December 17, 2024 16:02
];
$read_pipes = [];
for ($i = 0; $i < 10; $i++) {
$procs[] = proc_open([$php, "-r", "usleep(100000 * (10 - $i)); echo 'hello$i';"], $desc, $pipes);
Copy link
Member

Choose a reason for hiding this comment

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

Does it really need to take the whole second (10 x 100ms) to test this? I think it should work for shorter interval to test what is tested here.

Copy link
Member Author

Choose a reason for hiding this comment

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

I've reduced to 10 * 10ms, and CI is still green.

@cmb69 cmb69 merged commit 2021a58 into php:master Dec 25, 2024
10 checks passed
@cmb69 cmb69 deleted the cmb/proc_open_multiplex.phpt branch December 25, 2024 16:42
@iluuu1994
Copy link
Member

It seems this job can fail: https://github.com/php/php-src/actions/runs/13868159277/job/38811107251
I don't know if it's due to the PPC runner, or can happen in other environments. /cc @NattyNarwhal

@NattyNarwhal
Copy link
Member

Testing it in an interactive session and it seems to work there consistently running it quite a few times. My guess is it might be runner environment, (i.e. PTYs? though this test doesn't use PTYs, just piped I/O) but I'm not certain about that.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants