From c34db7a9c2d11f4995b13843045d170da631523f Mon Sep 17 00:00:00 2001 From: Anna Henningsen Date: Sun, 23 Sep 2018 19:14:57 +0200 Subject: [PATCH] worker: reduce `MessagePort` prototype to documented API MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit `MessagePort` is special because it has to be a C++ API that is exposed to userland. Therefore, there is a number of internal methods on its native prototype; this commit reduces this set of methods to only what is documented in the API. PR-URL: https://github.com/nodejs/node/pull/23037 Reviewed-By: Gus Caplan Reviewed-By: Michaël Zasso Reviewed-By: Luigi Pinca Reviewed-By: Ruben Bridgewater Reviewed-By: James M Snell --- lib/internal/worker.js | 49 ++++++++++++++--------- src/node_worker.cc | 4 -- test/parallel/test-heapdump-worker.js | 3 +- test/parallel/test-worker-message-port.js | 9 +++++ 4 files changed, 41 insertions(+), 24 deletions(-) diff --git a/lib/internal/worker.js b/lib/internal/worker.js index b5dab224211b13..ceec8469b19a9a 100644 --- a/lib/internal/worker.js +++ b/lib/internal/worker.js @@ -14,18 +14,18 @@ const { const { internalBinding } = require('internal/bootstrap/loaders'); const { MessagePort, MessageChannel } = internalBinding('messaging'); -const { handle_onclose } = internalBinding('symbols'); +const { + handle_onclose: handleOnCloseSymbol, + oninit: onInitSymbol +} = internalBinding('symbols'); const { clearAsyncIdStack } = require('internal/async_hooks'); const { serializeError, deserializeError } = require('internal/error-serdes'); const { pathToFileURL } = require('url'); -util.inherits(MessagePort, EventEmitter); - const { Worker: WorkerImpl, getEnvMessagePort, - threadId, - oninit: oninit_symbol + threadId } = internalBinding('worker'); const isMainThread = threadId === 0; @@ -58,6 +58,23 @@ const messageTypes = { LOAD_SCRIPT: 'loadScript' }; +// We have to mess with the MessagePort prototype a bit, so that a) we can make +// it inherit from EventEmitter, even though it is a C++ class, and b) we do +// not provide methods that are not present in the Browser and not documented +// on our side (e.g. hasRef). +// Save a copy of the original set of methods as a shallow clone. +const MessagePortPrototype = Object.create( + Object.getPrototypeOf(MessagePort.prototype), + Object.getOwnPropertyDescriptors(MessagePort.prototype)); +// Set up the new inheritance chain. +Object.setPrototypeOf(MessagePort, EventEmitter); +Object.setPrototypeOf(MessagePort.prototype, EventEmitter.prototype); +// Finally, purge methods we don't want to be public. +delete MessagePort.prototype.stop; +delete MessagePort.prototype.drain; +delete MessagePort.prototype.hasRef; +delete MessagePort.prototype.getAsyncId; + // A communication channel consisting of a handle (that wraps around an // uv_async_t) which can receive information from other threads and emits // .onmessage events, and a function used for sending data to a MessagePort @@ -81,10 +98,10 @@ Object.defineProperty(MessagePort.prototype, 'onmessage', { this[kOnMessageListener] = value; if (typeof value === 'function') { this.ref(); - this.start(); + MessagePortPrototype.start.call(this); } else { this.unref(); - this.stop(); + MessagePortPrototype.stop.call(this); } } }); @@ -94,7 +111,7 @@ function oninit() { setupPortReferencing(this, this, 'message'); } -Object.defineProperty(MessagePort.prototype, oninit_symbol, { +Object.defineProperty(MessagePort.prototype, onInitSymbol, { enumerable: true, writable: false, value: oninit @@ -111,22 +128,18 @@ function onclose() { this.emit('close'); } -Object.defineProperty(MessagePort.prototype, handle_onclose, { +Object.defineProperty(MessagePort.prototype, handleOnCloseSymbol, { enumerable: false, writable: false, value: onclose }); -const originalClose = MessagePort.prototype.close; MessagePort.prototype.close = function(cb) { if (typeof cb === 'function') this.once('close', cb); - originalClose.call(this); + MessagePortPrototype.close.call(this); }; -const drainMessagePort = MessagePort.prototype.drain; -delete MessagePort.prototype.drain; - Object.defineProperty(MessagePort.prototype, util.inspect.custom, { enumerable: false, writable: false, @@ -135,7 +148,7 @@ Object.defineProperty(MessagePort.prototype, util.inspect.custom, { try { // This may throw when `this` does not refer to a native object, // e.g. when accessing the prototype directly. - ref = this.hasRef(); + ref = MessagePortPrototype.hasRef.call(this); } catch { return this; } return Object.assign(Object.create(MessagePort.prototype), ref === undefined ? { @@ -157,12 +170,12 @@ function setupPortReferencing(port, eventEmitter, eventName) { eventEmitter.on('newListener', (name) => { if (name === eventName && eventEmitter.listenerCount(eventName) === 0) { port.ref(); - port.start(); + MessagePortPrototype.start.call(port); } }); eventEmitter.on('removeListener', (name) => { if (name === eventName && eventEmitter.listenerCount(eventName) === 0) { - port.stop(); + MessagePortPrototype.stop.call(port); port.unref(); } }); @@ -304,7 +317,7 @@ class Worker extends EventEmitter { [kOnExit](code) { debug(`[${threadId}] hears end event for Worker ${this.threadId}`); - drainMessagePort.call(this[kPublicPort]); + MessagePortPrototype.drain.call(this[kPublicPort]); this[kDispose](); this.emit('exit', code); this.removeAllListeners(); diff --git a/src/node_worker.cc b/src/node_worker.cc index 6b9657e115562c..7825c9b0f54793 100644 --- a/src/node_worker.cc +++ b/src/node_worker.cc @@ -501,10 +501,6 @@ void InitWorker(Local target, thread_id_string, Number::New(env->isolate(), static_cast(env->thread_id()))).FromJust(); - Local oninit_string = FIXED_ONE_BYTE_STRING(env->isolate(), "oninit"); - target->Set(env->context(), - oninit_string, - env->oninit_symbol()).FromJust(); } } // anonymous namespace diff --git a/test/parallel/test-heapdump-worker.js b/test/parallel/test-heapdump-worker.js index b7b5c2f756717c..44a50dd66b5fca 100644 --- a/test/parallel/test-heapdump-worker.js +++ b/test/parallel/test-heapdump-worker.js @@ -19,8 +19,7 @@ validateSnapshotNodes('Worker', [ validateSnapshotNodes('MessagePort', [ { children: [ - { name: 'MessagePortData' }, - { name: 'MessagePort' } + { name: 'MessagePortData' } ] } ], { loose: true }); diff --git a/test/parallel/test-worker-message-port.js b/test/parallel/test-worker-message-port.js index 2d321611ec7758..995868e9a770e0 100644 --- a/test/parallel/test-worker-message-port.js +++ b/test/parallel/test-worker-message-port.js @@ -69,3 +69,12 @@ const { MessageChannel, MessagePort } = require('worker_threads'); }); }); } + +{ + assert.deepStrictEqual( + Object.getOwnPropertyNames(MessagePort.prototype).sort(), + [ + 'close', 'constructor', 'onmessage', 'postMessage', 'ref', 'start', + 'unref' + ]); +}