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.

Sign up for GitHub

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

Jump to bottom

PHP: Explore building with pthreads #346

Closed
wants to merge 3 commits into from
Closed

PHP: Explore building with pthreads #346

wants to merge 3 commits into from

Conversation

adamziel
Copy link
Collaborator

@adamziel adamziel commented May 15, 2023 •
edited
Loading

This PR attempts to add pthreads and pcntl support necessary to handle pcntl_fork. Full PHPUnit support hinges on this, and AFAIR there's a few wp-cli commands also relying on threads. Pthreads support doesn't seem to be necessary for any mission-critical PHP feature or package. I used to think it is required to support fork(), but that's not actually the case.

I disabled all the extensions and got vanilla PHP to build. Unfortunately, the basic test.js file included in the PR leads to the following error:

zend_mm_heap corrupted

The error happens inside php_request_startup(). If I compile just PHP 7.4 without php_wasm.c and run zend_eval_string(code, NULL, "php-wasm run script"), the error turns into Bus Error – which is also what happens if eval is triggered via _run_php in php_wasm.c.

I wonder if -DZEND_ENABLE_STATIC_TSRMLS_CACHE=1 would help.

Related WordPress/playground-tools#25

cc @dmsnell @danielbachhuber @sejas

@adamziel adamziel mentioned this pull request May 16, 2023
Open
@adamziel adamziel added the [Type] Enhancement New feature or request label May 16, 2023
This was referenced May 22, 2023
Closed
Closed
@wojtekn
Copy link
Collaborator

wojtekn commented May 22, 2023

@adamziel I experienced a similar issue when I tried to save a post in the wp-now environment for my blog. Is this PR ready to test? Do you know a way to check which function causes the crash?

@adamziel
Copy link
Collaborator Author

This PR is not ready to test at all, unfortunately. It's at a stage where it builds, which is good progress and much further than I ever got before, but it doesn't run yet. I haven't prioritized finishing this PR – I just wanted to make it public for anyone interested to play with. If you'd like to take a stab at pthreads support, you're more than welcome to!

@adamziel
Copy link
Collaborator Author

I experienced a similar issue when I tried to save a post in the wp-now environment for my blog. Is this PR ready to test? Do you know a way to check which function causes the crash?

You mean with this PR applied? Or in a vanilla wp-now environment? If it's the latter, would you create a separate issue to track it? We'd need to build PHP with debugging symbols and spend some time in dev tools to pinpoint where exactly is it crashing.

@sejas sejas mentioned this pull request May 23, 2023
Merged
@wojtekn
Copy link
Collaborator

wojtekn commented May 23, 2023

You mean with this PR applied?

Yes, I was wondering if testing this PR against my blog makes sense to see if it still crashes. It's clear to me now that it's not finished yet.

If it's the latter, would you create a separate issue to track it?

Yes, I've added #416

@adamziel adamziel mentioned this pull request May 25, 2023
Closed
@adamziel
Copy link
Collaborator Author

adamziel commented May 30, 2023 •
edited
Loading

Adding a console.trace() to the exitRuntime() function yielded the following stack trace:

> node packages/php-wasm/node/test.js

munmap() failed: [28] Invalid argument
munmap() failed: [28] Invalid argument
zend_mm_heap corrupted
Trace
    at exitRuntime (/Users/cloudnik/www/Automattic/core/plugins/playground/packages/php-wasm/node/public/php_7_4.js:603:10)
    at exitJS (/Users/cloudnik/www/Automattic/core/plugins/playground/packages/php-wasm/node/public/php_7_4.js:3763:3)
    at zend_mm_panic (wasm://wasm/0123feb6:wasm-function[2456]:0xfda68)
    at _efree (wasm://wasm/0123feb6:wasm-function[2473]:0x1005c9)
    at zm_activate_date (wasm://wasm/0123feb6:wasm-function[2891]:0x15562f)
    at byn$fpcast-emu$zm_activate_date (wasm://wasm/0123feb6:wasm-function[7885]:0x2cbc03)
    at zend_activate_modules (wasm://wasm/0123feb6:wasm-function[1142]:0x8b77b)
    at byn$fpcast-emu$zend_activate_modules (wasm://wasm/0123feb6:wasm-function[9710]:0x2d2124)
    at dynCall_v (wasm://wasm/0123feb6:wasm-function[6419]:0x2c798b)
    at /Users/cloudnik/www/Automattic/core/plugins/playground/packages/php-wasm/node/public/php_7_4.js:756:20

adamziel pushed a commit that referenced this pull request May 30, 2023
@sejas @danielbachhuber
wp-now: Add executeWPCli() function to download and execute WP-CLI (#…
6170984 
…395)

## What?

- Add the `executeWPCli` function to download and execute `wp-cli. 
- In the future, we may include the command `wp-now wp`. Currently, we
drop it out until we improve the pthreads execution. Currently a PR in
progress: #346
- Surface `emscriptenOptions` to catch print and print error for
`wp-cli` execution.

## Why?

- See https://github.com/WordPress/wordpress-playground/issues/269

## How?

It downloads the wp-cli.phar file if the file doesn't exist, then uses
`php.cli()` to execute it.
There are some limitations in the `wp-cli` features. Some of them may
not work.

## Testing Instructions

-  Check out this branch.
- Copy your path to your theme or plugin
- After installing and building the project, run:
- Run the tests `npx nx test wp-now`
- Observe the tests pass.

<!--details>
<summary>~`WP_NOW_PROJECT_PATH=/path/to/your-theme-or-plugin npx nx
preview wp-now wp user list`~ </summary>

```
> nx run wp-now:preview wp user list

+----+------------+--------------+--------------+--------------+---------------+
| ID | user_login | display_name | user_email   | user_registe | roles         |
|    |            |              |              | red          |               |
+----+------------+--------------+--------------+--------------+---------------+
| 1  | admin      | admin        | admin@localh | 2023-05-19 1 | administrator |
|    |            |              | ost.com      | 7:33:35      |               |
+----+------------+--------------+--------------+--------------+---------------+

  >  NX   Successfully ran target preview for project wp-now and 12 tasks it depends on (10s)
 
         With additional flags:
           wp user list
```
</details!-->

---------

Co-authored-by: Daniel Bachhuber <daniel.bachhuber@automattic.com>
@adamziel adamziel mentioned this pull request Jun 25, 2023
Closed
@adamziel
Copy link
Collaborator Author

Turns out pthreads support is completely unrelated to pcntl and pcntl_fork(). I'll close this PR, then. It would take a lot of work to finish for no clear benefit other than supporting the Thread class which is typically polyfilled with pcntl_fork() anyway.

@adamziel adamziel closed this Jun 25, 2023
Pookie717 added a commit to Pookie717/wordpress-playground that referenced this pull request Oct 1, 2023
@Pookie717 @danielbachhuber
wp-now: Add executeWPCli() function to download and execute WP-CLI …
00aa039 
…(#395)

## What?

- Add the `executeWPCli` function to download and execute `wp-cli. 
- In the future, we may include the command `wp-now wp`. Currently, we
drop it out until we improve the pthreads execution. Currently a PR in
progress: WordPress/wordpress-playground#346
- Surface `emscriptenOptions` to catch print and print error for
`wp-cli` execution.

## Why?

- See https://github.com/WordPress/wordpress-playground/issues/269

## How?

It downloads the wp-cli.phar file if the file doesn't exist, then uses
`php.cli()` to execute it.
There are some limitations in the `wp-cli` features. Some of them may
not work.

## Testing Instructions

-  Check out this branch.
- Copy your path to your theme or plugin
- After installing and building the project, run:
- Run the tests `npx nx test wp-now`
- Observe the tests pass.

<!--details>
<summary>~`WP_NOW_PROJECT_PATH=/path/to/your-theme-or-plugin npx nx
preview wp-now wp user list`~ </summary>

```
> nx run wp-now:preview wp user list

+----+------------+--------------+--------------+--------------+---------------+
| ID | user_login | display_name | user_email   | user_registe | roles         |
|    |            |              |              | red          |               |
+----+------------+--------------+--------------+--------------+---------------+
| 1  | admin      | admin        | admin@localh | 2023-05-19 1 | administrator |
|    |            |              | ost.com      | 7:33:35      |               |
+----+------------+--------------+--------------+--------------+---------------+

  >  NX   Successfully ran target preview for project wp-now and 12 tasks it depends on (10s)
 
         With additional flags:
           wp user list
```
</details!-->

---------

Co-authored-by: Daniel Bachhuber <daniel.bachhuber@automattic.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
[Feature] PHP.wasm [Type] Enhancement New feature or request
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@adamziel @wojtekn