Skip to content

[Web][Networking] Call setupFetchNetworkTransport() in the web worker, not in remote.html - #2286

Merged
adamziel merged 3 commits into
trunkfrom
setup-fetch-network-transport-in-worker-thread-not-in-remote
Jun 19, 2025
Merged

adamziel merged 3 commits into
trunkfrom
setup-fetch-network-transport-in-worker-thread-not-in-remote

Conversation

@adamziel

@adamziel adamziel commented Jun 18, 2025 •

Copy link
Copy Markdown
Collaborator

This PR enables running simultaneous network calls asynchronously handled by multiple co-existing PHP instances within the same worker.

The gist of the problem can be seen here:

const mgr = new PHPProcessManager({
	phpFactory: async () =>
		new PHP(await loadNodeRuntime(RecommendedPHPVersion)),
	maxPhpInstances: 3,
});

const primaryPhp = await mgr.getPrimaryPhp();

// We setup a JavaScript handler for WP_HTTP_Fetch by listening
// to its post_message_to_js() call via primaryPHP.onMessage()...
setupFetchNetworkTransport(primaryPhp);

// ...but that does nothing for the next spawned PHP instance. When
//    it calls post_message_to_js(), there's no message handler to
//    translate that into fetch(). As a result, the response comes
//    back empty.
const { php } = await mgr.acquirePHPInstance({ considerPrimary: false });
php.run({ code: "<?php wp_safe_remote_get('https://w.org'); " })

Solution

We bind the .onMessage handler to every PHP instance created by the PHPProcessManager. As a side-effect, we also:

  • Added onPHPInstanceCreated callback to bootWordPress()
  • Moved the setupFetchNetworkTransport call from boot-playground-remote.ts to the web worker where PHP is spawned. This should make it marginally faster. Before this PR, the handler was one worker away.

Testing instructions

  1. Run this Blueprint: http://127.0.0.1:5400/website-server/?blueprint-url=https%3A%2F%2Fraw.githubusercontent.com%2Fautomattic%2Fremote-data-blocks-demo%2Ftrunk%2Fshopify%2Fshopify-blueprint.json
  2. Click "Edit page"
  3. Confirm the four requests to /wp-json/remote-data-blocks/v1/remote-data have actual data in their response body. Specifically, they should NOT have a JSON-encoded error message.

…, not in remote.html

This PR enables running simultaneous network calls asynchronously handled by multiple co-existing PHP
instances within the same worker.

The gist of the problem can be seen here:

```ts
const mgr = new PHPProcessManager({
	phpFactory: async () =>
		new PHP(await loadNodeRuntime(RecommendedPHPVersion)),
	maxPhpInstances: 3,
});

const primaryPhp = await mgr.getPrimaryPhp();

// We setup a JavaScript handler for WP_HTTP_Fetch by listening
// to its post_message_to_js() call via primaryPHP.onMessage()...
setupFetchNetworkTransport(primaryPhp);

// ...but that does nothing for the next spawned PHP instance. When
//    it calls post_message_to_js(), there's no message handler to
//    translate that into fetch(). As a result, the response comes
//    back empty.
const { php } = await mgr.acquirePHPInstance({ considerPrimary: false });
php.run({ code: "<?php wp_safe_remote_get('https://w.org'); " })
```

 ## Remaining work

A unit test or two would be nice.

 ## Testing instructions

1. Run this Blueprint: http://127.0.0.1:5400/website-server/?blueprint-url=https%3A%2F%2Fraw.githubusercontent.com%2Fautomattic%2Fremote-data-blocks-demo%2Ftrunk%2Fshopify%2Fshopify-blueprint.json
2. Click "Edit page"
3. Confirm the four requests to /wp-json/remote-data-blocks/v1/remote-data have actual data in their response body. Specifically, they should NOT have a JSON-encoded error message.
@adamziel adamziel added [Type] Bug An existing feature does not function as intended [Aspect] Networking labels Jun 18, 2025
@adamziel
adamziel requested a review from brandonpayton June 18, 2025 13:39

@brandonpayton brandonpayton left a comment •

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

I have not tested this, but it looks good to me.

Note: I think this is a web-only concern because AFAIK we do not have to set up a fetch transport for php-wasm/node. So I don't think we need to worry about the multi-worker scenario until we have multiple php-wasm/web workers, and even then, the fact that it is handled in the worker thread means it the fetch network transport should just work if we create more than one thread.

Thanks, @adamziel!

@adamziel

Copy link
Copy Markdown
Collaborator Author

Thank you for reviewing @brandonpayton!

I've explored a test but it was quite tedious so I went for an inline comment instead to communicate the necessary context to future developers:

5f2299e (#2286)

@adamziel
adamziel merged commit 1164ae1 into trunk Jun 19, 2025
@adamziel
adamziel deleted the setup-fetch-network-transport-in-worker-thread-not-in-remote branch June 19, 2025 09:31
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

[Aspect] Networking [Type] Bug An existing feature does not function as intended

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants