Add site server process

[fluiddot]

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]

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]

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

[fluiddot]

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.

[fluiddot]

This has been solved in 0f3e7db.

[fluiddot]

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]

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]

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

[kozer]

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

[fluiddot]

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

[fluiddot]

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

[katinthehatsite]

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]

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]

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]

[Suggestion]:

1. 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);
    }

[fluiddot]

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

[kozer]

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

[wojtekn]

@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]

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]

I noticed we weren't waiting for this promise.

[wojtekn]

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

[fluiddot]

@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]

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