Skip to content
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

Run curl_setopt_ssl.phpt on Windows #16086

Merged
merged 1 commit into from
Jan 28, 2025
Merged

Conversation

cmb69
Copy link
Member

@cmb69 cmb69 commented Sep 27, 2024

There seems to be no particular reason to skip this test on Windows.

While we're at it, we also remove the check for proc_open() since this function is required (and checked) by run-tests.php anyway.

@Ayesh
Copy link
Member

Ayesh commented Sep 27, 2024

I checked on Windows (openssl 3.3 custom build, Curl 8.10.1), and the test indeed passed on Windows. I'd say lets do it and enable this for Windows.

I noted that ext/openssl/tests/stream_server_reneg_limit.phpt also skips on Windows, with 1552d6ae7bb from 10 years ago mentioning openssl binary availability being unreliable.

@cmb69
Copy link
Member Author

cmb69 commented Sep 27, 2024

@Ayesh, indeed the test passes for me locally, but might hang in CI, possibly because the pipes are never read, but openssl produces output. Using socket instead of pipe should fix that.

However, that might also be the problem on FreeBSD:

if (PHP_OS === 'FreeBSD') die('skip proc_open seems to be stuck on FreeBSD');

So maybe we should just read stdout and stderr.

I'll have a look at ext/openssl/tests/stream_server_reneg_limit.phpt shortly. Thanks for mentioning!

@cmb69
Copy link
Member Author

cmb69 commented Sep 27, 2024

Hmm, there's still something wrong. Needs further investigation.

@cmb69 cmb69 force-pushed the cmb/curl_setopt_ssl.phpt branch from 525fcc8 to 28682b9 Compare November 29, 2024 17:26
@cmb69
Copy link
Member Author

cmb69 commented Nov 29, 2024

Rebased and force-pushed to trigger CI again (last failure looks actually completely unrelated).

@cmb69
Copy link
Member Author

cmb69 commented Nov 30, 2024

At least one of these tests stalls in CI; could reproduce locally too, but have not been able to resolve. More important things to do right now, so putting this to backlog.

@cmb69
Copy link
Member Author

cmb69 commented Dec 17, 2024

Gosh! When running the test (locally) the first time, it stalls. Afterwards, the openssl s_client process is still running, so running the test again works. proc_open() would not fail, because CreateProcessW doesn't wait until the process has finished its initialization, but returns success early. Why the test hangs for the first run, and why openssl is not closed, see 430a04f.

@cmb69 cmb69 marked this pull request as ready for review December 17, 2024 17:57
@cmb69 cmb69 requested a review from adoy as a code owner December 17, 2024 17:57
@cmb69
Copy link
Member Author

cmb69 commented Dec 17, 2024

Seems to work now. Test failure is unrelated; I've left a comment at #16875 (comment).

The whole point of using `proc_open()` to execute `openssl s_client` is
that we can terminate the process when we're done.  However, when going
through the shell on Windows, we get a handle to the shell process, and
if we terminate that, the grandchild will stay open.  Since the pipes
of the grandchild will stay open, the PHP process will not terminate
either, so the test stalls.

We solve this by simply bypassing the shell.
@cmb69 cmb69 force-pushed the cmb/curl_setopt_ssl.phpt branch from 430a04f to 74b33e4 Compare January 27, 2025 23:31
@cmb69 cmb69 merged commit 06c41ec into php:master Jan 28, 2025
9 checks passed
@cmb69 cmb69 deleted the cmb/curl_setopt_ssl.phpt branch January 28, 2025 00:15
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.

2 participants