Skip to content

Commit

Permalink
fix #595: revert the "worker_threads" change
Browse files Browse the repository at this point in the history
  • Loading branch information
evanw committed Dec 14, 2020
1 parent ef6eead commit e828322
Show file tree
Hide file tree
Showing 6 changed files with 82 additions and 178 deletions.
3 changes: 3 additions & 0 deletions .github/workflows/ci.yml
Original file line number Diff line number Diff line change
Expand Up @@ -44,6 +44,9 @@ jobs:
- name: npm ci
run: cd scripts && npm ci

- name: Register Test
run: node scripts/register-test.js

- name: Verify Source Map
run: node scripts/verify-source-map.js

Expand Down
10 changes: 10 additions & 0 deletions CHANGELOG.md
Original file line number Diff line number Diff line change
@@ -1,5 +1,15 @@
# Changelog

## Unreleased

* Fix non-string objects being passed to `transformSync` ([#596](https://github.com/evanw/esbuild/issues/596))

The transform function is only supposed to take a string. The type definitions also specify that the input must be a string. However, it happened to convert non-string inputs to a string and some code relied on that behavior. A change in 0.8.22 broke that behavior for `transformSync` specifically for `Uint8Array` objects, which became an array of numbers instead of a string. This release ensures that the conversion to a string is done up front to avoid something unexpected happening in the implementation. Future releases will likely enforce that the input is a string and throw an error otherwise.

* Revert the speedup to `transformSync` and `buildSync` ([#595](https://github.com/evanw/esbuild/issues/595))

This speedup relies on the `worker_threads` module in node. However, when esbuild is used via `node -r` as in `node -r esbuild-register file.ts`, the worker thread created by esbuild somehow ends up being completely detached from the main thread. This may be a bug in node itself. Regardless, the approach esbuild was using to improve speed doesn't work in all cases, so it has been reverted. It's unclear if it's possible to work around this issue, so this approach for improving the speed of synchronous APIs may be a dead end.

## 0.8.22

* Escape fewer characters in virtual module paths ([#588](https://github.com/evanw/esbuild/issues/588))
Expand Down
6 changes: 5 additions & 1 deletion Makefile
Original file line number Diff line number Diff line change
Expand Up @@ -11,7 +11,7 @@ test:
make -j6 test-common

# These tests are for development
test-common: test-go vet-go verify-source-map end-to-end-tests js-api-tests plugin-tests
test-common: test-go vet-go verify-source-map end-to-end-tests js-api-tests plugin-tests register-test

# These tests are for release (the extra tests are not included in "test" because they are pretty slow)
test-all:
Expand Down Expand Up @@ -39,6 +39,10 @@ test-wasm-node: platform-wasm
test-wasm-browser: platform-wasm | scripts/browser/node_modules
cd scripts/browser && node browser-tests.js

register-test: cmd/esbuild/version.go | scripts/node_modules
cd npm/esbuild && npm version "$(ESBUILD_VERSION)" --allow-same-version
node scripts/register-test.js

verify-source-map: cmd/esbuild/version.go | scripts/node_modules
cd npm/esbuild && npm version "$(ESBUILD_VERSION)" --allow-same-version
node scripts/verify-source-map.js
Expand Down
176 changes: 0 additions & 176 deletions lib/node.ts
Original file line number Diff line number Diff line change
Expand Up @@ -8,21 +8,6 @@ import fs = require('fs');
import os = require('os');
import tty = require('tty');

let worker_threads: typeof import('worker_threads') | undefined;

// There appears to be some some weird Windows-specific issue that non-
// deterministically causes tests that use the "worker_threads" library to fail
// to exit. This only happens in GitHub CI. I have not been able to reproduce
// this myself. Let's disable this optimization on Windows for now. This means
// synchronous API calls will potentially be ~10x slower on Windows.
if (os.platform() !== 'win32') {
// Don't crash if the "worker_threads" library isn't present
try {
worker_threads = require('worker_threads');
} catch {
}
}

declare const ESBUILD_VERSION: string;

// This file is used for both the "esbuild" package and the "esbuild-wasm"
Expand Down Expand Up @@ -109,13 +94,6 @@ export let transform: typeof types.transform = (input, options) => {
};

export let buildSync: typeof types.buildSync = (options: types.BuildOptions): any => {
// Try using a long-lived worker thread to avoid repeated start-up overhead
if (worker_threads) {
if (!workerThreadService) workerThreadService = startWorkerThreadService(worker_threads);
return workerThreadService.buildSync(options);
}

// Otherwise, fall back to running a dedicated child process
let result: types.BuildResult;
runServiceSync(service => service.buildOrServe('buildSync', null, options, isTTY(), (err, res) => {
if (err) throw err;
Expand All @@ -126,14 +104,6 @@ export let buildSync: typeof types.buildSync = (options: types.BuildOptions): an

export let transformSync: typeof types.transformSync = (input, options) => {
input += '';

// Try using a long-lived worker thread to avoid repeated start-up overhead
if (worker_threads) {
if (!workerThreadService) workerThreadService = startWorkerThreadService(worker_threads);
return workerThreadService.transformSync(input, options);
}

// Otherwise, fall back to running a dedicated child process
let result: types.TransformResult;
runServiceSync(service => service.transform('transformSync', input, options || {}, isTTY(), {
readFile(tempFile, callback) {
Expand Down Expand Up @@ -260,149 +230,3 @@ let runServiceSync = (callback: (service: common.StreamService) => void): void =
let randomFileName = () => {
return path.join(os.tmpdir(), `esbuild-${crypto.randomBytes(32).toString('hex')}`);
};

interface MainToWorkerMessage {
sharedBuffer: SharedArrayBuffer;
id: number;
command: string;
args: any[];
}

interface WorkerThreadService {
buildSync(options: types.BuildOptions): types.BuildResult;
transformSync(input: string, options?: types.TransformOptions): types.TransformResult;
}

let workerThreadService: WorkerThreadService | null = null;

let startWorkerThreadService = (worker_threads: typeof import('worker_threads')): WorkerThreadService => {
let { port1: mainPort, port2: workerPort } = new worker_threads.MessageChannel();
let worker = new worker_threads.Worker(__filename, {
workerData: workerPort,
transferList: [workerPort],
});
let nextID = 0;
let wasStopped = false;

// This forbids options which would cause structured clone errors
let validateBuildSyncOptions = (options: types.BuildOptions | undefined): void => {
if (!options) return
let plugins = options.plugins
let incremental = options.incremental
if (plugins && plugins.length > 0) throw new Error(`Cannot use plugins in synchronous API calls`);
if (incremental) throw new Error(`Cannot use "incremental" with a synchronous build`);
};

// MessagePort doesn't copy the properties of Error objects. We still want
// error objects to have extra properties such as "warnings" so implement the
// property copying manually.
let applyProperties = (object: any, properties: Record<string, any>): void => {
for (let key in properties) {
object[key] = properties[key];
}
};

let runCallSync = (command: string, args: any[]): any => {
if (wasStopped) throw new Error('The service was stopped');
let id = nextID++;

// Make a fresh shared buffer for every request. That way we can't have a
// race where a notification from the previous call overlaps with this call.
let sharedBuffer = new SharedArrayBuffer(8);
let sharedBufferView = new Int32Array(sharedBuffer);

// Send the message to the worker. Note that the worker could potentially
// complete the request before this thread returns from this call.
let msg: MainToWorkerMessage = { sharedBuffer, id, command, args };
worker.postMessage(msg);

// If the value hasn't changed (i.e. the request hasn't been completed,
// wait until the worker thread notifies us that the request is complete).
//
// Otherwise, if the value has changed, the request has already been
// completed. Don't wait in that case because the notification may never
// arrive if it has already been sent.
let status = Atomics.wait(sharedBufferView, 0, 0);
if (status !== 'ok' && status !== 'not-equal') throw new Error('Internal error: Atomics.wait() failed: ' + status);

let { message: { id: id2, resolve, reject, properties } } = worker_threads!.receiveMessageOnPort(mainPort)!;
if (id !== id2) throw new Error(`Internal error: Expected id ${id} but got id ${id2}`);
if (reject) {
applyProperties(reject, properties);
throw reject;
}
return resolve;
};

// Calling unref() on a worker will allow the thread to exit if it's the last
// only active handle in the event system. This means node will still exit
// when there are no more event handlers from the main thread. So there's no
// need to have a "stop()" function.
worker.unref();

return {
buildSync(options) {
validateBuildSyncOptions(options);
return runCallSync('build', [options]);
},
transformSync(input, options) {
return runCallSync('transform', [input, options]);
},
};
};

let startSyncServiceWorker = () => {
let workerPort: import('worker_threads').MessagePort = worker_threads!.workerData;
let parentPort = worker_threads!.parentPort!;
let servicePromise = startService();

// MessagePort doesn't copy the properties of Error objects. We still want
// error objects to have extra properties such as "warnings" so implement the
// property copying manually.
let extractProperties = (object: any): Record<string, any> => {
let properties: Record<string, any> = {};
if (object && typeof object === 'object') {
for (let key in object) {
properties[key] = object[key];
}
}
return properties;
};

parentPort.on('message', (msg: MainToWorkerMessage) => {
servicePromise.then(async (service) => {
let { sharedBuffer, id, command, args } = msg;
let sharedBufferView = new Int32Array(sharedBuffer);

try {
if (command === 'build') {
workerPort.postMessage({ id, resolve: await service.build(args[0]) });
} else if (command === 'transform') {
workerPort.postMessage({ id, resolve: await service.transform(args[0], args[1]) });
} else {
throw new Error(`Invalid command: ${command}`);
}
} catch (reject) {
workerPort.postMessage({ id, reject, properties: extractProperties(reject) });
}

// The message has already been posted by this point, so it should be
// safe to wake the main thread. The main thread should always get the
// message we sent above.

// First, change the shared value. That way if the main thread attempts
// to wait for us after this point, the wait will fail because the shared
// value has changed.
Atomics.add(sharedBufferView, 0, 1);

// Then, wake the main thread. This handles the case where the main
// thread was already waiting for us before the shared value was changed.
Atomics.notify(sharedBufferView, 0, Infinity);
});
});
};

// If we're in the worker thread, start the worker code
if (worker_threads && !worker_threads.isMainThread) {
startSyncServiceWorker();
}
5 changes: 4 additions & 1 deletion scripts/esbuild.js
Original file line number Diff line number Diff line change
Expand Up @@ -137,7 +137,10 @@ exports.installForTests = dir => {
childProcess.execSync(`npm install --silent --no-audit --progress=false esbuild-${version}.tgz`, { cwd: installDir, env, stdio: 'inherit' })

// Evaluate the code
return require(path.join(installDir, 'node_modules', 'esbuild'))
const ESBUILD_PACKAGE_PATH = path.join(installDir, 'node_modules', 'esbuild')
const mod = require(ESBUILD_PACKAGE_PATH)
mod.ESBUILD_PACKAGE_PATH = ESBUILD_PACKAGE_PATH
return mod
}

// This is helpful for ES6 modules which don't have access to __dirname
Expand Down
60 changes: 60 additions & 0 deletions scripts/register-test.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,60 @@
const { installForTests } = require('./esbuild')
const child_process = require('child_process')
const path = require('path')
const fs = require('fs')
const rimraf = require('rimraf')
const assert = require('assert')

const rootTestDir = path.join(__dirname, '.register-test')
const esbuild = installForTests(rootTestDir)

const entry = path.join(rootTestDir, 'entry.ts')
fs.writeFileSync(entry, `
console.log('worked' as string)
`)

const register = path.join(rootTestDir, 'register.js')
fs.writeFileSync(register, `
const esbuild = require(${JSON.stringify(esbuild.ESBUILD_PACKAGE_PATH)});
const fs = require('fs');
require.extensions['.ts'] = (mod, filename) => {
const ts = fs.readFileSync(filename, 'utf8');
const { code } = esbuild.transformSync(ts, { loader: 'ts' });
mod._compile(code, filename);
};
`)

async function main() {
let result
let promise = new Promise((resolve, reject) => child_process.execFile('node', ['-r', register, entry], (err, stdout) => {
if (err) {
reject(err)
} else {
result = stdout
resolve()
}
}))
let timeout
let wait = new Promise((_, reject) => {
timeout = setTimeout(() => reject(new Error('This test timed out')), 60 * 1000)
})
await Promise.race([promise, wait])
clearTimeout(timeout)
assert.strictEqual(result, `worked\n`)
}

main().then(
() => {
console.log(`✅ register test passed`)
try {
rimraf.sync(rootTestDir, { disableGlob: true })
} catch (e) {
// This doesn't work on Windows due to "EPERM: operation not permitted"
// but that's ok for CI because the VM will just be thrown away anyway.
}
},
e => {
console.error(`❌ register test failed: ${e && e.message || e}`)
process.exit(1)
},
)

0 comments on commit e828322

Please sign in to comment.