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

WP-NOW: Use rotatePHPRuntime to avoid memory error #152

Merged
merged 6 commits into from Mar 21, 2024

Conversation

sejas
Copy link
Collaborator

@sejas sejas commented Feb 5, 2024

What?

It uses the new function rotatePHPRuntime to mitigate the memory error.

How?

Update the dependencies and use rotatePHPRuntime for the main instance.

Testing Instructions

  • Run:
    • nvm use && npm install
    • npx nx build wp-now
    • node dist/packages/wp-now/cli.js start
    • Observe wp-now has started correctly

};

const phpInstances = [];
const loadRuntime = async () =>
await NodePHP.loadRuntime(options.phpVersion);
Copy link
Collaborator

Choose a reason for hiding this comment

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

Property 'loadRuntime' does not exist on type 'typeof NodePHP'.

This GitHub message looks suspicious, otherwise the PR seems to look good.

Copy link
Collaborator Author

@sejas sejas Mar 21, 2024

Choose a reason for hiding this comment

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

I think the GitHub message was because we weren't using the latest version of Playground dependencies. Now it's gone.

@adamziel adamziel added this to the Zero Crashes – Tools milestone Mar 4, 2024
@adamziel
Copy link
Collaborator

@brandonpayton this could be interesting for you, and it would also make wp-now far more usable.

@brandonpayton
Copy link
Member

@brandonpayton this could be interesting for you, and it would also make wp-now far more usable.

@adamziel, that sounds good! I can take a look.

@sejas
Copy link
Collaborator Author

sejas commented Mar 21, 2024

@adamziel , @bgrgicak , Now that we updated and adapted the Playground dependencies to the latest version, I could finish this PR. Would you mind reviewing it?

cc: @brandonpayton

await rotatePHPRuntime({
php,
recreateRuntime: loadRuntime,
maxRequests: 400,
Copy link
Collaborator

Choose a reason for hiding this comment

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

Maybe let's try 100? Also, it would be nice to detect out of memory errors and auto-rotate – not that it's a blocker here.

Suggested change
maxRequests: 400,
maxRequests: 100,

Copy link
Collaborator

@adamziel adamziel left a comment

Choose a reason for hiding this comment

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

LGTM!

@sejas sejas merged commit 6f7c5db into trunk Mar 21, 2024
2 checks passed
@sejas sejas deleted the add/wp-now-rotate-php-runtime branch March 21, 2024 17:26
sejas added a commit that referenced this pull request Mar 22, 2024
sejas added a commit that referenced this pull request Mar 22, 2024
Reverts #152

The php instance rotates correctly, but the new php instance doesn't
have any folder mounted. It's a clean php.

Reverting for now, and I'll create another PR.

After rotating I get this result: `404 File not found`, and I confirmed
the document root folder doesn't exist in VFS and therefore the
WordPress files.

<img width="518" alt="Captura de pantalla 2024-03-22 a las 12 52 35"
src="https://github.com/WordPress/playground-tools/assets/779993/7242e364-cbb8-48a7-9e39-a703b49bba26">

cc: @adamziel
@sejas
Copy link
Collaborator Author

sejas commented Mar 22, 2024

@adamziel , I reverted this PR. As it seems it was returning a 404 File not found after the first rotation. I'll debug it more and create another PR.

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

Successfully merging this pull request may close these issues.

None yet

3 participants