Fix shutdown errors #1104
Fix shutdown errors #1104
Conversation
|
@adamziel I tried adding tests, but wasn't able to crash it using PHP shutdown functions. |
bgrgicak
added
[Type] Bug
Snap! I ran into the same issue. It sounds like only the web version crashes at the intersection of Asyncify and shutdown handlers which is weird. Well, let's at least add an e2e test for the web to confirm that Blueprint works on all PHP versions with and without the kitchen sink bundle. Also, it seems like this PR only rebuilds the kitchen sink bundle >= 8.1? |
Everything should be rebuilt now (66b0c97). I had some issues with the rebuild script and Docker, so I had to do it in three stages (light, kitchen-sink, node). |
bgrgicak
force-pushed
the
fix/1078-woo-asyncify-crash
branch
from
3f07c6e
to
005dc24
Compare
| let envelope: RequestMessage; | ||
| try { | ||
| // PHP-WASM sends messages as strings, so we can't expect valid JSON. | ||
| envelope = JSON.parse(message); | ||
| } catch (e) { | ||
| return ''; | ||
| } |
There was a problem hiding this comment.
@adamziel I ended up wrapping this in a try/catch. If we expect json, we shouldn't throw an error.
If there is a plain text message, it's not intended for this code.
Done 005dc24. I also confirmed it fails on trunk. |
| @@ -81,4 +81,21 @@ describe('Blueprints', () => { | |||
| .find('[aria-label="“Test post” (Edit)"]') | |||
| .should('exist'); | |||
| }); | |||
|
|
|||
| it('PHP Shutdown should work', () => { | |||
There was a problem hiding this comment.
Nitpick: In a follow-up PR let's add a comment to link this test to this PR and the associated issue in case anyone would wonder where did this test come from.
There was a problem hiding this comment.
I can do that. Wouldn't git blame be enough here? This is at least how I would search for it.
There was a problem hiding this comment.
It would be enough until the first time this file is reformatted or until the test is refactored or moved to another file. WordPress core also documents all tests with ticket numbers, I was sceptical at first but then I found it actually helpful over time.
Fixes #1078
What is this PR doing?
If fixes an Asyncify error and memory leak when running PHP shutdown functions.
What problem is it solving?
It adds full support for PHP shutdown functions and prevents crashes.
How is the problem addressed?
By adding Asyncify support for
wasm_sapi_request_shutdownand preventingfreefrom running if memory wasn't allocated.Testing Instructions
npm run dev