Skip to content

Conversation

kozer
Copy link
Contributor

@kozer kozer commented Aug 28, 2024

Motivation for the change, related issues

Calling the php.exit caused a hanging connection. This had affect in tests, and leaving open connections around.

Implementation details

Attach the HTTP server that we spawn to handle websocket connections to the PHP instance.
When we call .exit we use the attached instance to close the connections.

Testing Instructions (or ideally a Blueprint)

  1. Create a project with the following test code:
import { PHP, __private__dont__use, loadPHPRuntime } from '@php-wasm/universal'; const isWindows = process.platform === 'win32'; const dostuff = async function() {	const id = await loadPHPRuntime(	await getPHPLoaderModule('8.3'),	await withNetworking({})	);	const php = new PHP(id);	const privateSymbol = Object.getOwnPropertySymbols(php)	.find(sym => String(sym).includes('__private__dont__use'));	const result = await php.run({ code: `<?php echo "HI!"; `} );	console.log(result.text);	php.exit(); } dostuff().catch(console.error); 
  1. Build php-wasm/node and php-wasm/universal
npx nx run php-wasm-universal:build npx nx run php-wasm-node:build 
  1. Cd into /dist php-wasm folder and run npm link
  2. Go to the project and run npm link @php-wasm/node and npm link @php-wam/universal
  3. Navigate inside node module, in each package and run npm install
  4. Run the script you have created
  5. Ensure that exits successfully
@kozer kozer requested a review from adamziel August 28, 2024 15:14
Copy link
Contributor

@jeroenpf jeroenpf left a comment

Choose a reason for hiding this comment

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

I was able to make this work but we need to add the close() call as well.

I'll take care of this and add a test case tomorrow as well.

@kozer kozer requested a review from jeroenpf August 29, 2024 08:56
@kozer kozer self-assigned this Aug 29, 2024
@adamziel adamziel added [Type] Bug An existing feature does not function as intended [Package][@php-wasm] Node labels Aug 29, 2024
@adamziel
Copy link
Collaborator

Looking good, thank you so much! For posterity, once we have more of these cleanup callbacks, we may want to introduce something like PHPRuntime.beforeExit: Array<() => void>. For now, though, we're good.

@adamziel adamziel merged commit a0abc36 into WordPress:trunk Aug 29, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

[Package][@php-wasm] Node [Type] Bug An existing feature does not function as intended

3 participants