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

Report errors encountered during bootPlaygroundRemote() #570

Merged
merged 14 commits into from
Jun 22, 2023
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
44 changes: 30 additions & 14 deletions packages/php-wasm/universal/src/lib/load-php-runtime.ts
Original file line number Diff line number Diff line change
Expand Up @@ -121,17 +121,15 @@ export async function loadPHPRuntime(
phpModuleArgs: EmscriptenOptions = {},
dataDependenciesModules: DataModule[] = []
): Promise<number> {
let resolvePhpReady: any, resolveDepsReady: any;
const depsReady = new Promise((resolve) => {
resolveDepsReady = resolve;
});
const phpReady = new Promise((resolve) => {
resolvePhpReady = resolve;
});
const [phpReady, resolvePHP, rejectPHP] = makePromise();
const [depsReady, resolveDeps] = makePromise();

const PHPRuntime = phpLoaderModule.init(currentJsRuntime, {
onAbort(reason) {
console.error('WASM aborted: ');
rejectPHP(reason);
resolveDeps();
// This can happen after PHP has been initialized so
// let's just log it.
console.error(reason);
},
ENV: {},
Expand All @@ -145,20 +143,24 @@ export async function loadPHPRuntime(
if (phpModuleArgs.onRuntimeInitialized) {
phpModuleArgs.onRuntimeInitialized();
}
resolvePhpReady();
resolvePHP();
},
monitorRunDependencies(nbLeft) {
if (nbLeft === 0) {
delete PHPRuntime.monitorRunDependencies;
resolveDepsReady();
resolveDeps();
}
},
});
for (const { default: loadDataModule } of dataDependenciesModules) {
loadDataModule(PHPRuntime);
}

await Promise.all(
dataDependenciesModules.map(({ default: dataModule }) =>
dataModule(PHPRuntime)
)
);
Copy link
Member

Choose a reason for hiding this comment

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

before we have been running these and moving on possibly before they finish, but now with Promise.all we are pausing until they all finish. is that intentional?

also, we are using Promise.all so if a single data dependency throws an error the entire thing collapses.

  • do we want to use Promise.allSettled so we can report failures for specific dependencies?
  • if not, do we want to report the failure of any data dependency? because we're not wrapping this in a try/catch or adding any catch handler to any of the promises.

Copy link
Collaborator Author

@adamziel adamziel Jun 21, 2023

Choose a reason for hiding this comment

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

Before we've waited for data dependencies, too, in one of the next lines: await depsReady;. That line is still there, in this PR, but maybe we don't need it anymore.

do we want to use Promise.allSettled so we can report failures for specific dependencies?

We could, but that would amount to the same outcome – display a loading error. Or would you like to do something more granular with that information?

do we want to report the failure of any data dependency?

That was my intention – try to download and start PHP and, if anything at all fails, reject the promise.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

No, we still need await depsReady; because it's resolved after the data dependencies have been both loaded and processed

Copy link
Member

Choose a reason for hiding this comment

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

We could, but that would amount to the same outcome – display a loading error. Or would you like to do something more granular with that information?

the point was that depsReady is all or nothing and .allSettled can tell us which dependencies failed. so if our goal is to expose more errors it could be beneficial to say something like "dependency {name} from {URL} failed to download" and be able to do that for each one that fails.

Copy link
Collaborator Author

@adamziel adamziel Jun 22, 2023

Choose a reason for hiding this comment

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

Ah, I see what you mean! The information about the URL is included in the error message thrown by the Emscripten module. The ability to display multiple errors would still be nice, we’d just need to handle an array of error messages up the call stack.

Worth noting: data dependencies are produced by file_packager.py and we only load a single data dependency now: wp.data.


if (!dataDependenciesModules.length) {
resolveDepsReady();
resolveDeps();
}

await depsReady;
Expand Down Expand Up @@ -195,6 +197,20 @@ export const currentJsRuntime = (function () {
}
})();

/**
* Creates and exposes Promise resolve/reject methods for later use.
*/
const makePromise = () => {
const methods: any = [];

const promise = new Promise((resolve, reject) => {
methods.push(resolve, reject);
});
methods.unshift(promise);

return methods as [Promise<any>, (v?: any) => void, (e?: any) => void];
};

export type PHPRuntime = any;

export type PHPLoaderModule = {
Expand Down
8 changes: 5 additions & 3 deletions packages/php-wasm/web/src/lib/api.ts
Original file line number Diff line number Diff line change
Expand Up @@ -74,14 +74,16 @@ export type PublicAPI<Methods, PipedAPI = unknown> = RemoteAPI<
export function exposeAPI<Methods, PipedAPI>(
apiMethods?: Methods,
pipedApi?: PipedAPI
): [() => void, PublicAPI<Methods, PipedAPI>] {
): [() => void, (e: Error) => void, PublicAPI<Methods, PipedAPI>] {
setupTransferHandlers();

const connected = Promise.resolve();

let setReady: any;
const ready = new Promise((resolve) => {
let setFailed: any;
const ready = new Promise((resolve, reject) => {
setReady = resolve;
setFailed = reject;
});

const methods = proxyClone(apiMethods);
Expand All @@ -104,7 +106,7 @@ export function exposeAPI<Methods, PipedAPI>(
? Comlink.windowEndpoint(self.parent)
: undefined
);
return [setReady, exposedApi];
return [setReady, setFailed, exposedApi];
}

let isTransferHandlersSetup = false;
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -10,7 +10,27 @@ export async function spawnPHPWorkerThread(
startupOptions: Record<string, string> = {}
) {
workerUrl = addQueryParams(workerUrl, startupOptions);
return new Worker(workerUrl, { type: 'module' });
const worker = new Worker(workerUrl, { type: 'module' });
return new Promise<Worker>((resolve, reject) => {
worker.onerror = (e) => {
const error = new Error(
`WebWorker failed to load at ${workerUrl}. ${
e.message ? `Original error: ${e.message}` : ''
}`
);
(error as any).filename = e.filename;
reject(error);
};
// There is no way to know when the worker script has started
// executing, so we use a message to signal that.
function onStartup(event: { data: string }) {
if (event.data === 'worker-script-started') {
resolve(worker);
worker.removeEventListener('message', onStartup);
}
}
worker.addEventListener('message', onStartup);
});
}

function addQueryParams(
Expand Down
4 changes: 4 additions & 0 deletions packages/playground/compile-wordpress/Dockerfile
Original file line number Diff line number Diff line change
Expand Up @@ -195,6 +195,10 @@ COPY ./build-assets/esm-prefix.js ./build-assets/esm-suffix.js /root/
# This tells web browsers it's a new file and they should reload it.
RUN cat /root/output/wp.js \
| sed -E "s#'[^']*wp\.data'#dependencyFilename#g" \
| sed -E 's#xhr.onerror = #xhr.onerror = onLoadingFailed; const z = #g' \
| sed -E 's#throw new Error\(xhr.+$#onLoadingFailed(event);#g' \
| sed -E 's#runWithFS#runWithFSThenResolve#g' \
| sed -E 's#function runWithFSThenResolve#function runWithFSThenResolve() { runWithFS(); resolve(); }; function runWithFS#g' \
> /tmp/wp.js && \
mv /tmp/wp.js /root/output/wp.js;

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -13,4 +13,10 @@ export const defaultThemeName = WP_THEME_NAME;
// into an ESM module.
// This replaces the Emscripten's MODULARIZE=1 which pollutes the
// global namespace and does not play well with import() mechanics.
export default function(PHPModule) {
export default function (PHPModule) {
return new Promise(function(resolve, reject) {
function onLoadingFailed(error) {
const wrappingError = new Error(`Failed to load data dependency module "${dependencyFilename}"${typeof error === 'string' ? ` (${error})` : ''}`);
wrappingError.cause = error instanceof Error ? error : null;
reject(wrappingError);
};
Original file line number Diff line number Diff line change
@@ -1,2 +1,3 @@
// See esm-prefix.js
});
}
51 changes: 50 additions & 1 deletion packages/playground/remote/remote.html
Original file line number Diff line number Diff line change
Expand Up @@ -21,6 +21,27 @@
body {
overflow: hidden;
}

body.has-error {
background: #f1f1f1;
display: flex;
flex-direction: column;
align-items: center;
justify-content: center;
font-size: 20px;
font-family: Arial, Helvetica, sans-serif;
line-height: 1.4;
}
body.has-error .error-message {
padding: 20px;
max-width: 800px;
}
body.has-error button {
margin-top: 15px;
font-size: 20px;
padding: 5px 10px;
cursor: pointer;
}
</style>
</head>
<body class="is-loading">
Expand All @@ -33,7 +54,35 @@
document.body.classList.add('is-embedded');
}
import { bootPlaygroundRemote } from './src/index';
bootPlaygroundRemote();
try {
await bootPlaygroundRemote();
} catch (e) {
document.body.className = 'has-error';
document.body.innerHTML = '';

const div = document.createElement('div');
div.className = 'error-message';
div.innerText = 'Ooops! WordPress Playground had a hiccup!';
if (
e?.message?.includes(
'The user denied permission to use Service Worker'
)
) {
div.innerText =
'It looks like you have disabled third-party cookies in your browser. This tends to ' +
'also disable the ServiceWorker API used by WordPress Playground. Please re-enable ' +
'third-party cookies and try again.';
}

document.body.append(div);

const button = document.createElement('button');
button.innerText = 'Try again';
button.onclick = () => {
window.location.reload();
};
document.body.append(button);
}
</script>
</body>
</html>
25 changes: 15 additions & 10 deletions packages/playground/remote/src/lib/boot-playground-remote.ts
Original file line number Diff line number Diff line change
Expand Up @@ -145,18 +145,23 @@ export async function bootPlaygroundRemote() {
// https://github.com/GoogleChromeLabs/comlink/issues/426#issuecomment-578401454
// @TODO: Handle the callback conversion automatically and don't explicitly re-expose
// the onDownloadProgress method
const [setAPIReady, playground] = exposeAPI(webApi, workerApi);
const [setAPIReady, setAPIError, playground] = exposeAPI(webApi, workerApi);

await workerApi.isReady();
await registerServiceWorker(
workerApi,
await workerApi.scope,
serviceWorkerUrl + ''
);
wpFrame.src = await playground.pathToInternalUrl('/');
setupPostMessageRelay(wpFrame, getOrigin(await playground.absoluteUrl));
try {
await workerApi.isReady();
await registerServiceWorker(
workerApi,
await workerApi.scope,
serviceWorkerUrl + ''
);
wpFrame.src = await playground.pathToInternalUrl('/');
setupPostMessageRelay(wpFrame, getOrigin(await playground.absoluteUrl));

setAPIReady();
setAPIReady();
} catch (e) {
setAPIError(e as Error);
throw e;
}

/*
* An asssertion to make sure Playground Client is compatible
Expand Down
67 changes: 37 additions & 30 deletions packages/playground/remote/src/lib/worker-thread.ts
Original file line number Diff line number Diff line change
Expand Up @@ -27,6 +27,9 @@ import {
import { applyWordPressPatches } from '@wp-playground/blueprints';
import { journalMemfsToOpfs } from './opfs/journal-memfs-to-opfs';

// post message to parent
self.postMessage('worker-script-started');

const startupOptions = parseWorkerStartupOptions<{
wpVersion?: string;
phpVersion?: string;
Expand Down Expand Up @@ -127,42 +130,46 @@ export class PlaygroundWorkerEndpoint extends WebPHPEndpoint {
}
}

const [setApiReady] = exposeAPI(
const [setApiReady, setAPIError] = exposeAPI(
new PlaygroundWorkerEndpoint(php, monitor, scope, wpVersion, phpVersion)
);
try {
await phpReady;

if (!useOpfs || !wordPressAvailableInOPFS) {
/**
* When WordPress is restored from OPFS, these patches are already applied.
* Thus, let's not apply them again.
*/
await wordPressModule;
applyWebWordPressPatches(php);
await applyWordPressPatches(php, {
wordpressPath: DOCROOT,
patchSecrets: true,
disableWpNewBlogNotification: true,
addPhpInfo: true,
disableSiteHealth: true,
});
}

await phpReady;
if (useOpfs) {
if (wordPressAvailableInOPFS) {
await copyOpfsToMemfs(php, opfsDir!, DOCROOT);
} else {
await copyMemfsToOpfs(php, opfsDir!, DOCROOT);
}

if (!useOpfs || !wordPressAvailableInOPFS) {
/**
* When WordPress is restored from OPFS, these patches are already applied.
* Thus, let's not apply them again.
*/
await wordPressModule;
applyWebWordPressPatches(php);
journalMemfsToOpfs(php, opfsDir!, DOCROOT);
}

// Always setup the current site URL.
await applyWordPressPatches(php, {
wordpressPath: DOCROOT,
patchSecrets: true,
disableWpNewBlogNotification: true,
addPhpInfo: true,
disableSiteHealth: true,
siteUrl: scopedSiteUrl,
});
}

if (useOpfs) {
if (wordPressAvailableInOPFS) {
await copyOpfsToMemfs(php, opfsDir!, DOCROOT);
} else {
await copyMemfsToOpfs(php, opfsDir!, DOCROOT);
}

journalMemfsToOpfs(php, opfsDir!, DOCROOT);
setApiReady();
} catch (e) {
setAPIError(e as Error);
throw e;
}

// Always setup the current site URL.
await applyWordPressPatches(php, {
wordpressPath: DOCROOT,
siteUrl: scopedSiteUrl,
});

setApiReady();