Add site server process #19

fluiddot · 2024-04-24T10:14:01Z

Related to https://github.com/Automattic/dotcom-forge/issues/6423.

Proposed Changes

The primary objective of this PR is to decouple the process of running a site from the main thread. This aligns with Playground's concept of orchestrating site execution with an app in the browser. The Studio app faces similar challenges when running a site, where the PHP runtime can impede the main thread and consequently the overall app performance. Therefore, adopting the same approach by separating the process can address this issue.

Key differences between the approach presented here and the app include:

Instead of utilizing a service worker, we employ Express to listen to requests and proxy them to the PHP runtime.
Rather than using a web worker, we utilize Electron's utilityProcess to spawn a separate process. This is akin to Node's worker threads, but the process is managed by Chromium. Initially, I considered using worker threads as suggested in the Electron documentation. However, I encountered several challenges when loading and executing the worker due to the Typescript + Webpack setup. While that approach might be viable, I ultimately opted for utilityProcess due to its simplicity and similar functionality.

Important

Note that the PHP runtime is no longer directly accessible from the app; it is now only accessible by the site server process. However, we provide an API to facilitate running PHP requests.

Changes:

Introduced the site server process class, responsible for managing the server operations such as starting/stopping a site and executing PHP requests. It communicates with the separate process through messages.
Added the site server process child, which houses the functionality executed in a separate process. It listens to messages from its parent.
The getWpNowPath function is utilized in both the main and separate processes. In the latter, the Electron library isn't available, so we rely on process environment variables.
To initiate the separate process, the code needs to be bundled into a distinct file by webpack. The configuration has been adjusted accordingly.
Updated the site server class to integrate the new site server process.
The app now solely performs a PHP request to retrieve the current site's theme. The logic has been revamped to utilize the new API provided by the site server process.
During testing, I noticed a port mismatch issue. Both the app and wp-now attempt to find an open port when starting a site. The port selected by the app should take precedence; hence, the logic has been updated to ensure this.
Update the logging system to support forked processes.

Results

Main process performance:

Running the app on Windows

Before	After
studio-trunk.mp4	studio-separate-thread.mp4

App size

I conducted a comparison of the files generated when running npm run make:macos-arm64, which I assumed produces the app in production mode. The results indicate that with the changes from this PR, the binary size increases by 1.2 MB (+0.27%), and the zipped installer file grows by 0.4 MB (+0.23%). While this represents a minor increase, it's noteworthy. The primary contributor to this size increase is the generation of a separate file for the site server process child. In the future, it might be worthwhile to explore methods to reduce this size. I have a suspicion that the index.js file generated by webpack includes more code than necessary after relocating certain portions (particularly the wp-now code) to the site server process child.

Testing Instructions

Performance

Open the app on Windows.
Start multiple sites quickly.
Navigate through the app.
Observe that the UI responds without lag.

Create a site

Open the app.
Create a site.
Observe the site is created and is running.
Navigate to the site.
Observe the site's home is loaded properly.
Navigate to the site's WP Admin and some of its pages.
Observe that WP Admin's pages load properly.

Start a site

Open the app.
Start a previously created site.
Observe the site is running.
Navigate to the site.
Observe the site's home is loaded properly.
Navigate to the site's WP Admin and some of its pages.
Observe that WP Admin's pages load properly.

Stop a site (UPDATE)

Check in the OS the running processes for the Studio app. NOTE: When using a local build, the name of the process will be Electron Helper.
Open the app.
Start a previously created site.
Observe the site is running.
Check the running processes and observe a new one has been created.
Stop the site.
Check the running processes and observe the new process was killed.

Logging in the forked process

Run the command npm run make to generate a build in production mode.
Install and open the app.
Locate the logs folder (reference). E.g. in macOS it's ~/Library/Logs/Studio.
Open the current log file (e.g. current.log).
Start a site.
Observe in the logs that entries with prefixes [main] and [site-server-process] are registered.

Pre-merge Checklist

Have you checked for TypeScript, React or other console errors?

fluiddot · 2024-04-24T10:31:14Z

src/lib/php-get-theme-details.ts

+		themeDetails = await server.runPhp( {
+			code: themeDetailsPhp,
+		} );


runPhp always returns the PHP response as a string:

studio/src/lib/site-server-process-child.ts

Line 32 in 9b54b8c

process.parentPort.postMessage( { message, messageId, data: response.text } );

fluiddot · 2024-04-24T10:33:12Z

webpack.plugins.ts

 import type IForkTsCheckerWebpackPlugin from 'fork-ts-checker-webpack-plugin';
 import type { WebpackPluginInstance } from 'webpack';
 const ForkTsCheckerWebpackPlugin: typeof IForkTsCheckerWebpackPlugin = require( 'fork-ts-checker-webpack-plugin' ); // eslint-disable-line @typescript-eslint/no-var-requires

+const siteServerProcessModulePath = path.resolve( __dirname, '.webpack/main/siteServerProcess.js' );


Not sure if there's a more elegant way to infer the resulting output file.

I noticed this path is wrong when building the app in production mode. I'll work on a fix before setting the PR ready to review.

This has been solved in 0f3e7db.

fluiddot · 2024-04-24T10:36:04Z

vendor/wp-now/src/get-wp-now-path.ts

+			const appDataPath = process.env.STUDIO_APP_DATA_PATH;
+			return path.join( appDataPath, process.env.STUDIO_APP_NAME, 'server-files' );


This logic is the same as the one provided in getServerFilesPath but using the values provided via the process' environment variables:

studio/src/storage/paths.ts

Lines 9 to 12 in 9b54b8c

export function getServerFilesPath(): string {

const appDataPath = app.getPath( 'appData' ); // Resolves to ~/Library/Application Support on macOS

return path.join( appDataPath, app.getName(), 'server-files' );

}

fluiddot · 2024-04-24T10:38:42Z

src/lib/site-server-process.ts

+		return messageId;
+	}
+
+	async waitForMessage< T = undefined >(


The messaging logic is mostly based on the one used in Playground's web service worker:
https://github.com/WordPress/playground/blob/trunk/packages/php-wasm/web-service-worker/src/messaging.ts

fluiddot · 2024-04-24T10:39:17Z

src/lib/site-server-process.ts

+// whether you're running in development or production).
+declare const SITE_SERVER_PROCESS_MODULE_PATH: string;
+
+export type MessageName = 'start-server' | 'stop-server' | 'run-php';


If we need to expose more functionality from the server or the PHP runtime, we could introduce it in the future.

See my suggestion above. I suggest having an object with the events, to avoid typos and magic strings.

fluiddot · 2024-04-24T17:16:03Z

src/logging.ts

+	if ( ! isForkedProcess ) {
+		// Set the logging path to the default for the platform.
+		app.setAppLogsPath();
+	}


We only permit configuring the app logs path in the main process since the Electron library is only available there.

fluiddot · 2024-04-25T14:18:39Z

The failure in the Build Installer jobs is unrelated to the changes in this PR. It will be fixed in #31.

katinthehatsite · 2024-04-25T14:53:25Z

I have not tested this yet and I will take a closer look tomorrow but I just wanted to say that I really appreciate how detailed your description and explanation is. It is the first time I am working on an Electron app and learning about the infrastructure and architecture of it on the go and it is very helpful to have everything outlined so clearly ❤️

fluiddot · 2024-04-25T15:32:19Z

I have not tested this yet and I will take a closer look tomorrow but I just wanted to say that I really appreciate how detailed your description and explanation is. It is the first time I am working on an Electron app and learning about the infrastructure and architecture of it on the go and it is very helpful to have everything outlined so clearly ❤️

Thank you @katinthehatsite 🙂 ! The PR represents a substantial shift in how sites are executed, so I wanted to provide clear context on both the rationale behind it and its implementation.

fluiddot · 2024-04-26T08:16:21Z

Note

I'd like to cover the refactor and new site server process with unit/e2e tests. However, I'm thinking of doing it in a separate PR to avoid blocking this PR.

kozer

Amazing work @fluiddot !!!

I left some minor comments / suggestions.
Other than that, this works as expected!!

kozer · 2024-05-01T11:13:03Z

src/lib/site-server-process-child.ts

+	const { message, messageId, data }: { message: MessageName; messageId: number; data: unknown } =
+		messagePayload;
+	try {
+		switch ( message ) {


[Suggestion]:

Extract messages in order to avoid magic strings; eg:

const messages = { START_SERVER: 'start-server' ...

or
2. Extract messages along with their associated functions; Eg:

const messageHandlers = { 'start-server': start, 'stop-server': stop, 'run-php': runPhp }; .... try { const handler = messageHandlers[message]; if (!handler) { return } await handler(message, messageId, data); } catch (error) { const errorObj = error instanceof Error ? error : new Error('Unknown Error'); postMessage(errorObj); }

We can further decouple by extracting the creating a emitMessage function like so:

const emitMessage = (error) => { const response = { message, messageId }; if (error) { response['error'] = error; } process.parentPort.postMessage(response); };

and remove it from the handlers like so:

async function runPhp( message: string, messageId: number, data: PHPRunOptions ) { await server.php.run( data ); }

and use it like so:

const emitMessage = (error = null) => { const response = { message, messageId }; if (error) { response['error'] = error; } process.parentPort.postMessage(response); }; try { const handler = messageHandlers[message]; if (!handler) { return } await handler(message, messageId, data); emitMessage(); } catch (error) { const errorObj = error as Error; if ( ! errorObj ) { emitMessage( Error( 'Unknown Error' ) ); return; } emitMessage( errorObj); }

@kozer following your suggestion, I updated this file in 3566662. Let me know if it's aligned with the changes you proposed, thanks 🙇 !

kozer · 2024-05-01T11:13:54Z

src/lib/site-server-process.ts

+// whether you're running in development or production).
+declare const SITE_SERVER_PROCESS_MODULE_PATH: string;
+
+export type MessageName = 'start-server' | 'stop-server' | 'run-php';


See my suggestion above. I suggest having an object with the events, to avoid typos and magic strings.

wojtekn · 2024-05-07T07:45:38Z

@fluiddot I tested it on macOS, and test cases worked as expected: I was able to create a site and start multiple servers, and I noticed a separate process tag in the log file.

One thing I noticed is that macOS spawns those processes correctly, but when I stopped all sites, those processes were still lingering there. Is it expected?

fluiddot · 2024-05-07T10:11:51Z

One thing I noticed is that macOS spawns those processes correctly, but when I stopped all sites, those processes were still lingering there. Is it expected?

Good catch @wojtekn. Seems we should manually kill the processes once the server stops. I've pushed a fix in 8e17947.

kill-processes-upon-server-stop.mp4

fluiddot · 2024-05-07T10:17:08Z

src/site-server.ts

@@ -111,7 +113,7 @@ export class SiteServer {

 	async stop() {
 		console.log( 'Stopping server with ID', this.details.id );
-		this.server?.stopServer();
+		await this.server?.stop();


I noticed we weren't waiting for this promise.

wojtekn

Thanks @fluiddot , it works as expected now - processes are killed.

fluiddot · 2024-05-08T07:59:31Z

@kozer I asked for another review just to confirm that the latest changes are aligned with the suggestion you shared here. Since you already approved it as well as @wojtekn, I'm going to proceed with the merge. In case there are any further suggestions, I'll be happy to apply them in a separate PR. Thanks 🙇 !

kozer · 2024-05-08T08:25:58Z

@fluiddot , I was afk and just saw your comment! Yes, those changes seem good! Thanks for implementing that!

fluiddot added 6 commits April 24, 2024 10:57

Add site server process

f144692

Update getWpNowPath to support site server process

d37aecc

Update webpack configuration to generate site server process bundle

196c599

Use server process in the app

e52bc0e

Update phpGetThemeDetails to use server process

1de5f35

Ensure server uses port defined by the app

9b54b8c

fluiddot self-assigned this Apr 24, 2024

fluiddot commented Apr 24, 2024

View reviewed changes

fluiddot added 9 commits April 24, 2024 17:05

Extend logging to support forked processes

fc7b220

Setup logging for site server process

f69e139

Improve process exit error message

e3740cd

Update webpack main configuration to support extra entries

0f3e7db

Merge branch 'trunk' into add/site-server-process

713cb65

Fix error case when waiting for a message response

12509f3

Rename wait for response function

4a2969c

Rename message payload in site server process child

fb09b9a

Update inline comments in main webpack configuration

43846c7

fluiddot commented Apr 25, 2024

View reviewed changes

fluiddot marked this pull request as ready for review April 25, 2024 14:18

fluiddot requested a review from a team April 25, 2024 14:18

Merge branch 'trunk' into add/site-server-process

3f7c463

fluiddot added the [Type] Enhancement Improvement upon an existing feature label Apr 30, 2024

kozer approved these changes May 1, 2024

View reviewed changes

Use handlers approach in server child process

3566662

fluiddot requested review from a team and kozer May 6, 2024 11:48

fluiddot added 2 commits May 7, 2024 12:06

Kill process upon server stop

8e17947

Wait for server to stop

9909cf5

fluiddot commented May 7, 2024

View reviewed changes

wojtekn approved these changes May 8, 2024

View reviewed changes

fluiddot merged commit 2d37b41 into trunk May 8, 2024
7 checks passed

fluiddot deleted the add/site-server-process branch May 8, 2024 08:00

dcalhoun mentioned this pull request May 17, 2024

Studio: Fix stop server error #144

Merged

1 task

fluiddot mentioned this pull request Jun 19, 2024

Improve performance when calculating chat context initial load #272

Merged

1 task

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add site server process #19

Add site server process #19

fluiddot commented Apr 24, 2024 •

edited

Loading

fluiddot Apr 24, 2024

fluiddot Apr 24, 2024

fluiddot Apr 24, 2024

fluiddot Apr 24, 2024

fluiddot Apr 24, 2024

fluiddot Apr 24, 2024

fluiddot Apr 24, 2024

kozer May 1, 2024

fluiddot Apr 24, 2024

fluiddot commented Apr 25, 2024

katinthehatsite commented Apr 25, 2024 •

edited

Loading

fluiddot commented Apr 25, 2024

fluiddot commented Apr 26, 2024

kozer left a comment

kozer May 1, 2024

fluiddot May 6, 2024

kozer May 1, 2024

wojtekn commented May 7, 2024

fluiddot commented May 7, 2024

fluiddot May 7, 2024

wojtekn left a comment

fluiddot commented May 8, 2024

kozer commented May 8, 2024

		const appDataPath = process.env.STUDIO_APP_DATA_PATH;
		return path.join( appDataPath, process.env.STUDIO_APP_NAME, 'server-files' );

	export function getServerFilesPath(): string {
	const appDataPath = app.getPath( 'appData' ); // Resolves to ~/Library/Application Support on macOS
	return path.join( appDataPath, app.getName(), 'server-files' );
	}

Add site server process #19

Add site server process #19

Conversation

fluiddot commented Apr 24, 2024 • edited Loading

Proposed Changes

Results

Main process performance:

Running the app on Windows

App size

Testing Instructions

Performance

Create a site

Start a site

Stop a site (UPDATE)

Logging in the forked process

Pre-merge Checklist

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

fluiddot commented Apr 25, 2024

katinthehatsite commented Apr 25, 2024 • edited Loading

fluiddot commented Apr 25, 2024

fluiddot commented Apr 26, 2024

kozer left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

wojtekn commented May 7, 2024

fluiddot commented May 7, 2024

Choose a reason for hiding this comment

wojtekn left a comment

Choose a reason for hiding this comment

fluiddot commented May 8, 2024

kozer commented May 8, 2024

fluiddot commented Apr 24, 2024 •

edited

Loading

katinthehatsite commented Apr 25, 2024 •

edited

Loading