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

Fix: Exit http server on php exit #1714

Merged
merged 7 commits into from
Aug 29, 2024

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.

packages/php-wasm/universal/src/lib/load-php-runtime.ts Outdated Show resolved Hide resolved
@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
5 checks passed
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
Projects
Archived in project
Development

Successfully merging this pull request may close these issues.

3 participants