Skip to content

Commit

Permalink
BasePHP.#handleRequest(): Replace double "await await" with .then()
Browse files Browse the repository at this point in the history
Before this PR, #handleRequest used the following construction:

new Promise(async (resolve, reject) => {
    try {
        await await ccall();
        resolve();
    } catch(e) {
        reject(e);
    }
});

The double await was in there due to a weird issue found earlier on,
where the singular `await` still resolved to a promise.

However, a promise with an async function and a double await, is
a confusing beast.

The Promise A+ specification claims that a promise never resolves to
another promise. It always resolves to the final value:

https://promisesaplus.com/

Every attempt to reproduce the original issue have failed. Perhaps it
was a browser bug or gamma rays, who know? The important thing is: if
ccall() returns a promise, it is now resolved to its final value.

Therefore, this commit removes the async/await await construction, and
replaces it with a simpler one relying on .then().

Importantly, ccall() may return either a promise (when Asyncify is
involved, e.g. for network calls) or a primitive value. This commit
takes special care to handle both use-cases, and provides tests to
ensure that both synchronous and asynchronous exceptions are correctly
handled.

Authored by: @kozer
Co-authored by: @zieladam
  • Loading branch information
adamziel committed May 18, 2023
1 parent ed51d9a commit 1a31776
Show file tree
Hide file tree
Showing 2 changed files with 97 additions and 20 deletions.
88 changes: 87 additions & 1 deletion packages/php-wasm/node/src/test/php.spec.ts
Original file line number Diff line number Diff line change
@@ -1,5 +1,9 @@
import { getPHPLoaderModule, NodePHP } from '..';
import { loadPHPRuntime, SupportedPHPVersions } from '@php-wasm/universal';
import {
loadPHPRuntime,
SupportedPHPVersions,
__private__dont__use,
} from '@php-wasm/universal';
import { existsSync, rmSync, readFileSync } from 'fs';

const testDirPath = '/__test987654321';
Expand Down Expand Up @@ -554,3 +558,85 @@ bar1
});
});
});

// @TODO Prevent crash on PHP versions 5.6, 7.2, 8.2
describe.each(['7.0', '7.1', '7.3', '7.4', '8.0', '8.1'])(
'PHP %s – process crash',
(phpVersion) => {
let php: NodePHP;
beforeEach(async () => {
php = await NodePHP.load(phpVersion as any);
php.setPhpIniEntry('allow_url_fopen', '1');
vi.restoreAllMocks();
});

it('Does not crash due to an unhandled Asyncify error ', async () => {
let caughtError;
try {
/**
* PHP is intentionally built without network support for __clone()
* because it's an extremely unlikely place for any network activity
* and not supporting it allows us to test the error handling here.
*
* `clone $x` will throw an asynchronous error out when attempting
* to do a network call ("unreachable" WASM instruction executed).
* This test should gracefully catch and handle that error.
*
* A failure to do so will crash the entire process
*/
await php.run({
code: `<?php
class Top {
function __clone() {
file_get_contents("http://127.0.0.1");
}
}
$x = new Top();
clone $x;
`,
});
} catch (error: unknown) {
caughtError = error;
if (error instanceof Error) {
expect(error.message).toMatch(
/Aborted|Program terminated with exit\(1\)|/
);
}
}
if (!caughtError) {
expect.fail('php.run should have thrown an error');
}
});

it('Does not crash due to an unhandled non promise error ', async () => {
let caughtError;
try {
const spy = vi.spyOn(php[__private__dont__use], 'ccall');
expect(spy.getMockName()).toEqual('ccall');
spy.mockImplementation((c_func) => {
if (c_func === 'wasm_sapi_handle_request') {
throw new Error('test');
}
});

await php.run({
code: `<?php
function top() {
file_get_contents("http://127.0.0.1");
}
top();
`,
});
} catch (error: unknown) {
caughtError = error;
if (error instanceof Error) {
expect(error.message).toMatch('test');
expect(error.stack).toContain('#handleRequest');
}
}
if (!caughtError) {
expect.fail('php.run should have thrown an error');
}
});
}
);
29 changes: 10 additions & 19 deletions packages/php-wasm/universal/src/lib/base-php.ts
Original file line number Diff line number Diff line change
Expand Up @@ -385,7 +385,7 @@ export abstract class BasePHP implements IsomorphicLocalPHP {
let errorListener: any;
try {
// eslint-disable-next-line no-async-promise-executor
exitCode = await new Promise<number>(async (resolve, reject) => {
exitCode = await new Promise<number>((resolve, reject) => {
errorListener = (e: ErrorEvent) => {
const rethrown = new Error('Rethrown');
rethrown.cause = e.error;
Expand All @@ -396,25 +396,16 @@ export abstract class BasePHP implements IsomorphicLocalPHP {
'error',
errorListener
);

try {
resolve(
/**
* This is awkward, but Asyncify makes wasm_sapi_handle_request return
* Promise<Promise<number>>.
*
* @TODO: Determine whether this is a bug in emscripten or in our code.
*/
await await this[__private__dont__use].ccall(
'wasm_sapi_handle_request',
NUMBER,
[],
[]
)
);
} catch (e) {
reject(e);
const response = this[__private__dont__use].ccall(
'wasm_sapi_handle_request',
NUMBER,
[],
[]
);
if (response instanceof Promise) {
return response.then(resolve, reject);
}
return resolve(response);
});
} catch (e) {
/**
Expand Down

0 comments on commit 1a31776

Please sign in to comment.