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

PHP: Support read() from a blocking pipe #931

Merged
merged 15 commits into from
Jan 18, 2024
Merged

Conversation

adamziel
Copy link
Collaborator

@adamziel adamziel commented Jan 10, 2024

Shims read(2) functionallity by providing an alternative read()
function called wasm_read() that gracefully handles blocking
pipes. This is neecessary because Emscripten returns EWOULDBLOCK
when reading from a blocking pipe, whereas a clang-compiled
program would wait until data becomes available.

See:

Other changes

This PR also ships:

  • A regexp fix for @php-wasm/cli to preserve a trailing whitespace when rewrite PHP-related spawn shell commands.
  • A fix to correctly propagate the child process exit code back to PHP. The WordPress bootstrap script for PHPUnit needs that to work correctly.

Motivation

wp-cli

Without this PR, running wp-cli.phar doesn't output anything. The output is piped through a pager like less using proc_open and then displayed by reading from a blocking pipe.

With this PR, running wp-cli.phar returns its regular help message as the pager pipe can be read:

NAME
  wp
DESCRIPTION
  Manage WordPress through the command-line.
SYNOPSIS
  wp <command>
SUBCOMMANDS
  cache                 Adds, removes, fetches, and flushes the WP Object Cache
                        object.
  cap                   Adds, removes, and lists capabilities of a user role.
  cli                   Reviews current WP-CLI info, checks for updates, or
                        views defined aliases.
...

PHPUnit

With this PR, Playground can run PHPunit on wordpress-develop!

TMPDIR=/tmp PHP=8.2 node --loader ../plugins/playground/packages/nx-extensions/src/executors/built-script/loader.mjs ../plugins/playground/dist/packages/php-wasm/cli/main.js ./vendor/bin/phpunit --no-coverage -v
(node:87723) ExperimentalWarning: Custom ESM Loaders is an experimental feature. This feature could change at any time
(Use `node --trace-warnings ...` to show where the warning was created)
Installing...
aaabb
int(0)
Running as single site... To run multisite, use -c tests/phpunit/multisite.xml
Not running ajax tests. To execute these, use --group ajax.
Not running ms-files tests. To execute these, use --group ms-files.
Not running external-http tests. To execute these, use --group external-http.
PHPUnit 9.6.15 by Sebastian Bergmann and contributors.

Runtime:       PHP 8.2.10-dev
Configuration: /Users/cloudnik/www/Automattic/core/wordpress-develop/phpunit.xml.dist
Warning:       Your XML configuration validates against a deprecated schema.
Suggestion:    Migrate your XML configuration using "--migrate-configuration"!

...........................................................    59 / 17622 (  0%)
...........................................................   118 / 17622 (  0%)
............................FFFFFF.........................   177 / 17622 (  1%)
...........................................................   236 / 17622 (  1%)

Testing instructions

Confirm the proc_open() tests pass on CI. This PR adjusts a few
of them to make sure the output is read without the tricky sleep()
call.

Related:

CC @mho22

@adamziel
Copy link
Collaborator Author

Relevant explorations: #951 (comment)

Shims read(2) functionallity by providing an alternative read()
function called wasm_read() that gracefully handles blocking
pipes. This is neecessary because Emscripten does not support
blocking pipes and instead returns EWOULDBLOCK.

See:

* #951
* emscripten-core/emscripten#13214

 ## Testing instructions

Confirm the `proc_open()` tests pass on CI. This PR adjusts a few
of them to make sure the output is read without the tricky sleep()
call.
@adamziel adamziel marked this pull request as ready for review January 17, 2024 15:35
@adamziel adamziel changed the title proc_open – don't require sleep() call PHP: Support read() from a blocking pipe Jan 17, 2024

if (returnCode === 6 /*EWOULDBLOCK*/) {
return Asyncify.handleSleep(function (wakeUp) {
var timeout = 10000; // @TODO Do not hardcode this
Copy link
Member

Choose a reason for hiding this comment

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

😄

what actually happens if we do timeout letting this run indefinitely? will the PHP process run its own timeout? if that's the case, is it safe to let this block forever?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Aha, good spot. Yes, I think we're good to just let this block until PHP times out.

Copy link
Collaborator Author

@adamziel adamziel Jan 17, 2024

Choose a reason for hiding this comment

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

Actually, this may block a PHPUnit test suite without any visible feedback so I think we have to give up eventually. It's a shame, I'd rather rely on PHP timeout.

@adamziel adamziel merged commit a1f324e into trunk Jan 18, 2024
5 checks passed
@adamziel adamziel deleted the proc_open-make-sleep-optional branch January 18, 2024 00:12
@adamziel
Copy link
Collaborator Author

One remaining issue is reading from a blocking file pipe as opposed to just a process pipe.

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

Successfully merging this pull request may close these issues.

2 participants