Skip to content

Commit

Permalink
Update: Simplify base-php sapi_handle_request call (#387)
Browse files Browse the repository at this point in the history
<!-- Thanks for contributing to WordPress Playground! -->

## What?

<!-- In a few words, what is the PR actually doing? Include screenshots
or screencasts if applicable -->
We decided to split #331 into two parts, to make changes clear.
Part 1 of #331

Part 2 is #388

This pr simplifies the logic of `#handleRequest` method by removing
unnecessary async wrapping

## Why?

It was originally introduced in #331, where a 404 page for nonexisting
files was introduced, in order to simplify things.

For more info, look at #331 
<!-- Why is this PR necessary? What problem is it solving? Reference any
existing previous issue(s) or PR(s), but please add a short summary
here, too -->

## How?

- By removing the `async` function. `async` functions return a promise
by themselves and also `resolve` method is able to resolve nested
promises if they returned correctly. So the original `async` function
wasn't really necessary

<!-- How is your PR addressing the issue at hand? What are the
implementation details? -->

## Testing Instructions

N/A
  • Loading branch information
kozer authored May 18, 2023
1 parent b2fb4b8 commit 72221a5
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 72221a5

Please sign in to comment.