Skip to content

Commit

Permalink
Fix: Playground not starting due to a race condition (#1084)
Browse files Browse the repository at this point in the history
Without this PR, the isConnected() method tries for two seconds before
assuming the connection worked out. However, sometimes it takes more
than that to reach the remote frame or context. This PR replaces that
with an infinite loop that will keep trying until the remote end
actually responds.

This solves a problem with the progress bar stuck at 0% or 50%.

 ## Testing instructions

Apply the following patch to artificially and synchronously slow down
the boot in all contexts (website, remote, worker)

```patch
diff --git a/packages/php-wasm/web/src/lib/web-php.ts b/packages/php-wasm/web/src/lib/web-php.ts
index 27b976a23..e4f9071b5 100644
--- a/packages/php-wasm/web/src/lib/web-php.ts
+++ b/packages/php-wasm/web/src/lib/web-php.ts
@@ -44,6 +44,10 @@ const fakeWebsocket = () => {
        };
 };

+for (let i = 0; i < 5000000; i++) {
+       crypto.randomUUID();
+}
+
 export class WebPHP extends BasePHP {
        /**
         * Creates a new PHP instance.
```

Then open the local Playground at http://localhost:5400/website-server/
and confirm it starts loading after a few seconds.

Repeat a dozen or so times and confirm Playground reliably loads each
time.

Then do the same without this PR and confirm the progress bar tends to
get stuck.

I'd love to ship an E2E test with this, but I can't think of a way to
build one.
  • Loading branch information
adamziel authored Mar 5, 2024
1 parent f9379b3 commit c0df8b5
Showing 1 changed file with 7 additions and 7 deletions.
14 changes: 7 additions & 7 deletions packages/php-wasm/web/src/lib/api.ts
Original file line number Diff line number Diff line change
Expand Up @@ -38,17 +38,17 @@ export function consumeAPI<APIType>(
get: (target, prop) => {
if (prop === 'isConnected') {
return async () => {
/*
* If exposeAPI() is called after this function,
* the isConnected() call will hang forever. Let's
* retry it a few times.
*/
for (let i = 0; i < 10; i++) {
// Keep retrying until the remote API confirms it's connected.
while (true) {
try {
await runWithTimeout(api.isConnected(), 200);
break;
} catch (e) {
// Timeout exceeded, try again
// Timeout exceeded, try again. We can't just use a single
// `runWithTimeout` call because it won't reach the remote API
// if it's not connected yet. Instead, we need to keep retrying
// until the remote API is connected and registers a handler
// for the `isConnected` method.
}
}
};
Expand Down

0 comments on commit c0df8b5

Please sign in to comment.