From 217e3dfea64d1cdf336eaec4748e592c9eaea632 Mon Sep 17 00:00:00 2001 From: Antoine du HAMEL Date: Thu, 6 Feb 2020 16:08:07 +0100 Subject: [PATCH] worker: allow URL in Worker constructor The explicit goal is to let users use `import.meta.url` to re-load thecurrent module inside a Worker instance. Fixes: https://github.com/nodejs/node/issues/30780 PR-URL: https://github.com/nodejs/node/pull/31664 Reviewed-By: James M Snell Reviewed-By: Anna Henningsen Reviewed-By: Gus Caplan Reviewed-By: Joyee Cheung --- doc/api/worker_threads.md | 16 ++++++--- lib/internal/errors.js | 11 ++++-- lib/internal/url.js | 12 ++++--- lib/internal/worker.js | 34 +++++++++++++++---- test/parallel/test-worker-type-check.js | 3 +- .../test-worker-unsupported-eval-on-url.mjs | 7 ++++ test/parallel/test-worker-unsupported-path.js | 24 +++++++++++++ test/parallel/test-worker.mjs | 19 +++++++++++ 8 files changed, 106 insertions(+), 20 deletions(-) create mode 100644 test/parallel/test-worker-unsupported-eval-on-url.mjs create mode 100644 test/parallel/test-worker.mjs diff --git a/doc/api/worker_threads.md b/doc/api/worker_threads.md index 8414b1c236b7c5..d50f9f0931fef2 100644 --- a/doc/api/worker_threads.md +++ b/doc/api/worker_threads.md @@ -513,6 +513,10 @@ if (isMainThread) { -* `filename` {string} The path to the Worker’s main script. Must be - either an absolute path or a relative path (i.e. relative to the - current working directory) starting with `./` or `../`. +* `filename` {string|URL} The path to the Worker’s main script or module. Must + be either an absolute path or a relative path (i.e. relative to the + current working directory) starting with `./` or `../`, or a WHATWG `URL` + object using `file:` protocol. If `options.eval` is `true`, this is a string containing JavaScript code rather than a path. * `options` {Object} @@ -536,8 +541,9 @@ changes: to specify that the parent thread and the child thread should share their environment variables; in that case, changes to one thread’s `process.env` object will affect the other thread as well. **Default:** `process.env`. - * `eval` {boolean} If `true`, interpret the first argument to the constructor - as a script that is executed once the worker is online. + * `eval` {boolean} If `true` and the first argument is a `string`, interpret + the first argument to the constructor as a script that is executed once the + worker is online. * `execArgv` {string[]} List of node CLI options passed to the worker. V8 options (such as `--max-old-space-size`) and options that affect the process (such as `--title`) are not supported. If set, this will be provided diff --git a/lib/internal/errors.js b/lib/internal/errors.js index 43e6ec2b8cc304..32d750d6178a89 100644 --- a/lib/internal/errors.js +++ b/lib/internal/errors.js @@ -1384,9 +1384,14 @@ E('ERR_WORKER_INVALID_EXEC_ARGV', (errors, msg = 'invalid execArgv flags') => E('ERR_WORKER_NOT_RUNNING', 'Worker instance not running', Error); E('ERR_WORKER_OUT_OF_MEMORY', 'Worker terminated due to reaching memory limit: %s', Error); -E('ERR_WORKER_PATH', - 'The worker script filename must be an absolute path or a relative ' + - 'path starting with \'./\' or \'../\'. Received "%s"', +E('ERR_WORKER_PATH', (filename) => + 'The worker script or module filename must be an absolute path or a ' + + 'relative path starting with \'./\' or \'../\'.' + + (filename.startsWith('file://') ? + ' If you want to pass a file:// URL, you must wrap it around `new URL`.' : + '' + ) + + ` Received "${filename}"`, TypeError); E('ERR_WORKER_UNSERIALIZABLE_ERROR', 'Serializing an uncaught exception failed', Error); diff --git a/lib/internal/url.js b/lib/internal/url.js index 09fa9f2cf47d50..efb842a074d750 100644 --- a/lib/internal/url.js +++ b/lib/internal/url.js @@ -1348,8 +1348,7 @@ function getPathFromURLPosix(url) { function fileURLToPath(path) { if (typeof path === 'string') path = new URL(path); - else if (path == null || !path[searchParams] || - !path[searchParams][searchParams]) + else if (!isURLInstance(path)) throw new ERR_INVALID_ARG_TYPE('path', ['string', 'URL'], path); if (path.protocol !== 'file:') throw new ERR_INVALID_URL_SCHEME('file'); @@ -1396,9 +1395,13 @@ function pathToFileURL(filepath) { return outURL; } +function isURLInstance(fileURLOrPath) { + return fileURLOrPath != null && fileURLOrPath[searchParams] && + fileURLOrPath[searchParams][searchParams]; +} + function toPathIfFileURL(fileURLOrPath) { - if (fileURLOrPath == null || !fileURLOrPath[searchParams] || - !fileURLOrPath[searchParams][searchParams]) + if (!isURLInstance(fileURLOrPath)) return fileURLOrPath; return fileURLToPath(fileURLOrPath); } @@ -1431,6 +1434,7 @@ module.exports = { fileURLToPath, pathToFileURL, toPathIfFileURL, + isURLInstance, URL, URLSearchParams, domainToASCII, diff --git a/lib/internal/worker.js b/lib/internal/worker.js index 7fc45695851f8c..782128749c09dc 100644 --- a/lib/internal/worker.js +++ b/lib/internal/worker.js @@ -27,8 +27,8 @@ const { ERR_INVALID_ARG_TYPE, // eslint-disable-next-line no-unused-vars ERR_WORKER_INIT_FAILED, + ERR_INVALID_ARG_VALUE, } = errorCodes; -const { validateString } = require('internal/validators'); const { getOptionValue } = require('internal/options'); const workerIo = require('internal/worker/io'); @@ -45,7 +45,7 @@ const { WritableWorkerStdio } = workerIo; const { deserializeError } = require('internal/error-serdes'); -const { pathToFileURL } = require('url'); +const { fileURLToPath, isURLInstance, pathToFileURL } = require('internal/url'); const { ownsProcessState, @@ -86,7 +86,6 @@ class Worker extends EventEmitter { constructor(filename, options = {}) { super(); debug(`[${threadId}] create new worker`, filename, options); - validateString(filename, 'filename'); if (options.execArgv && !ArrayIsArray(options.execArgv)) { throw new ERR_INVALID_ARG_TYPE('options.execArgv', 'Array', @@ -99,11 +98,33 @@ class Worker extends EventEmitter { } argv = options.argv.map(String); } - if (!options.eval) { - if (!path.isAbsolute(filename) && !/^\.\.?[\\/]/.test(filename)) { + + let url; + if (options.eval) { + if (typeof filename !== 'string') { + throw new ERR_INVALID_ARG_VALUE( + 'options.eval', + options.eval, + 'must be false when \'filename\' is not a string' + ); + } + url = null; + } else { + if (isURLInstance(filename)) { + url = filename; + filename = fileURLToPath(filename); + } else if (typeof filename !== 'string') { + throw new ERR_INVALID_ARG_TYPE( + 'filename', + ['string', 'URL'], + filename + ); + } else if (path.isAbsolute(filename) || /^\.\.?[\\/]/.test(filename)) { + filename = path.resolve(filename); + url = pathToFileURL(filename); + } else { throw new ERR_WORKER_PATH(filename); } - filename = path.resolve(filename); const ext = path.extname(filename); if (ext !== '.js' && ext !== '.mjs' && ext !== '.cjs') { @@ -125,7 +146,6 @@ class Worker extends EventEmitter { options.env); } - const url = options.eval ? null : pathToFileURL(filename); // Set up the C++ handle for the worker, as well as some internal wiring. this[kHandle] = new WorkerImpl(url, env === process.env ? null : env, diff --git a/test/parallel/test-worker-type-check.js b/test/parallel/test-worker-type-check.js index 2a67bfec405af2..88965f4be4c62d 100644 --- a/test/parallel/test-worker-type-check.js +++ b/test/parallel/test-worker-type-check.js @@ -20,7 +20,8 @@ const { Worker } = require('worker_threads'); { code: 'ERR_INVALID_ARG_TYPE', name: 'TypeError', - message: 'The "filename" argument must be of type string.' + + message: 'The "filename" argument must be of type string ' + + 'or an instance of URL.' + common.invalidArgTypeHelper(val) } ); diff --git a/test/parallel/test-worker-unsupported-eval-on-url.mjs b/test/parallel/test-worker-unsupported-eval-on-url.mjs new file mode 100644 index 00000000000000..f58f0d09789b82 --- /dev/null +++ b/test/parallel/test-worker-unsupported-eval-on-url.mjs @@ -0,0 +1,7 @@ +// Flags: --experimental-modules +import '../common/index.mjs'; +import assert from 'assert'; +import { Worker } from 'worker_threads'; + +const re = /The argument 'options\.eval' must be false when 'filename' is not a string\./; +assert.throws(() => new Worker(new URL(import.meta.url), { eval: true }), re); diff --git a/test/parallel/test-worker-unsupported-path.js b/test/parallel/test-worker-unsupported-path.js index 21ff40671965e5..e9cd6a32ec7cb9 100644 --- a/test/parallel/test-worker-unsupported-path.js +++ b/test/parallel/test-worker-unsupported-path.js @@ -13,6 +13,7 @@ const { Worker } = require('worker_threads'); assert.throws(() => { new Worker('/b'); }, expectedErr); assert.throws(() => { new Worker('/c.wasm'); }, expectedErr); assert.throws(() => { new Worker('/d.txt'); }, expectedErr); + assert.throws(() => { new Worker(new URL('file:///C:/e.wasm')); }, expectedErr); } { @@ -26,3 +27,26 @@ const { Worker } = require('worker_threads'); assert.throws(() => { new Worker('file:///file_url'); }, expectedErr); assert.throws(() => { new Worker('https://www.url.com'); }, expectedErr); } + +{ + assert.throws( + () => { new Worker('file:///file_url'); }, + /If you want to pass a file:\/\/ URL, you must wrap it around `new URL`/ + ); + assert.throws( + () => { new Worker('relative_no_dot'); }, + // eslint-disable-next-line node-core/no-unescaped-regexp-dot + /^((?!If you want to pass a file:\/\/ URL, you must wrap it around `new URL`).)*$/s + ); +} + +{ + const expectedErr = { + code: 'ERR_INVALID_URL_SCHEME', + name: 'TypeError' + }; + assert.throws(() => { new Worker(new URL('https://www.url.com')); }, + expectedErr); + assert.throws(() => { new Worker(new URL('data:application/javascript,')); }, + expectedErr); +} diff --git a/test/parallel/test-worker.mjs b/test/parallel/test-worker.mjs new file mode 100644 index 00000000000000..19059dccb14cc3 --- /dev/null +++ b/test/parallel/test-worker.mjs @@ -0,0 +1,19 @@ +// Flags: --experimental-modules +import { mustCall } from '../common/index.mjs'; +import assert from 'assert'; +import { Worker, isMainThread, parentPort } from 'worker_threads'; + +const TEST_STRING = 'Hello, world!'; + +if (isMainThread) { + const w = new Worker(new URL(import.meta.url)); + w.on('message', mustCall((message) => { + assert.strictEqual(message, TEST_STRING); + })); +} else { + setImmediate(() => { + process.nextTick(() => { + parentPort.postMessage(TEST_STRING); + }); + }); +}