From 5af80b299ab57cda8811fa19aa1d1992a50e592b Mon Sep 17 00:00:00 2001 From: Nick Babcock Date: Sat, 17 Dec 2022 09:20:42 -0600 Subject: [PATCH] fix(wasm): rejecting with error on node.js file read (#1346) When node.js is reading the wasm file and encounters an error (like if the file doesn't exist or user doesn't have permissions to read). It will still try and call `_instantiateOrCompile` with `undefined`, which results in an uncaught exception, possibly obscuring the error. As instead of the file read error, the following error will return: > TypeError: WebAssembly.compile(): Argument 0 must be a buffer source This can be replicated with the following script (don't execute at a REPL): ```js (async () => { await new Promise((resolve, reject) => { reject(new Error("permission error")); resolve(WebAssembly.compile(undefined)); }); })(); ``` Node.js will exit with an error code and print only the type error to stderr. If we try and catch the error at the outer level ```js (async () => { try { await new Promise((resolve, reject) => { reject(new Error("permission error")); resolve(WebAssembly.compile(undefined)); }); } catch (ex) { console.log(`this is the error: ${ex}`) } })(); ``` The log statement will exclude the type error: > this is the error: Error: permission error But node.js will still exit with an error code and the type error is printed to stderr. This is the behavior of node.js default [uncaught exception handler](https://nodejs.org/api/process.html#event-uncaughtexception) > By default, Node.js handles such exceptions by printing the stack trace to stderr and exiting with code 1 This commit avoids the uncaught exception by only invoking `resolve` behind a conditional on an absent error. This commit doesn't change the status of the promise as calling `resolve` after `reject`, while legal, doesn't really do anything [(ref)](https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/Promise/Promise#description): > Only the first call to `resolveFunc` or `rejectFunc` affects the promise's > eventual state, and subsequent calls to either function can neither change the > fulfillment value/rejection reason nor toggle its eventual state from > "fulfilled" to "rejected" or opposite So depending on the runtime and how the user is overriding the default handler, uncaught exceptions may be handled differently. The test executes in a worker so that it can require the appropiate node js modules. I saw that another Wasm test used a worker, but would never execute as require is not defined, so this commit also fixes that test case. --- packages/wasm/src/helper.ts | 4 +-- packages/wasm/test/test.mjs | 67 ++++++++++++++++++++++++------------- 2 files changed, 46 insertions(+), 25 deletions(-) diff --git a/packages/wasm/src/helper.ts b/packages/wasm/src/helper.ts index 161b4d13d..ea35455ef 100644 --- a/packages/wasm/src/helper.ts +++ b/packages/wasm/src/helper.ts @@ -10,9 +10,9 @@ return new Promise((resolve, reject) => { fs.readFile(path.resolve(__dirname, filepath), (error, buffer) => { if (error != null) { reject(error) + } else { + resolve(_instantiateOrCompile(buffer, imports, false)) } - - resolve(_instantiateOrCompile(buffer, imports, false)) }); }); `; diff --git a/packages/wasm/test/test.mjs b/packages/wasm/test/test.mjs index c27f1b0ed..2714c87b7 100755 --- a/packages/wasm/test/test.mjs +++ b/packages/wasm/test/test.mjs @@ -1,6 +1,7 @@ import { createRequire } from 'module'; import { sep, posix, join } from 'path'; import { fileURLToPath } from 'url'; +import { Worker } from 'worker_threads'; import { rollup } from 'rollup'; import globby from 'globby'; @@ -96,32 +97,26 @@ test('imports', async (t) => { await testBundle(t, bundle); }); -try { - // eslint-disable-next-line global-require - const { Worker } = require('worker_threads'); - test('worker', async (t) => { - t.plan(2); +test('worker', async (t) => { + t.plan(2); - const bundle = await rollup({ - input: 'fixtures/worker.js', - plugins: [wasmPlugin()] - }); - const code = await getCode(bundle); - const executeWorker = () => { - const worker = new Worker(code, { eval: true }); - return new Promise((resolve, reject) => { - worker.on('error', (err) => reject(err)); - worker.on('exit', (exitCode) => resolve(exitCode)); - }); - }; - await t.notThrowsAsync(async () => { - const result = await executeWorker(); - t.true(result === 0); + const bundle = await rollup({ + input: 'fixtures/worker.js', + plugins: [wasmPlugin()] + }); + const code = await getCode(bundle); + const executeWorker = () => { + const worker = new Worker(code, { eval: true }); + return new Promise((resolve, reject) => { + worker.on('error', (err) => reject(err)); + worker.on('exit', (exitCode) => resolve(exitCode)); }); + }; + await t.notThrowsAsync(async () => { + const result = await executeWorker(); + t.true(result === 0); }); -} catch (err) { - // worker threads aren't fully supported in Node versions before 11.7.0 -} +}); test('injectHelper', async (t) => { t.plan(4); @@ -233,3 +228,29 @@ test('works as CJS plugin', async (t) => { }); await testBundle(t, bundle); }); + +// uncaught exception will cause test failures on this node version. +if (!process.version.startsWith('v14')) { + test('avoid uncaught exception on file read', async (t) => { + t.plan(2); + + const bundle = await rollup({ + input: 'fixtures/complex.js', + plugins: [wasmPlugin({ maxFileSize: 0, targetEnv: 'node' })] + }); + + const raw = await getCode(bundle); + const code = raw.replace('.wasm', '-does-not-exist.wasm'); + + const executeWorker = () => { + const worker = new Worker(`let result; ${code}`, { eval: true }); + return new Promise((resolve, reject) => { + worker.on('error', (err) => reject(err)); + worker.on('exit', (exitCode) => resolve(exitCode)); + }); + }; + + const err = await t.throwsAsync(() => executeWorker()); + t.regex(err.message, /no such file or directory/); + }); +}