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

Add npm run fix-asyncify task that adds the missing ASYNCIFY_ONLY items #253

Merged
merged 13 commits into from
May 9, 2023

Conversation

adamziel
Copy link
Collaborator

@adamziel adamziel commented May 5, 2023

See #251

Description

With this commit, fixing Asyncify issues only requires:

  1. Adding a test case that triggers a crash to packages/php-wasm/node/src/test/php-asyncify.spec.ts
  2. Running: npm run fix-asyncify

The npm run fix-asyncify command

  1. Runs Asyncify test suite that makes PHP trigger an async call through all known code paths. If it works, we're done. If it fails, keep going.
  2. Automatically adds any missing C functions to the ASYNCIFY_ONLY list in the Dockerfile
  3. Rebuilds PHP
  4. Loops to 1

To make it all work, this PR:

  • Converts unhandled Asyncify rejections into catchable errors thrown inside PHP.run()
  • Logs PHP functions at the time of an async call, not at the time of rewinding
  • Prevents Node.js from exiting or hiding the error message when the WASM module calls exit()

Follow-up work

  • Add a documentation page
  • Add more test cases to php-asyncify.spec.js

cc @sejas @danielbachhuber

@adamziel adamziel force-pushed the automate-asyncify-debugging branch from fd4d1cd to 302ea84 Compare May 9, 2023 14:37
@adamziel adamziel mentioned this pull request May 9, 2023
56 tasks
@adamziel
Copy link
Collaborator Author

adamziel commented May 9, 2023

I cleaned this one up. I'll merge once the tests pass

@adamziel
Copy link
Collaborator Author

adamziel commented May 9, 2023

One more update needed here, I don't think all the PHP versions were correctly rebuilt

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.

1 participant