From df90330347ff6912e4dc8f7fd69288ca54f8931e Mon Sep 17 00:00:00 2001 From: Anna Henningsen Date: Fri, 27 Jul 2018 14:35:39 +0200 Subject: [PATCH 1/4] lib,src: standardize `owner_symbol` for handles Instead of somtimes using an `owner` string to link from a native handle object to the corresponding JS object, standardize on a single symbol that fulfills this role. --- lib/_tls_wrap.js | 23 ++++++++++++----------- lib/dgram.js | 16 ++++++++++++---- lib/internal/async_hooks.js | 4 ++-- lib/internal/child_process.js | 3 ++- lib/internal/cluster/child.js | 5 +++-- lib/internal/fs/watchers.js | 20 +++++++++++++++----- lib/internal/http2/core.js | 3 ++- lib/internal/wrap_js_stream.js | 13 +++++++------ lib/net.js | 24 ++++++++++++++++-------- src/async_wrap.cc | 4 ++++ src/env.h | 1 + src/tcp_wrap.cc | 2 +- 12 files changed, 77 insertions(+), 41 deletions(-) diff --git a/lib/_tls_wrap.js b/lib/_tls_wrap.js index 225d65fee3dd3d..83da2721e85173 100644 --- a/lib/_tls_wrap.js +++ b/lib/_tls_wrap.js @@ -35,6 +35,7 @@ const debug = util.debuglog('tls'); const tls_wrap = process.binding('tls_wrap'); const { TCP, constants: TCPConstants } = process.binding('tcp_wrap'); const { Pipe, constants: PipeConstants } = process.binding('pipe_wrap'); +const { owner_symbol } = require('internal/async_hooks').symbols; const { SecureContext: NativeSecureContext } = process.binding('crypto'); @@ -76,7 +77,7 @@ function onhandshakestart(now) { else this.handshakes++; - const { owner } = this; + const owner = this[owner_symbol]; if (this.handshakes > tls.CLIENT_RENEG_LIMIT) { owner._emitTLSError(new ERR_TLS_SESSION_ATTACK()); return; @@ -89,7 +90,7 @@ function onhandshakestart(now) { function onhandshakedone() { debug('onhandshakedone'); - const owner = this.owner; + const owner = this[owner_symbol]; // `newSession` callback wasn't called yet if (owner._newSessionPending) { @@ -102,7 +103,7 @@ function onhandshakedone() { function loadSession(hello) { - const owner = this.owner; + const owner = this[owner_symbol]; var once = false; function onSession(err, session) { @@ -130,7 +131,7 @@ function loadSession(hello) { function loadSNI(info) { - const owner = this.owner; + const owner = this[owner_symbol]; const servername = info.servername; if (!servername || !owner._SNICallback) return requestOCSP(owner, info); @@ -212,7 +213,7 @@ function requestOCSPDone(socket) { function onnewsession(key, session) { - const owner = this.owner; + const owner = this[owner_symbol]; if (!owner.server) return; @@ -241,11 +242,11 @@ function onnewsession(key, session) { function onocspresponse(resp) { - this.owner.emit('OCSPResponse', resp); + this[owner_symbol].emit('OCSPResponse', resp); } function onerror(err) { - const owner = this.owner; + const owner = this[owner_symbol]; if (owner._writableState.errorEmitted) return; @@ -364,9 +365,9 @@ for (var n = 0; n < proxiedMethods.length; n++) { tls_wrap.TLSWrap.prototype.close = function close(cb) { let ssl; - if (this.owner) { - ssl = this.owner.ssl; - this.owner.ssl = null; + if (this[owner_symbol]) { + ssl = this[owner_symbol].ssl; + this[owner_symbol].ssl = null; } // Invoke `destroySSL` on close to clean up possibly pending write requests @@ -405,7 +406,7 @@ TLSSocket.prototype._wrapHandle = function(wrap) { handle = options.pipe ? new Pipe(PipeConstants.SOCKET) : new TCP(TCPConstants.SOCKET); - handle.owner = this; + handle[owner_symbol] = this; } // Wrap socket's handle diff --git a/lib/dgram.js b/lib/dgram.js index 292f7daf876c43..8ddd523237185c 100644 --- a/lib/dgram.js +++ b/lib/dgram.js @@ -43,7 +43,7 @@ const { isUint8Array } = require('internal/util/types'); const EventEmitter = require('events'); const { defaultTriggerAsyncIdScope, - symbols: { async_id_symbol } + symbols: { async_id_symbol, owner_symbol } } = require('internal/async_hooks'); const { UV_UDP_REUSEADDR } = process.binding('constants').os; @@ -78,7 +78,7 @@ function Socket(type, listener) { } var handle = newHandle(type, lookup); - handle.owner = this; + handle[owner_symbol] = this; this[async_id_symbol] = handle.getAsyncId(); this.type = type; @@ -132,7 +132,7 @@ function replaceHandle(self, newHandle) { newHandle.lookup = oldHandle.lookup; newHandle.bind = oldHandle.bind; newHandle.send = oldHandle.send; - newHandle.owner = self; + newHandle[owner_symbol] = self; // Replace the existing handle by the handle we got from master. oldHandle.close(); @@ -635,7 +635,7 @@ function stopReceiving(socket) { function onMessage(nread, handle, buf, rinfo) { - var self = handle.owner; + var self = handle[owner_symbol]; if (nread < 0) { return self.emit('error', errnoException(nread, 'recvmsg')); } @@ -745,6 +745,14 @@ Socket.prototype._stopReceiving = function() { }; +// Legacy alias on the C++ wrapper object. This is not public API, so we may +// want to runtime-deprecate it at some point. There's no hurry, though. +Object.defineProperty(UDP.prototype, 'owner', { + get() { return this[owner_symbol]; }, + set(v) { return this[owner_symbol] = v; } +}); + + module.exports = { _createSocketHandle, createSocket, diff --git a/lib/internal/async_hooks.js b/lib/internal/async_hooks.js index 90155fa3c773dc..9edb23f8a6bdee 100644 --- a/lib/internal/async_hooks.js +++ b/lib/internal/async_hooks.js @@ -30,7 +30,7 @@ const async_wrap = process.binding('async_wrap'); * It has a fixed size, so if that is exceeded, calls to the native * side are used instead in pushAsyncIds() and popAsyncIds(). */ -const { async_hook_fields, async_id_fields } = async_wrap; +const { async_hook_fields, async_id_fields, owner_symbol } = async_wrap; // Store the pair executionAsyncId and triggerAsyncId in a std::stack on // Environment::AsyncHooks::async_ids_stack_ tracks the resource responsible for // the current execution stack. This is unwound as each resource exits. In the @@ -434,7 +434,7 @@ module.exports = { symbols: { async_id_symbol, trigger_async_id_symbol, init_symbol, before_symbol, after_symbol, destroy_symbol, - promise_resolve_symbol + promise_resolve_symbol, owner_symbol }, constants: { kInit, kBefore, kAfter, kDestroy, kTotals, kPromiseResolve diff --git a/lib/internal/child_process.js b/lib/internal/child_process.js index d22009505a4574..558932d341c8e8 100644 --- a/lib/internal/child_process.js +++ b/lib/internal/child_process.js @@ -27,6 +27,7 @@ const { TTY } = process.binding('tty_wrap'); const { TCP } = process.binding('tcp_wrap'); const { UDP } = process.binding('udp_wrap'); const SocketList = require('internal/socket_list'); +const { owner_symbol } = require('internal/async_hooks').symbols; const { convertToValidSignal } = require('internal/util'); const { isUint8Array } = require('internal/util/types'); const spawn_sync = process.binding('spawn_sync'); @@ -209,7 +210,7 @@ function ChildProcess() { this.spawnfile = null; this._handle = new Process(); - this._handle.owner = this; + this._handle[owner_symbol] = this; this._handle.onexit = (exitCode, signalCode) => { if (signalCode) { diff --git a/lib/internal/cluster/child.js b/lib/internal/cluster/child.js index 40c1a12327558f..39e891214ff787 100644 --- a/lib/internal/cluster/child.js +++ b/lib/internal/cluster/child.js @@ -3,6 +3,7 @@ const assert = require('assert'); const util = require('util'); const path = require('path'); const EventEmitter = require('events'); +const { owner_symbol } = require('internal/async_hooks').symbols; const Worker = require('internal/cluster/worker'); const { internal, sendHelper } = require('internal/cluster/utils'); const cluster = new EventEmitter(); @@ -207,8 +208,8 @@ function _disconnect(masterInitiated) { delete handles[key]; waitingCount++; - if (handle.owner) - handle.owner.close(checkWaitingCount); + if (handle[owner_symbol]) + handle[owner_symbol].close(checkWaitingCount); else handle.close(checkWaitingCount); } diff --git a/lib/internal/fs/watchers.js b/lib/internal/fs/watchers.js index e7edc1c5acd91f..90ea3971aae6f5 100644 --- a/lib/internal/fs/watchers.js +++ b/lib/internal/fs/watchers.js @@ -11,7 +11,10 @@ const { getStatsFromBinding, validatePath } = require('internal/fs/utils'); -const { defaultTriggerAsyncIdScope } = require('internal/async_hooks'); +const { + defaultTriggerAsyncIdScope, + symbols: { owner_symbol } +} = require('internal/async_hooks'); const { toNamespacedPath } = require('path'); const { validateUint32 } = require('internal/validators'); const { getPathFromURL } = require('internal/url'); @@ -20,7 +23,6 @@ const assert = require('assert'); const kOldStatus = Symbol('kOldStatus'); const kUseBigint = Symbol('kUseBigint'); -const kOwner = Symbol('kOwner'); function emitStop(self) { self.emit('stop'); @@ -36,7 +38,7 @@ function StatWatcher(bigint) { util.inherits(StatWatcher, EventEmitter); function onchange(newStatus, stats) { - const self = this[kOwner]; + const self = this[owner_symbol]; if (self[kOldStatus] === -1 && newStatus === -1 && stats[2/* new nlink */] === stats[16/* old nlink */]) { @@ -59,7 +61,7 @@ StatWatcher.prototype.start = function(filename, persistent, interval) { return; this._handle = new _StatWatcher(this[kUseBigint]); - this._handle[kOwner] = this; + this._handle[owner_symbol] = this; this._handle.onchange = onchange; if (!persistent) this._handle.unref(); @@ -104,7 +106,7 @@ function FSWatcher() { EventEmitter.call(this); this._handle = new FSEvent(); - this._handle.owner = this; + this._handle[owner_symbol] = this; this._handle.onchange = (status, eventType, filename) => { // TODO(joyeecheung): we may check self._handle.initialized here @@ -131,6 +133,7 @@ function FSWatcher() { } util.inherits(FSWatcher, EventEmitter); + // FIXME(joyeecheung): this method is not documented. // At the moment if filename is undefined, we // 1. Throw an Error if it's the first time .start() is called @@ -187,6 +190,13 @@ function emitCloseNT(self) { self.emit('close'); } +// Legacy alias on the C++ wrapper object. This is not public API, so we may +// want to runtime-deprecate it at some point. There's no hurry, though. +Object.defineProperty(FSEvent.prototype, 'owner', { + get() { return this[owner_symbol]; }, + set(v) { return this[owner_symbol] = v; } +}); + module.exports = { FSWatcher, StatWatcher diff --git a/lib/internal/http2/core.js b/lib/internal/http2/core.js index 18c0c4ce4c1fbd..568980c927bdb8 100644 --- a/lib/internal/http2/core.js +++ b/lib/internal/http2/core.js @@ -28,6 +28,7 @@ const { defaultTriggerAsyncIdScope, symbols: { async_id_symbol, + owner_symbol, }, } = require('internal/async_hooks'); const { internalBinding } = require('internal/bootstrap/loaders'); @@ -141,7 +142,7 @@ const kInfoHeaders = Symbol('sent-info-headers'); const kMaybeDestroy = Symbol('maybe-destroy'); const kLocalSettings = Symbol('local-settings'); const kOptions = Symbol('options'); -const kOwner = Symbol('owner'); +const kOwner = owner_symbol; const kProceed = Symbol('proceed'); const kProtocol = Symbol('protocol'); const kProxySocket = Symbol('proxy-socket'); diff --git a/lib/internal/wrap_js_stream.js b/lib/internal/wrap_js_stream.js index ec966028a024f7..e80716059ef6e7 100644 --- a/lib/internal/wrap_js_stream.js +++ b/lib/internal/wrap_js_stream.js @@ -6,16 +6,17 @@ const { Socket } = require('net'); const { JSStream } = process.binding('js_stream'); const uv = process.binding('uv'); const debug = util.debuglog('stream_wrap'); +const { owner_symbol } = require('internal/async_hooks').symbols; const { ERR_STREAM_WRAP } = require('internal/errors').codes; const kCurrentWriteRequest = Symbol('kCurrentWriteRequest'); const kCurrentShutdownRequest = Symbol('kCurrentShutdownRequest'); -function isClosing() { return this.owner.isClosing(); } -function onreadstart() { return this.owner.readStart(); } -function onreadstop() { return this.owner.readStop(); } -function onshutdown(req) { return this.owner.doShutdown(req); } -function onwrite(req, bufs) { return this.owner.doWrite(req, bufs); } +function isClosing() { return this[owner_symbol].isClosing(); } +function onreadstart() { return this[owner_symbol].readStart(); } +function onreadstop() { return this[owner_symbol].readStop(); } +function onshutdown(req) { return this[owner_symbol].doShutdown(req); } +function onwrite(req, bufs) { return this[owner_symbol].doWrite(req, bufs); } /* This class serves as a wrapper for when the C++ side of Node wants access * to a standard JS stream. For example, TLS or HTTP do not operate on network @@ -37,7 +38,7 @@ class JSStreamWrap extends Socket { this.doClose(cb); }; // Inside of the following functions, `this` refers to the handle - // and `this.owner` refers to this JSStreamWrap instance. + // and `this[owner_symbol]` refers to this JSStreamWrap instance. handle.isClosing = isClosing; handle.onreadstart = onreadstart; handle.onreadstop = onreadstop; diff --git a/lib/net.js b/lib/net.js index 889f28b0aa7042..8c05f2c07b5da5 100644 --- a/lib/net.js +++ b/lib/net.js @@ -56,7 +56,7 @@ const { const { newAsyncId, defaultTriggerAsyncIdScope, - symbols: { async_id_symbol } + symbols: { async_id_symbol, owner_symbol } } = require('internal/async_hooks'); const { createWriteWrap, @@ -207,7 +207,7 @@ function initSocketHandle(self) { // Handle creation may be deferred to bind() or connect() time. if (self._handle) { - self._handle.owner = self; + self._handle[owner_symbol] = self; self._handle.onread = onread; self[async_id_symbol] = getNewAsyncId(self._handle); } @@ -371,7 +371,7 @@ Socket.prototype._final = function(cb) { function afterShutdown(status, handle) { - var self = handle.owner; + var self = handle[owner_symbol]; debug('afterShutdown destroyed=%j', self.destroyed, self._readableState); @@ -620,7 +620,7 @@ Socket.prototype._destroy = function(exception, cb) { // buffer, or when there's an error reading. function onread(nread, buffer) { var handle = this; - var self = handle.owner; + var self = handle[owner_symbol]; assert(handle === self._handle, 'handle != self._handle'); self._unrefTimer(); @@ -819,7 +819,7 @@ protoGetter('bytesWritten', function bytesWritten() { function afterWrite(status, handle, err) { - var self = handle.owner; + var self = handle[owner_symbol]; if (self !== process.stderr && self !== process.stdout) debug('afterWrite', status); @@ -1123,7 +1123,7 @@ Socket.prototype.unref = function() { function afterConnect(status, handle, req, readable, writable) { - var self = handle.owner; + var self = handle[owner_symbol]; // callback may come after call to destroy if (self.destroyed) { @@ -1325,7 +1325,7 @@ function setupListenHandle(address, port, addressType, backlog, fd) { this[async_id_symbol] = getNewAsyncId(this._handle); this._handle.onconnection = onconnection; - this._handle.owner = this; + this._handle[owner_symbol] = this; // Use a backlog of 512 entries. We pass 511 to the listen() call because // the kernel does: backlogsize = roundup_pow_of_two(backlogsize + 1); @@ -1538,7 +1538,7 @@ Server.prototype.address = function() { function onconnection(err, clientHandle) { var handle = this; - var self = handle.owner; + var self = handle[owner_symbol]; debug('onconnection'); @@ -1669,6 +1669,14 @@ function emitCloseNT(self) { } +// Legacy alias on the C++ wrapper object. This is not public API, so we may +// want to runtime-deprecate it at some point. There's no hurry, though. +Object.defineProperty(TCP.prototype, 'owner', { + get() { return this[owner_symbol]; }, + set(v) { return this[owner_symbol] = v; } +}); + + Server.prototype.listenFD = internalUtil.deprecate(function(fd, type) { return this.listen({ fd: fd }); }, 'Server.listenFD is deprecated. Use Server.listen({fd: }) instead.', diff --git a/src/async_wrap.cc b/src/async_wrap.cc index 7ef3dafdf992c5..9d9d03b0fa9647 100644 --- a/src/async_wrap.cc +++ b/src/async_wrap.cc @@ -481,6 +481,10 @@ void AsyncWrap::Initialize(Local target, env->async_ids_stack_string(), env->async_hooks()->async_ids_stack().GetJSArray()).FromJust(); + target->Set(context, + FIXED_ONE_BYTE_STRING(env->isolate(), "owner_symbol"), + env->owner_symbol()).FromJust(); + Local constants = Object::New(isolate); #define SET_HOOKS_CONSTANT(name) \ FORCE_SET_TARGET_FIELD( \ diff --git a/src/env.h b/src/env.h index acbdd013285d6c..09e35431c031d9 100644 --- a/src/env.h +++ b/src/env.h @@ -116,6 +116,7 @@ struct PackageConfig { // for the sake of convenience. #define PER_ISOLATE_SYMBOL_PROPERTIES(V) \ V(handle_onclose_symbol, "handle_onclose") \ + V(owner_symbol, "owner") \ // Strings are per-isolate primitives but Environment proxies them // for the sake of convenience. Strings should be ASCII-only. diff --git a/src/tcp_wrap.cc b/src/tcp_wrap.cc index 805e566bfab08b..e817087d97110b 100644 --- a/src/tcp_wrap.cc +++ b/src/tcp_wrap.cc @@ -81,7 +81,7 @@ void TCPWrap::Initialize(Local target, // Init properties t->InstanceTemplate()->Set(FIXED_ONE_BYTE_STRING(env->isolate(), "reading"), Boolean::New(env->isolate(), false)); - t->InstanceTemplate()->Set(env->owner_string(), Null(env->isolate())); + t->InstanceTemplate()->Set(env->owner_symbol(), Null(env->isolate())); t->InstanceTemplate()->Set(env->onread_string(), Null(env->isolate())); t->InstanceTemplate()->Set(env->onconnection_string(), Null(env->isolate())); From df151457bce79c020b8c953b4acc39beab9e45a3 Mon Sep 17 00:00:00 2001 From: Anna Henningsen Date: Fri, 27 Jul 2018 14:36:40 +0200 Subject: [PATCH 2/4] process: use owner_symbol for `_getActive*` This makes it easier to provide public APIs in the return types of `process._getActiveHandles()` and `process._getActiveRequests()`. --- src/async_wrap.cc | 21 +++++++++++++++++++++ src/async_wrap.h | 6 ++++++ src/node.cc | 12 +++--------- 3 files changed, 30 insertions(+), 9 deletions(-) diff --git a/src/async_wrap.cc b/src/async_wrap.cc index 9d9d03b0fa9647..2688609aa891d2 100644 --- a/src/async_wrap.cc +++ b/src/async_wrap.cc @@ -734,6 +734,27 @@ std::string AsyncWrap::diagnostic_name() const { std::to_string(static_cast(async_id_)) + ")"; } +Local AsyncWrap::GetOwner() { + return GetOwner(env(), object()); +} + +Local AsyncWrap::GetOwner(Environment* env, Local obj) { + v8::EscapableHandleScope handle_scope(env->isolate()); + CHECK(!obj.IsEmpty()); + + v8::TryCatch ignore_exceptions(env->isolate()); + while (true) { + Local owner; + if (!obj->Get(env->context(), + env->owner_symbol()).ToLocal(&owner) || + !owner->IsObject()) { + return handle_scope.Escape(obj); + } + + obj = owner.As(); + }; +} + } // namespace node NODE_BUILTIN_MODULE_CONTEXT_AWARE(async_wrap, node::AsyncWrap::Initialize) diff --git a/src/async_wrap.h b/src/async_wrap.h index fed05ab021b3ca..1647f4bebc9a76 100644 --- a/src/async_wrap.h +++ b/src/async_wrap.h @@ -178,6 +178,12 @@ class AsyncWrap : public BaseObject { static void WeakCallback(const v8::WeakCallbackInfo &info); + // Returns the object that 'owns' an async wrap. For example, for a + // TCP connection handle, this is the corresponding net.Socket. + v8::Local GetOwner(); + static v8::Local GetOwner(Environment* env, + v8::Local obj); + // This is a simplified version of InternalCallbackScope that only runs // the `before` and `after` hooks. Only use it when not actually calling // back into JS; otherwise, use InternalCallbackScope. diff --git a/src/node.cc b/src/node.cc index 1f4d39a56b044c..655cfda107ddb3 100644 --- a/src/node.cc +++ b/src/node.cc @@ -1126,7 +1126,7 @@ static void GetActiveRequests(const FunctionCallbackInfo& args) { for (auto w : *env->req_wrap_queue()) { if (w->persistent().IsEmpty()) continue; - argv[idx] = w->object(); + argv[idx] = w->GetOwner(); if (++idx >= arraysize(argv)) { fn->Call(ctx, ary, idx, argv).ToLocalChecked(); idx = 0; @@ -1152,16 +1152,10 @@ void GetActiveHandles(const FunctionCallbackInfo& args) { Local argv[NODE_PUSH_VAL_TO_ARRAY_MAX]; size_t idx = 0; - Local owner_sym = env->owner_string(); - for (auto w : *env->handle_wrap_queue()) { - if (w->persistent().IsEmpty() || !HandleWrap::HasRef(w)) + if (!HandleWrap::HasRef(w)) continue; - Local object = w->object(); - Local owner = object->Get(owner_sym); - if (owner->IsUndefined()) - owner = object; - argv[idx] = owner; + argv[idx] = w->GetOwner(); if (++idx >= arraysize(argv)) { fn->Call(ctx, ary, idx, argv).ToLocalChecked(); idx = 0; From 4c477b985b523ffb95cbad32ebc0670734e2993c Mon Sep 17 00:00:00 2001 From: Anna Henningsen Date: Fri, 27 Jul 2018 15:07:19 +0200 Subject: [PATCH 3/4] fixup! process: use owner_symbol for `_getActive*` --- src/async_wrap.cc | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/async_wrap.cc b/src/async_wrap.cc index 2688609aa891d2..d47c0d6cfb6f44 100644 --- a/src/async_wrap.cc +++ b/src/async_wrap.cc @@ -752,7 +752,7 @@ Local AsyncWrap::GetOwner(Environment* env, Local obj) { } obj = owner.As(); - }; + } } } // namespace node From 8d547e5d3606707eaa3a7837bc0ff3d14048c78d Mon Sep 17 00:00:00 2001 From: Anna Henningsen Date: Fri, 27 Jul 2018 16:54:32 +0200 Subject: [PATCH 4/4] fixup! lib,src: standardize `owner_symbol` for handles --- src/env.h | 1 - 1 file changed, 1 deletion(-) diff --git a/src/env.h b/src/env.h index 09e35431c031d9..1237708469e30b 100644 --- a/src/env.h +++ b/src/env.h @@ -240,7 +240,6 @@ struct PackageConfig { V(openssl_error_stack, "opensslErrorStack") \ V(output_string, "output") \ V(order_string, "order") \ - V(owner_string, "owner") \ V(parse_error_string, "Parse Error") \ V(password_string, "password") \ V(path_string, "path") \