From 5d75ba4930bff448054fa6ff7bc02b70ea3d49c4 Mon Sep 17 00:00:00 2001 From: Trevor Norris Date: Mon, 31 Oct 2016 16:01:18 -0600 Subject: [PATCH 01/40] test: remove async_wrap tests The async_wrap API will be changing substantially and breaking the existing API. So remove the current tests and add them back after they've been reimplemented. --- .../test-async-wrap-check-providers.js | 124 ------------------ ...st-async-wrap-disabled-propagate-parent.js | 52 -------- .../test-async-wrap-post-did-throw.js | 39 ------ .../test-async-wrap-propagate-parent.js | 51 ------- .../test-async-wrap-throw-from-callback.js | 73 ----------- .../parallel/test-async-wrap-throw-no-init.js | 25 ---- test/parallel/test-async-wrap-uid.js | 63 --------- test/parallel/test-stream-base-no-abort.js | 64 --------- 8 files changed, 491 deletions(-) delete mode 100644 test/parallel/test-async-wrap-check-providers.js delete mode 100644 test/parallel/test-async-wrap-disabled-propagate-parent.js delete mode 100644 test/parallel/test-async-wrap-post-did-throw.js delete mode 100644 test/parallel/test-async-wrap-propagate-parent.js delete mode 100644 test/parallel/test-async-wrap-throw-from-callback.js delete mode 100644 test/parallel/test-async-wrap-throw-no-init.js delete mode 100644 test/parallel/test-async-wrap-uid.js delete mode 100644 test/parallel/test-stream-base-no-abort.js diff --git a/test/parallel/test-async-wrap-check-providers.js b/test/parallel/test-async-wrap-check-providers.js deleted file mode 100644 index 354534a6b30a5a..00000000000000 --- a/test/parallel/test-async-wrap-check-providers.js +++ /dev/null @@ -1,124 +0,0 @@ -'use strict'; - -const common = require('../common'); -if (!common.hasCrypto) { - common.skip('missing crypto'); - return; -} - -const assert = require('assert'); -const crypto = require('crypto'); -const dgram = require('dgram'); -const dns = require('dns'); -const fs = require('fs'); -const net = require('net'); -const tls = require('tls'); -const zlib = require('zlib'); -const ChildProcess = require('child_process').ChildProcess; -const StreamWrap = require('_stream_wrap').StreamWrap; -const HTTPParser = process.binding('http_parser').HTTPParser; -const async_wrap = process.binding('async_wrap'); -const pkeys = Object.keys(async_wrap.Providers); - -let keyList = pkeys.slice(); -// Drop NONE -keyList.splice(0, 1); - -// fs-watch currently needs special configuration on AIX and we -// want to improve under https://github.com/nodejs/node/issues/5085. -// strip out fs watch related parts for now -if (common.isAix) { - for (let i = 0; i < keyList.length; i++) { - if ((keyList[i] === 'FSEVENTWRAP') || (keyList[i] === 'STATWATCHER')) { - keyList.splice(i, 1); - } - } -} - -function init(id, provider) { - keyList = keyList.filter((e) => e !== pkeys[provider]); -} - -function noop() { } - -async_wrap.setupHooks({ init }); - -async_wrap.enable(); - - -setTimeout(function() { }, 1); - -fs.stat(__filename, noop); - -if (!common.isAix) { - // fs-watch currently needs special configuration on AIX and we - // want to improve under https://github.com/nodejs/node/issues/5085. - // strip out fs watch related parts for now - fs.watchFile(__filename, noop); - fs.unwatchFile(__filename); - fs.watch(__filename).close(); -} - -dns.lookup('localhost', noop); -dns.lookupService('::', 0, noop); -dns.resolve('localhost', noop); - -new StreamWrap(new net.Socket()); - -new (process.binding('tty_wrap').TTY)(); - -crypto.randomBytes(1, noop); - -common.refreshTmpDir(); - -net.createServer(function(c) { - c.end(); - this.close(); -}).listen(common.PIPE, function() { - net.connect(common.PIPE, noop); -}); - -net.createServer(function(c) { - c.end(); - this.close(checkTLS); -}).listen(0, function() { - net.connect(this.address().port, noop); -}); - -dgram.createSocket('udp4').bind(0, function() { - this.send(Buffer.allocUnsafe(2), 0, 2, this.address().port, '::', () => { - this.close(); - }); -}); - -process.on('SIGINT', () => process.exit()); - -// Run from closed net server above. -function checkTLS() { - const options = { - key: fs.readFileSync(common.fixturesDir + '/keys/ec-key.pem'), - cert: fs.readFileSync(common.fixturesDir + '/keys/ec-cert.pem') - }; - const server = tls.createServer(options, noop) - .listen(0, function() { - const connectOpts = { rejectUnauthorized: false }; - tls.connect(this.address().port, connectOpts, function() { - this.destroy(); - server.close(); - }); - }); -} - -zlib.createGzip(); - -new ChildProcess(); - -new HTTPParser(HTTPParser.REQUEST); - -process.on('exit', function() { - if (keyList.length !== 0) { - process._rawDebug('Not all keys have been used:'); - process._rawDebug(keyList); - assert.strictEqual(keyList.length, 0); - } -}); diff --git a/test/parallel/test-async-wrap-disabled-propagate-parent.js b/test/parallel/test-async-wrap-disabled-propagate-parent.js deleted file mode 100644 index 4a6df7500956bf..00000000000000 --- a/test/parallel/test-async-wrap-disabled-propagate-parent.js +++ /dev/null @@ -1,52 +0,0 @@ -'use strict'; - -require('../common'); -const assert = require('assert'); -const net = require('net'); -const async_wrap = process.binding('async_wrap'); -const providers = Object.keys(async_wrap.Providers); - -const uidSymbol = Symbol('uid'); - -let cntr = 0; -let client; - -function init(uid, type, parentUid, parentHandle) { - this[uidSymbol] = uid; - - if (parentHandle) { - cntr++; - // Cannot assert in init callback or will abort. - process.nextTick(() => { - assert.strictEqual(providers[type], 'TCPWRAP'); - assert.strictEqual(parentUid, server._handle[uidSymbol], - 'server uid doesn\'t match parent uid'); - assert.strictEqual(parentHandle, server._handle, - 'server handle doesn\'t match parent handle'); - assert.strictEqual(this, client._handle, 'client doesn\'t match context'); - }); - } -} - -function noop() { } - -async_wrap.setupHooks({ init }); -async_wrap.enable(); - -const server = net.createServer(function(c) { - client = c; - // Allow init callback to run before closing. - setImmediate(() => { - c.end(); - this.close(); - }); -}).listen(0, function() { - net.connect(this.address().port, noop); -}); - -async_wrap.disable(); - -process.on('exit', function() { - // init should have only been called once with a parent. - assert.strictEqual(cntr, 1); -}); diff --git a/test/parallel/test-async-wrap-post-did-throw.js b/test/parallel/test-async-wrap-post-did-throw.js deleted file mode 100644 index 35dbfe1378464a..00000000000000 --- a/test/parallel/test-async-wrap-post-did-throw.js +++ /dev/null @@ -1,39 +0,0 @@ -'use strict'; - -const common = require('../common'); -if (!common.hasCrypto) { - common.skip('missing crypto'); - return; -} - -const assert = require('assert'); -const async_wrap = process.binding('async_wrap'); -let asyncThrows = 0; -let uncaughtExceptionCount = 0; - -process.on('uncaughtException', (e) => { - assert.strictEqual(e.message, 'oh noes!', 'error messages do not match'); -}); - -process.on('exit', () => { - process.removeAllListeners('uncaughtException'); - assert.strictEqual(uncaughtExceptionCount, 1); - assert.strictEqual(uncaughtExceptionCount, asyncThrows); -}); - -function init() { } -function post(id, threw) { - if (threw) - uncaughtExceptionCount++; -} - -async_wrap.setupHooks({ init, post }); -async_wrap.enable(); - -// Timers still aren't supported, so use crypto API. -// It's also important that the callback not happen in a nextTick, like many -// error events in core. -require('crypto').randomBytes(0, () => { - asyncThrows++; - throw new Error('oh noes!'); -}); diff --git a/test/parallel/test-async-wrap-propagate-parent.js b/test/parallel/test-async-wrap-propagate-parent.js deleted file mode 100644 index 0968490f5975cc..00000000000000 --- a/test/parallel/test-async-wrap-propagate-parent.js +++ /dev/null @@ -1,51 +0,0 @@ -'use strict'; - -require('../common'); -const assert = require('assert'); -const net = require('net'); -const async_wrap = process.binding('async_wrap'); -const providers = Object.keys(async_wrap.Providers); - -const uidSymbol = Symbol('uid'); - -let cntr = 0; -let client; - -function init(uid, type, parentUid, parentHandle) { - this[uidSymbol] = uid; - - if (parentHandle) { - cntr++; - // Cannot assert in init callback or will abort. - process.nextTick(() => { - assert.strictEqual(providers[type], 'TCPWRAP'); - assert.strictEqual(parentUid, server._handle[uidSymbol], - 'server uid doesn\'t match parent uid'); - assert.strictEqual(parentHandle, server._handle, - 'server handle doesn\'t match parent handle'); - assert.strictEqual(this, client._handle, 'client doesn\'t match context'); - }); - } -} - -function noop() { } - -async_wrap.setupHooks({ init }); -async_wrap.enable(); - -const server = net.createServer(function(c) { - client = c; - // Allow init callback to run before closing. - setImmediate(() => { - c.end(); - this.close(); - }); -}).listen(0, function() { - net.connect(this.address().port, noop); -}); - - -process.on('exit', function() { - // init should have only been called once with a parent. - assert.strictEqual(cntr, 1); -}); diff --git a/test/parallel/test-async-wrap-throw-from-callback.js b/test/parallel/test-async-wrap-throw-from-callback.js deleted file mode 100644 index c4a6a9bdaddd1c..00000000000000 --- a/test/parallel/test-async-wrap-throw-from-callback.js +++ /dev/null @@ -1,73 +0,0 @@ -'use strict'; - -const common = require('../common'); -if (!common.hasCrypto) { - common.skip('missing crypto'); - return; -} - -const async_wrap = process.binding('async_wrap'); -const assert = require('assert'); -const crypto = require('crypto'); -const domain = require('domain'); -const spawn = require('child_process').spawn; -const callbacks = [ 'init', 'pre', 'post', 'destroy' ]; -const toCall = process.argv[2]; -let msgCalled = 0; -let msgReceived = 0; - -function init() { - if (toCall === 'init') - throw new Error('init'); -} -function pre() { - if (toCall === 'pre') - throw new Error('pre'); -} -function post() { - if (toCall === 'post') - throw new Error('post'); -} -function destroy() { - if (toCall === 'destroy') - throw new Error('destroy'); -} - -if (typeof process.argv[2] === 'string') { - async_wrap.setupHooks({ init, pre, post, destroy }); - async_wrap.enable(); - - process.on('uncaughtException', common.fail); - - const d = domain.create(); - d.on('error', common.fail); - d.run(() => { - // Using randomBytes because timers are not yet supported. - crypto.randomBytes(0, () => { }); - }); - -} else { - - process.on('exit', (code) => { - assert.strictEqual(msgCalled, callbacks.length); - assert.strictEqual(msgCalled, msgReceived); - }); - - callbacks.forEach((item) => { - msgCalled++; - - const child = spawn(process.execPath, [__filename, item]); - let errstring = ''; - - child.stderr.on('data', (data) => { - errstring += data.toString(); - }); - - child.on('close', (code) => { - if (errstring.includes('Error: ' + item)) - msgReceived++; - - assert.strictEqual(code, 1, `${item} closed with code ${code}`); - }); - }); -} diff --git a/test/parallel/test-async-wrap-throw-no-init.js b/test/parallel/test-async-wrap-throw-no-init.js deleted file mode 100644 index 1f8cc70b36c6e7..00000000000000 --- a/test/parallel/test-async-wrap-throw-no-init.js +++ /dev/null @@ -1,25 +0,0 @@ -'use strict'; - -require('../common'); -const assert = require('assert'); -const async_wrap = process.binding('async_wrap'); - -assert.throws(function() { - async_wrap.setupHooks(null); -}, /first argument must be an object/); - -assert.throws(function() { - async_wrap.setupHooks({}); -}, /init callback must be a function/); - -assert.throws(function() { - async_wrap.enable(); -}, /init callback is not assigned to a function/); - -// Should not throw -async_wrap.setupHooks({ init: () => {} }); -async_wrap.enable(); - -assert.throws(function() { - async_wrap.setupHooks(() => {}); -}, /hooks should not be set while also enabled/); diff --git a/test/parallel/test-async-wrap-uid.js b/test/parallel/test-async-wrap-uid.js deleted file mode 100644 index f16388cfe766f0..00000000000000 --- a/test/parallel/test-async-wrap-uid.js +++ /dev/null @@ -1,63 +0,0 @@ -'use strict'; - -require('../common'); -const fs = require('fs'); -const assert = require('assert'); -const async_wrap = process.binding('async_wrap'); - -// Give the event loop time to clear out the final uv_close(). -let si_cntr = 3; -process.on('beforeExit', () => { - if (--si_cntr > 0) setImmediate(() => {}); -}); - -const storage = new Map(); -async_wrap.setupHooks({ init, pre, post, destroy }); -async_wrap.enable(); - -function init(uid) { - storage.set(uid, { - init: true, - pre: false, - post: false, - destroy: false, - }); -} - -function pre(uid) { - storage.get(uid).pre = true; -} - -function post(uid) { - storage.get(uid).post = true; -} - -function destroy(uid) { - storage.get(uid).destroy = true; -} - -fs.access(__filename, function(err) { - assert.ifError(err); -}); - -fs.access(__filename, function(err) { - assert.ifError(err); -}); - -async_wrap.disable(); - -process.once('exit', function() { - assert.strictEqual(storage.size, 2); - - for (const item of storage) { - const uid = item[0]; - const value = item[1]; - assert.strictEqual(typeof uid, 'number'); - assert.deepStrictEqual(value, { - init: true, - pre: true, - post: true, - destroy: true, - }); - } -}); diff --git a/test/parallel/test-stream-base-no-abort.js b/test/parallel/test-stream-base-no-abort.js deleted file mode 100644 index 8b85acea2fb7ea..00000000000000 --- a/test/parallel/test-stream-base-no-abort.js +++ /dev/null @@ -1,64 +0,0 @@ -'use strict'; - -const common = require('../common'); -if (!common.hasCrypto) { - common.skip('missing crypto'); - return; -} - -const async_wrap = process.binding('async_wrap'); -const uv = process.binding('uv'); -const assert = require('assert'); -const dgram = require('dgram'); -const fs = require('fs'); -const net = require('net'); -const tls = require('tls'); -const providers = Object.keys(async_wrap.Providers); -let flags = 0; - -// Make sure all asserts have run at least once. -process.on('exit', () => assert.strictEqual(flags, 0b111)); - -function init(id, provider) { - this._external; // Test will abort if nullptr isn't properly checked. - switch (providers[provider]) { - case 'TCPWRAP': - assert.strictEqual(this.fd, uv.UV_EINVAL); - flags |= 0b1; - break; - case 'TLSWRAP': - assert.strictEqual(this.fd, uv.UV_EINVAL); - flags |= 0b10; - break; - case 'UDPWRAP': - assert.strictEqual(this.fd, uv.UV_EBADF); - flags |= 0b100; - break; - } -} - -async_wrap.setupHooks({ init }); -async_wrap.enable(); - -const checkTLS = common.mustCall(function checkTLS() { - const options = { - key: fs.readFileSync(common.fixturesDir + '/keys/ec-key.pem'), - cert: fs.readFileSync(common.fixturesDir + '/keys/ec-cert.pem') - }; - const server = tls.createServer(options, () => {}) - .listen(0, function() { - const connectOpts = { rejectUnauthorized: false }; - tls.connect(this.address().port, connectOpts, function() { - this.destroy(); - server.close(); - }); - }); -}); - -const checkTCP = common.mustCall(function checkTCP() { - net.createServer(() => {}).listen(0, function() { - this.close(checkTLS); - }); -}); - -dgram.createSocket('udp4').close(checkTCP); From 53a85f186a0534d2b76607407b48c80339d3085a Mon Sep 17 00:00:00 2001 From: Trevor Norris Date: Mon, 31 Oct 2016 16:48:14 -0600 Subject: [PATCH 02/40] stream_base,tls_wrap: notify on destruct Classes like TLSWrap hold a pointer to another StreamBase instance. The lifetime of these two are independent and allowed one to be retained while the other is cleaned up. In the case of TLSWrap, the private member TLSWrap::stream_ would be deleted, while the TLSWrap could still attempt to access it to see if it was alive. Give StreamBase a destruct callback to notify when the class is being cleaned up. Use this in TLSWrap to clear out the field. --- src/stream_base.h | 9 +++- src/tls_wrap.cc | 9 +++- src/tls_wrap.h | 3 ++ .../test-tls-retain-handle-no-abort.js | 42 +++++++++++++++++++ 4 files changed, 61 insertions(+), 2 deletions(-) create mode 100644 test/parallel/test-tls-retain-handle-no-abort.js diff --git a/src/stream_base.h b/src/stream_base.h index faddee88c82786..35929750bfbc54 100644 --- a/src/stream_base.h +++ b/src/stream_base.h @@ -146,10 +146,14 @@ class StreamResource { const uv_buf_t* buf, uv_handle_type pending, void* ctx); + typedef void (*DestructCb)(void* ctx); StreamResource() : bytes_read_(0) { } - virtual ~StreamResource() = default; + virtual ~StreamResource() { + if (!destruct_cb_.is_empty()) + destruct_cb_.fn(destruct_cb_.ctx); + } virtual int DoShutdown(ShutdownWrap* req_wrap) = 0; virtual int DoTryWrite(uv_buf_t** bufs, size_t* count); @@ -186,15 +190,18 @@ class StreamResource { inline void set_alloc_cb(Callback c) { alloc_cb_ = c; } inline void set_read_cb(Callback c) { read_cb_ = c; } + inline void set_destruct_cb(Callback c) { destruct_cb_ = c; } inline Callback after_write_cb() { return after_write_cb_; } inline Callback alloc_cb() { return alloc_cb_; } inline Callback read_cb() { return read_cb_; } + inline Callback destruct_cb() { return destruct_cb_; } private: Callback after_write_cb_; Callback alloc_cb_; Callback read_cb_; + Callback destruct_cb_; uint64_t bytes_read_; friend class StreamBase; diff --git a/src/tls_wrap.cc b/src/tls_wrap.cc index d56128fec6c5ce..505cadee8faa81 100644 --- a/src/tls_wrap.cc +++ b/src/tls_wrap.cc @@ -64,6 +64,7 @@ TLSWrap::TLSWrap(Environment* env, stream_->set_after_write_cb({ OnAfterWriteImpl, this }); stream_->set_alloc_cb({ OnAllocImpl, this }); stream_->set_read_cb({ OnReadImpl, this }); + stream_->set_destruct_cb({ OnDestructImpl, this }); set_alloc_cb({ OnAllocSelf, this }); set_read_cb({ OnReadSelf, this }); @@ -522,7 +523,7 @@ int TLSWrap::GetFD() { bool TLSWrap::IsAlive() { - return ssl_ != nullptr && stream_->IsAlive(); + return ssl_ != nullptr && stream_ != nullptr && stream_->IsAlive(); } @@ -660,6 +661,12 @@ void TLSWrap::OnReadImpl(ssize_t nread, } +void TLSWrap::OnDestructImpl(void* ctx) { + TLSWrap* wrap = static_cast(ctx); + wrap->clear_stream(); +} + + void TLSWrap::OnAllocSelf(size_t suggested_size, uv_buf_t* buf, void* ctx) { buf->base = node::Malloc(suggested_size); buf->len = suggested_size; diff --git a/src/tls_wrap.h b/src/tls_wrap.h index f390c9fe9281f7..fbf664edd530c0 100644 --- a/src/tls_wrap.h +++ b/src/tls_wrap.h @@ -54,6 +54,8 @@ class TLSWrap : public AsyncWrap, size_t self_size() const override { return sizeof(*this); } + void clear_stream() { stream_ = nullptr; } + protected: static const int kClearOutChunkSize = 16384; @@ -121,6 +123,7 @@ class TLSWrap : public AsyncWrap, const uv_buf_t* buf, uv_handle_type pending, void* ctx); + static void OnDestructImpl(void* ctx); void DoRead(ssize_t nread, const uv_buf_t* buf, uv_handle_type pending); diff --git a/test/parallel/test-tls-retain-handle-no-abort.js b/test/parallel/test-tls-retain-handle-no-abort.js new file mode 100644 index 00000000000000..43b3709fd5f85b --- /dev/null +++ b/test/parallel/test-tls-retain-handle-no-abort.js @@ -0,0 +1,42 @@ +'use strict'; + +const common = require('../common'); +const assert = require('assert'); + +if (!common.hasCrypto) { + common.skip('missing crypto'); + return; +} +const tls = require('tls'); +const fs = require('fs'); +const util = require('util'); + +const sent = 'hello world'; +const serverOptions = { + isServer: true, + key: fs.readFileSync(common.fixturesDir + '/keys/agent1-key.pem'), + cert: fs.readFileSync(common.fixturesDir + '/keys/agent1-cert.pem') +}; + +let ssl = null; + +process.on('exit', function() { + assert.ok(ssl !== null); + // If the internal pointer to stream_ isn't cleared properly then this + // will abort. + util.inspect(ssl); +}); + +const server = tls.createServer(serverOptions, function(s) { + s.on('data', function() { }); + s.on('end', function() { + server.close(); + s.destroy(); + }); +}).listen(0, function() { + const c = new tls.TLSSocket(); + ssl = c.ssl; + c.connect(this.address().port, function() { + c.end(sent); + }); +}); From 757ac358370eaba641e685a4456ecf8f1d917f19 Mon Sep 17 00:00:00 2001 From: Trevor Norris Date: Wed, 28 Sep 2016 15:40:01 -0600 Subject: [PATCH 03/40] crypto: use named FunctionTemplate RandomBytes and PBKDF2 were using the same "generic" ObjectTemplate for construction. Instead create one for each that is properly named. --- src/env-inl.h | 7 ------- src/env.h | 5 ++--- src/node_crypto.cc | 19 +++++++++++++++++-- 3 files changed, 19 insertions(+), 12 deletions(-) diff --git a/src/env-inl.h b/src/env-inl.h index 1a17e2947d0597..93552bf6117ed0 100644 --- a/src/env-inl.h +++ b/src/env-inl.h @@ -190,7 +190,6 @@ inline Environment::Environment(IsolateData* isolate_data, fn->SetClassName(FIXED_ONE_BYTE_STRING(isolate(), "InternalFieldObject")); v8::Local obj = fn->InstanceTemplate(); obj->SetInternalFieldCount(1); - set_generic_internal_field_template(obj); RB_INIT(&cares_task_list_); AssignToContext(context); @@ -465,12 +464,6 @@ inline void Environment::SetTemplateMethod(v8::Local that, t->SetClassName(name_string); // NODE_SET_METHOD() compatibility. } -inline v8::Local Environment::NewInternalFieldObject() { - v8::MaybeLocal m_obj = - generic_internal_field_template()->NewInstance(context()); - return m_obj.ToLocalChecked(); -} - #define VP(PropertyName, StringValue) V(v8::Private, PropertyName) #define VS(PropertyName, StringValue) V(v8::String, PropertyName) #define V(TypeName, PropertyName) \ diff --git a/src/env.h b/src/env.h index 8c256ca9c7faa4..251249465025b3 100644 --- a/src/env.h +++ b/src/env.h @@ -240,13 +240,14 @@ namespace node { V(domain_array, v8::Array) \ V(domains_stack_array, v8::Array) \ V(fs_stats_constructor_function, v8::Function) \ - V(generic_internal_field_template, v8::ObjectTemplate) \ V(jsstream_constructor_template, v8::FunctionTemplate) \ V(module_load_list_array, v8::Array) \ + V(pbkdf2_constructor_template, v8::ObjectTemplate) \ V(pipe_constructor_template, v8::FunctionTemplate) \ V(process_object, v8::Object) \ V(promise_reject_function, v8::Function) \ V(push_values_to_array_function, v8::Function) \ + V(randombytes_constructor_template, v8::ObjectTemplate) \ V(script_context_constructor_template, v8::FunctionTemplate) \ V(script_data_constructor_function, v8::Function) \ V(secure_context_constructor_template, v8::FunctionTemplate) \ @@ -507,8 +508,6 @@ class Environment { const char* name, v8::FunctionCallback callback); - inline v8::Local NewInternalFieldObject(); - // Strings and private symbols are shared across shared contexts // The getters simply proxy to the per-isolate primitive. #define VP(PropertyName, StringValue) V(v8::Private, PropertyName) diff --git a/src/node_crypto.cc b/src/node_crypto.cc index f84d42f2321ab9..4cef8acf891bbb 100644 --- a/src/node_crypto.cc +++ b/src/node_crypto.cc @@ -78,6 +78,7 @@ using v8::Isolate; using v8::Local; using v8::Null; using v8::Object; +using v8::ObjectTemplate; using v8::Persistent; using v8::PropertyAttribute; using v8::PropertyCallbackInfo; @@ -5382,7 +5383,8 @@ void PBKDF2(const FunctionCallbackInfo& args) { digest = EVP_sha1(); } - obj = env->NewInternalFieldObject(); + obj = env->pbkdf2_constructor_template()-> + NewInstance(env->context()).ToLocalChecked(); req = new PBKDF2Request(env, obj, digest, @@ -5550,7 +5552,8 @@ void RandomBytes(const FunctionCallbackInfo& args) { if (size < 0 || size > Buffer::kMaxLength) return env->ThrowRangeError("size is not a valid Smi"); - Local obj = env->NewInternalFieldObject(); + Local obj = env->randombytes_constructor_template()-> + NewInstance(env->context()).ToLocalChecked(); RandomBytesRequest* req = new RandomBytesRequest(env, obj, size); if (args[1]->IsFunction()) { @@ -6019,6 +6022,18 @@ void InitCrypto(Local target, PublicKeyCipher::Cipher); + + Local pb = FunctionTemplate::New(env->isolate()); + pb->SetClassName(FIXED_ONE_BYTE_STRING(env->isolate(), "PBKDF2")); + Local pbt = pb->InstanceTemplate(); + pbt->SetInternalFieldCount(1); + env->set_pbkdf2_constructor_template(pbt); + + Local rb = FunctionTemplate::New(env->isolate()); + rb->SetClassName(FIXED_ONE_BYTE_STRING(env->isolate(), "RandomBytes")); + Local rbt = rb->InstanceTemplate(); + rbt->SetInternalFieldCount(1); + env->set_randombytes_constructor_template(rbt); } } // namespace crypto From 29e9d0980b8201f5618be9e0f05f972736b7b966 Mon Sep 17 00:00:00 2001 From: Trevor Norris Date: Wed, 2 Nov 2016 14:53:42 -0600 Subject: [PATCH 04/40] async_wrap: use more specific providers Instead of wrapping several providers into PROVIDER_CRYPTO, have them all be named after their class. Rename other providers to also match their class names. With the exception of Parser. Which is actually HTTPParser. Add PROVIDER_LENGTH to make better checks in WrapperInfo(). --- src/async-wrap.cc | 4 +++- src/async-wrap.h | 11 +++++++---- src/node_crypto.cc | 4 ++-- src/node_crypto.h | 2 +- src/node_zlib.cc | 2 +- src/udp_wrap.cc | 2 +- 6 files changed, 15 insertions(+), 10 deletions(-) diff --git a/src/async-wrap.cc b/src/async-wrap.cc index a0780566db72d8..343530985d7bdc 100644 --- a/src/async-wrap.cc +++ b/src/async-wrap.cc @@ -88,7 +88,9 @@ intptr_t RetainedAsyncInfo::GetSizeInBytes() { RetainedObjectInfo* WrapperInfo(uint16_t class_id, Local wrapper) { // No class_id should be the provider type of NONE. - CHECK_NE(NODE_ASYNC_ID_OFFSET, class_id); + CHECK_GT(class_id, NODE_ASYNC_ID_OFFSET); + // And make sure the class_id doesn't extend past the last provider. + CHECK_LE(class_id - NODE_ASYNC_ID_OFFSET, AsyncWrap::PROVIDERS_LENGTH); CHECK(wrapper->IsObject()); CHECK(!wrapper.IsEmpty()); diff --git a/src/async-wrap.h b/src/async-wrap.h index d01c6ce9f9b724..93d9078e08a1c5 100644 --- a/src/async-wrap.h +++ b/src/async-wrap.h @@ -15,17 +15,20 @@ namespace node { #define NODE_ASYNC_PROVIDER_TYPES(V) \ V(NONE) \ - V(CRYPTO) \ + V(CONNECTION) \ V(FSEVENTWRAP) \ V(FSREQWRAP) \ V(GETADDRINFOREQWRAP) \ V(GETNAMEINFOREQWRAP) \ V(HTTPPARSER) \ V(JSSTREAM) \ - V(PIPEWRAP) \ + V(PBKDF2REQUEST) \ V(PIPECONNECTWRAP) \ + V(PIPEWRAP) \ V(PROCESSWRAP) \ V(QUERYWRAP) \ + V(RANDOMBYTESREQUEST) \ + V(SENDWRAP) \ V(SHUTDOWNWRAP) \ V(SIGNALWRAP) \ V(STATWATCHER) \ @@ -35,9 +38,8 @@ namespace node { V(TLSWRAP) \ V(TTYWRAP) \ V(UDPWRAP) \ - V(UDPSENDWRAP) \ V(WRITEWRAP) \ - V(ZLIB) + V(ZCTX) class Environment; @@ -48,6 +50,7 @@ class AsyncWrap : public BaseObject { PROVIDER_ ## PROVIDER, NODE_ASYNC_PROVIDER_TYPES(V) #undef V + PROVIDERS_LENGTH, }; AsyncWrap(Environment* env, diff --git a/src/node_crypto.cc b/src/node_crypto.cc index 4cef8acf891bbb..4a6d16e5416624 100644 --- a/src/node_crypto.cc +++ b/src/node_crypto.cc @@ -5164,7 +5164,7 @@ class PBKDF2Request : public AsyncWrap { char* salt, int iter, int keylen) - : AsyncWrap(env, object, AsyncWrap::PROVIDER_CRYPTO), + : AsyncWrap(env, object, AsyncWrap::PROVIDER_PBKDF2REQUEST), digest_(digest), error_(0), passlen_(passlen), @@ -5430,7 +5430,7 @@ void PBKDF2(const FunctionCallbackInfo& args) { class RandomBytesRequest : public AsyncWrap { public: RandomBytesRequest(Environment* env, Local object, size_t size) - : AsyncWrap(env, object, AsyncWrap::PROVIDER_CRYPTO), + : AsyncWrap(env, object, AsyncWrap::PROVIDER_RANDOMBYTESREQUEST), error_(0), size_(size), data_(node::Malloc(size)) { diff --git a/src/node_crypto.h b/src/node_crypto.h index 175206c40df586..4c524e66181441 100644 --- a/src/node_crypto.h +++ b/src/node_crypto.h @@ -381,7 +381,7 @@ class Connection : public AsyncWrap, public SSLWrap { v8::Local wrap, SecureContext* sc, SSLWrap::Kind kind) - : AsyncWrap(env, wrap, AsyncWrap::PROVIDER_CRYPTO), + : AsyncWrap(env, wrap, AsyncWrap::PROVIDER_CONNECTION), SSLWrap(env, sc, kind), bio_read_(nullptr), bio_write_(nullptr), diff --git a/src/node_zlib.cc b/src/node_zlib.cc index 0b85e6a1d60921..a47ab45bfdc62c 100644 --- a/src/node_zlib.cc +++ b/src/node_zlib.cc @@ -52,7 +52,7 @@ void InitZlib(v8::Local target); class ZCtx : public AsyncWrap { public: ZCtx(Environment* env, Local wrap, node_zlib_mode mode) - : AsyncWrap(env, wrap, AsyncWrap::PROVIDER_ZLIB), + : AsyncWrap(env, wrap, AsyncWrap::PROVIDER_ZCTX), dictionary_(nullptr), dictionary_len_(0), err_(0), diff --git a/src/udp_wrap.cc b/src/udp_wrap.cc index d14eefd64d600a..61884623755d48 100644 --- a/src/udp_wrap.cc +++ b/src/udp_wrap.cc @@ -44,7 +44,7 @@ class SendWrap : public ReqWrap { SendWrap::SendWrap(Environment* env, Local req_wrap_obj, bool have_callback) - : ReqWrap(env, req_wrap_obj, AsyncWrap::PROVIDER_UDPSENDWRAP), + : ReqWrap(env, req_wrap_obj, AsyncWrap::PROVIDER_SENDWRAP), have_callback_(have_callback) { Wrap(req_wrap_obj, this); } From 82d8606d2bd267ef518fa23bb1e02873f8ef7787 Mon Sep 17 00:00:00 2001 From: Trevor Norris Date: Mon, 12 Dec 2016 23:24:02 -0700 Subject: [PATCH 05/40] async_wrap: use double, not int64_t, for uid The array math in JS converting the in64_t to a safe integer is unnecessary. Instead set the field to a double and allow internals direct access to the array to reduce overhead. Not ideal, but gets us the same number of values and simplifies the JS interface. --- src/async-wrap-inl.h | 4 ++-- src/async-wrap.cc | 12 ++++++------ src/async-wrap.h | 4 ++-- src/env-inl.h | 8 ++++---- src/env.h | 8 ++++---- 5 files changed, 18 insertions(+), 18 deletions(-) diff --git a/src/async-wrap-inl.h b/src/async-wrap-inl.h index 64b5f091612368..a9bb90a8aa7fa2 100644 --- a/src/async-wrap-inl.h +++ b/src/async-wrap-inl.h @@ -25,8 +25,8 @@ inline AsyncWrap::ProviderType AsyncWrap::provider_type() const { } -inline int64_t AsyncWrap::get_uid() const { - return uid_; +inline double AsyncWrap::get_id() const { + return id_; } diff --git a/src/async-wrap.cc b/src/async-wrap.cc index 343530985d7bdc..61d31eb72b6ddf 100644 --- a/src/async-wrap.cc +++ b/src/async-wrap.cc @@ -201,7 +201,7 @@ void AsyncWrap::DestroyIdsCb(uv_idle_t* handle) { TryCatch try_catch(env->isolate()); - std::vector destroy_ids_list; + std::vector destroy_ids_list; destroy_ids_list.swap(*env->destroy_ids_list()); for (auto current_id : destroy_ids_list) { // Want each callback to be cleaned up after itself, instead of cleaning @@ -236,7 +236,7 @@ AsyncWrap::AsyncWrap(Environment* env, ProviderType provider, AsyncWrap* parent) : BaseObject(env, object), bits_(static_cast(provider) << 1), - uid_(env->get_async_wrap_uid()) { + id_(env->get_async_wrap_uid()) { CHECK_NE(provider, PROVIDER_NONE); CHECK_GE(object->InternalFieldCount(), 1); @@ -258,14 +258,14 @@ AsyncWrap::AsyncWrap(Environment* env, HandleScope scope(env->isolate()); Local argv[] = { - Number::New(env->isolate(), get_uid()), + Number::New(env->isolate(), get_id()), Int32::New(env->isolate(), provider), Null(env->isolate()), Null(env->isolate()) }; if (parent != nullptr) { - argv[2] = Number::New(env->isolate(), parent->get_uid()); + argv[2] = Number::New(env->isolate(), parent->get_id()); argv[3] = parent->object(); } @@ -290,7 +290,7 @@ AsyncWrap::~AsyncWrap() { if (env()->destroy_ids_list()->empty()) uv_idle_start(env()->destroy_ids_idle_handle(), DestroyIdsCb); - env()->destroy_ids_list()->push_back(get_uid()); + env()->destroy_ids_list()->push_back(get_id()); } @@ -301,7 +301,7 @@ Local AsyncWrap::MakeCallback(const Local cb, Local pre_fn = env()->async_hooks_pre_function(); Local post_fn = env()->async_hooks_post_function(); - Local uid = Number::New(env()->isolate(), get_uid()); + Local uid = Number::New(env()->isolate(), get_id()); Local context = object(); Local domain; bool has_domain = false; diff --git a/src/async-wrap.h b/src/async-wrap.h index 93d9078e08a1c5..c156ce3a3b8a80 100644 --- a/src/async-wrap.h +++ b/src/async-wrap.h @@ -68,7 +68,7 @@ class AsyncWrap : public BaseObject { inline ProviderType provider_type() const; - inline int64_t get_uid() const; + inline double get_id() const; // Only call these within a valid HandleScope. v8::Local MakeCallback(const v8::Local cb, @@ -91,7 +91,7 @@ class AsyncWrap : public BaseObject { // expected the context object will receive a _asyncQueue object property // that will be used to call pre/post in MakeCallback. uint32_t bits_; - const int64_t uid_; + const double id_; }; void LoadAsyncWrapperInfo(Environment* env); diff --git a/src/env-inl.h b/src/env-inl.h index 93552bf6117ed0..ea67fa20221c51 100644 --- a/src/env-inl.h +++ b/src/env-inl.h @@ -171,7 +171,7 @@ inline Environment::Environment(IsolateData* isolate_data, printed_error_(false), trace_sync_io_(false), makecallback_cntr_(0), - async_wrap_uid_(0), + async_wrap_id_(0), debugger_agent_(this), #if HAVE_INSPECTOR inspector_agent_(this), @@ -320,11 +320,11 @@ inline void Environment::set_trace_sync_io(bool value) { trace_sync_io_ = value; } -inline int64_t Environment::get_async_wrap_uid() { - return ++async_wrap_uid_; +inline double Environment::get_async_wrap_uid() { + return ++async_wrap_id_; } -inline std::vector* Environment::destroy_ids_list() { +inline std::vector* Environment::destroy_ids_list() { return &destroy_ids_list_; } diff --git a/src/env.h b/src/env.h index 251249465025b3..8683286faf360b 100644 --- a/src/env.h +++ b/src/env.h @@ -465,10 +465,10 @@ class Environment { void PrintSyncTrace() const; inline void set_trace_sync_io(bool value); - inline int64_t get_async_wrap_uid(); + inline double get_async_wrap_uid(); // List of id's that have been destroyed and need the destroy() cb called. - inline std::vector* destroy_ids_list(); + inline std::vector* destroy_ids_list(); inline double* heap_statistics_buffer() const; inline void set_heap_statistics_buffer(double* pointer); @@ -567,8 +567,8 @@ class Environment { bool printed_error_; bool trace_sync_io_; size_t makecallback_cntr_; - int64_t async_wrap_uid_; - std::vector destroy_ids_list_; + double async_wrap_id_; + std::vector destroy_ids_list_; debugger::Agent debugger_agent_; #if HAVE_INSPECTOR inspector::Agent inspector_agent_; From 4a427e3bf91f8da61c2d84aea0b446b2a581dc2e Mon Sep 17 00:00:00 2001 From: Trevor Norris Date: Wed, 2 Nov 2016 13:55:00 -0600 Subject: [PATCH 06/40] async_wrap: add GetAsyncId() method Allow handles to retrieve their own uid's by adding a new method on the FunctionTemplates. Implementation of these into all other classes will come in a future commit. --- src/async-wrap.cc | 8 ++++++++ src/async-wrap.h | 2 ++ 2 files changed, 10 insertions(+) diff --git a/src/async-wrap.cc b/src/async-wrap.cc index 61d31eb72b6ddf..8af2159b4255a9 100644 --- a/src/async-wrap.cc +++ b/src/async-wrap.cc @@ -294,6 +294,14 @@ AsyncWrap::~AsyncWrap() { } +void AsyncWrap::GetAsyncId(const FunctionCallbackInfo& args) { + AsyncWrap* wrap; + args.GetReturnValue().Set(-1); + ASSIGN_OR_RETURN_UNWRAP(&wrap, args.Holder()); + args.GetReturnValue().Set(wrap->get_id()); +} + + Local AsyncWrap::MakeCallback(const Local cb, int argc, Local* argv) { diff --git a/src/async-wrap.h b/src/async-wrap.h index c156ce3a3b8a80..70df8faaa74d9c 100644 --- a/src/async-wrap.h +++ b/src/async-wrap.h @@ -64,6 +64,8 @@ class AsyncWrap : public BaseObject { v8::Local unused, v8::Local context); + static void GetAsyncId(const v8::FunctionCallbackInfo& args); + static void DestroyIdsCb(uv_idle_t* handle); inline ProviderType provider_type() const; From f5d6056c0bcc231915f31af7f9613c6e73121345 Mon Sep 17 00:00:00 2001 From: Trevor Norris Date: Wed, 28 Sep 2016 16:48:34 -0600 Subject: [PATCH 07/40] src: add AsyncWrap::GetAsyncId() to all children Add the method AsyncWrap::GetAsyncId() to all inheriting class objects so the uid of the handle can be retrieved from JS. --- src/cares_wrap.cc | 3 +++ src/fs_event_wrap.cc | 1 + src/js_stream.cc | 2 ++ src/node_crypto.cc | 3 +++ src/node_file.cc | 1 + src/node_http_parser.cc | 1 + src/node_stat_watcher.cc | 1 + src/node_zlib.cc | 1 + src/pipe_wrap.cc | 3 +++ src/process_wrap.cc | 2 ++ src/signal_wrap.cc | 1 + src/stream_wrap.cc | 2 ++ src/tcp_wrap.cc | 2 ++ src/timer_wrap.cc | 2 ++ src/tls_wrap.cc | 1 + src/tty_wrap.cc | 2 ++ src/udp_wrap.cc | 3 +++ 17 files changed, 31 insertions(+) diff --git a/src/cares_wrap.cc b/src/cares_wrap.cc index 2b61209f6e643a..c571cccea8f1ef 100644 --- a/src/cares_wrap.cc +++ b/src/cares_wrap.cc @@ -1381,6 +1381,7 @@ static void Initialize(Local target, Local aiw = FunctionTemplate::New(env->isolate(), NewGetAddrInfoReqWrap); aiw->InstanceTemplate()->SetInternalFieldCount(1); + env->SetProtoMethod(aiw, "getAsyncId", AsyncWrap::GetAsyncId); aiw->SetClassName( FIXED_ONE_BYTE_STRING(env->isolate(), "GetAddrInfoReqWrap")); target->Set(FIXED_ONE_BYTE_STRING(env->isolate(), "GetAddrInfoReqWrap"), @@ -1389,6 +1390,7 @@ static void Initialize(Local target, Local niw = FunctionTemplate::New(env->isolate(), NewGetNameInfoReqWrap); niw->InstanceTemplate()->SetInternalFieldCount(1); + env->SetProtoMethod(niw, "getAsyncId", AsyncWrap::GetAsyncId); niw->SetClassName( FIXED_ONE_BYTE_STRING(env->isolate(), "GetNameInfoReqWrap")); target->Set(FIXED_ONE_BYTE_STRING(env->isolate(), "GetNameInfoReqWrap"), @@ -1397,6 +1399,7 @@ static void Initialize(Local target, Local qrw = FunctionTemplate::New(env->isolate(), NewQueryReqWrap); qrw->InstanceTemplate()->SetInternalFieldCount(1); + env->SetProtoMethod(qrw, "getAsyncId", AsyncWrap::GetAsyncId); qrw->SetClassName( FIXED_ONE_BYTE_STRING(env->isolate(), "QueryReqWrap")); target->Set(FIXED_ONE_BYTE_STRING(env->isolate(), "QueryReqWrap"), diff --git a/src/fs_event_wrap.cc b/src/fs_event_wrap.cc index 025f511d93b1a5..80038dea4f649c 100644 --- a/src/fs_event_wrap.cc +++ b/src/fs_event_wrap.cc @@ -70,6 +70,7 @@ void FSEventWrap::Initialize(Local target, t->InstanceTemplate()->SetInternalFieldCount(1); t->SetClassName(fsevent_string); + env->SetProtoMethod(t, "getAsyncId", AsyncWrap::GetAsyncId); env->SetProtoMethod(t, "start", Start); env->SetProtoMethod(t, "close", Close); diff --git a/src/js_stream.cc b/src/js_stream.cc index e51c4ae9b35084..1d20e1c6d77dfb 100644 --- a/src/js_stream.cc +++ b/src/js_stream.cc @@ -221,6 +221,8 @@ void JSStream::Initialize(Local target, t->SetClassName(FIXED_ONE_BYTE_STRING(env->isolate(), "JSStream")); t->InstanceTemplate()->SetInternalFieldCount(1); + env->SetProtoMethod(t, "getAsyncId", AsyncWrap::GetAsyncId); + env->SetProtoMethod(t, "doAlloc", DoAlloc); env->SetProtoMethod(t, "doRead", DoRead); env->SetProtoMethod(t, "doAfterWrite", DoAfterWrite); diff --git a/src/node_crypto.cc b/src/node_crypto.cc index 4a6d16e5416624..c62de0aabf9421 100644 --- a/src/node_crypto.cc +++ b/src/node_crypto.cc @@ -2719,6 +2719,7 @@ void Connection::Initialize(Environment* env, Local target) { t->InstanceTemplate()->SetInternalFieldCount(1); t->SetClassName(FIXED_ONE_BYTE_STRING(env->isolate(), "Connection")); + env->SetProtoMethod(t, "getAsyncId", AsyncWrap::GetAsyncId); env->SetProtoMethod(t, "encIn", Connection::EncIn); env->SetProtoMethod(t, "clearOut", Connection::ClearOut); env->SetProtoMethod(t, "clearIn", Connection::ClearIn); @@ -6025,12 +6026,14 @@ void InitCrypto(Local target, Local pb = FunctionTemplate::New(env->isolate()); pb->SetClassName(FIXED_ONE_BYTE_STRING(env->isolate(), "PBKDF2")); + env->SetProtoMethod(pb, "getAsyncId", AsyncWrap::GetAsyncId); Local pbt = pb->InstanceTemplate(); pbt->SetInternalFieldCount(1); env->set_pbkdf2_constructor_template(pbt); Local rb = FunctionTemplate::New(env->isolate()); rb->SetClassName(FIXED_ONE_BYTE_STRING(env->isolate(), "RandomBytes")); + env->SetProtoMethod(rb, "getAsyncId", AsyncWrap::GetAsyncId); Local rbt = rb->InstanceTemplate(); rbt->SetInternalFieldCount(1); env->set_randombytes_constructor_template(rbt); diff --git a/src/node_file.cc b/src/node_file.cc index 0abb88088786ae..544512dc7a7715 100644 --- a/src/node_file.cc +++ b/src/node_file.cc @@ -1495,6 +1495,7 @@ void InitFs(Local target, Local fst = FunctionTemplate::New(env->isolate(), NewFSReqWrap); fst->InstanceTemplate()->SetInternalFieldCount(1); + env->SetProtoMethod(fst, "getAsyncId", AsyncWrap::GetAsyncId); fst->SetClassName(FIXED_ONE_BYTE_STRING(env->isolate(), "FSReqWrap")); target->Set(FIXED_ONE_BYTE_STRING(env->isolate(), "FSReqWrap"), fst->GetFunction()); diff --git a/src/node_http_parser.cc b/src/node_http_parser.cc index f757cd6797058d..6d759639275ce4 100644 --- a/src/node_http_parser.cc +++ b/src/node_http_parser.cc @@ -763,6 +763,7 @@ void InitHttpParser(Local target, #undef V target->Set(FIXED_ONE_BYTE_STRING(env->isolate(), "methods"), methods); + env->SetProtoMethod(t, "getAsyncId", AsyncWrap::GetAsyncId); env->SetProtoMethod(t, "close", Parser::Close); env->SetProtoMethod(t, "execute", Parser::Execute); env->SetProtoMethod(t, "finish", Parser::Finish); diff --git a/src/node_stat_watcher.cc b/src/node_stat_watcher.cc index cff92e34045a1e..b79313c0e60e2c 100644 --- a/src/node_stat_watcher.cc +++ b/src/node_stat_watcher.cc @@ -28,6 +28,7 @@ void StatWatcher::Initialize(Environment* env, Local target) { t->InstanceTemplate()->SetInternalFieldCount(1); t->SetClassName(FIXED_ONE_BYTE_STRING(env->isolate(), "StatWatcher")); + env->SetProtoMethod(t, "getAsyncId", AsyncWrap::GetAsyncId); env->SetProtoMethod(t, "start", StatWatcher::Start); env->SetProtoMethod(t, "stop", StatWatcher::Stop); diff --git a/src/node_zlib.cc b/src/node_zlib.cc index a47ab45bfdc62c..678200494ece06 100644 --- a/src/node_zlib.cc +++ b/src/node_zlib.cc @@ -658,6 +658,7 @@ void InitZlib(Local target, z->InstanceTemplate()->SetInternalFieldCount(1); + env->SetProtoMethod(z, "getAsyncId", AsyncWrap::GetAsyncId); env->SetProtoMethod(z, "write", ZCtx::Write); env->SetProtoMethod(z, "writeSync", ZCtx::Write); env->SetProtoMethod(z, "init", ZCtx::Init); diff --git a/src/pipe_wrap.cc b/src/pipe_wrap.cc index 5f47dadddb4d96..f41fc8dc80e505 100644 --- a/src/pipe_wrap.cc +++ b/src/pipe_wrap.cc @@ -48,6 +48,8 @@ void PipeWrap::Initialize(Local target, t->SetClassName(FIXED_ONE_BYTE_STRING(env->isolate(), "Pipe")); t->InstanceTemplate()->SetInternalFieldCount(1); + env->SetProtoMethod(t, "getAsyncId", AsyncWrap::GetAsyncId); + env->SetProtoMethod(t, "close", HandleWrap::Close); env->SetProtoMethod(t, "unref", HandleWrap::Unref); env->SetProtoMethod(t, "ref", HandleWrap::Ref); @@ -77,6 +79,7 @@ void PipeWrap::Initialize(Local target, }; auto cwt = FunctionTemplate::New(env->isolate(), constructor); cwt->InstanceTemplate()->SetInternalFieldCount(1); + env->SetProtoMethod(cwt, "getAsyncId", AsyncWrap::GetAsyncId); cwt->SetClassName(FIXED_ONE_BYTE_STRING(env->isolate(), "PipeConnectWrap")); target->Set(FIXED_ONE_BYTE_STRING(env->isolate(), "PipeConnectWrap"), cwt->GetFunction()); diff --git a/src/process_wrap.cc b/src/process_wrap.cc index 8c8e4704be4f82..3bc0bb7b21906a 100644 --- a/src/process_wrap.cc +++ b/src/process_wrap.cc @@ -32,6 +32,8 @@ class ProcessWrap : public HandleWrap { constructor->InstanceTemplate()->SetInternalFieldCount(1); constructor->SetClassName(FIXED_ONE_BYTE_STRING(env->isolate(), "Process")); + env->SetProtoMethod(constructor, "getAsyncId", AsyncWrap::GetAsyncId); + env->SetProtoMethod(constructor, "close", HandleWrap::Close); env->SetProtoMethod(constructor, "spawn", Spawn); diff --git a/src/signal_wrap.cc b/src/signal_wrap.cc index 582d1a9ecfdc02..7792c11aec60ec 100644 --- a/src/signal_wrap.cc +++ b/src/signal_wrap.cc @@ -28,6 +28,7 @@ class SignalWrap : public HandleWrap { constructor->InstanceTemplate()->SetInternalFieldCount(1); constructor->SetClassName(FIXED_ONE_BYTE_STRING(env->isolate(), "Signal")); + env->SetProtoMethod(constructor, "getAsyncId", AsyncWrap::GetAsyncId); env->SetProtoMethod(constructor, "close", HandleWrap::Close); env->SetProtoMethod(constructor, "ref", HandleWrap::Ref); env->SetProtoMethod(constructor, "unref", HandleWrap::Unref); diff --git a/src/stream_wrap.cc b/src/stream_wrap.cc index ac656505503b22..563d2e4b28f943 100644 --- a/src/stream_wrap.cc +++ b/src/stream_wrap.cc @@ -42,6 +42,7 @@ void StreamWrap::Initialize(Local target, FunctionTemplate::New(env->isolate(), ShutdownWrap::NewShutdownWrap); sw->InstanceTemplate()->SetInternalFieldCount(1); sw->SetClassName(FIXED_ONE_BYTE_STRING(env->isolate(), "ShutdownWrap")); + env->SetProtoMethod(sw, "getAsyncId", AsyncWrap::GetAsyncId); target->Set(FIXED_ONE_BYTE_STRING(env->isolate(), "ShutdownWrap"), sw->GetFunction()); @@ -49,6 +50,7 @@ void StreamWrap::Initialize(Local target, FunctionTemplate::New(env->isolate(), WriteWrap::NewWriteWrap); ww->InstanceTemplate()->SetInternalFieldCount(1); ww->SetClassName(FIXED_ONE_BYTE_STRING(env->isolate(), "WriteWrap")); + env->SetProtoMethod(ww, "getAsyncId", AsyncWrap::GetAsyncId); target->Set(FIXED_ONE_BYTE_STRING(env->isolate(), "WriteWrap"), ww->GetFunction()); env->set_write_wrap_constructor_function(ww->GetFunction()); diff --git a/src/tcp_wrap.cc b/src/tcp_wrap.cc index b2617a5695719e..8cb7e5aa1af4bd 100644 --- a/src/tcp_wrap.cc +++ b/src/tcp_wrap.cc @@ -63,6 +63,7 @@ void TCPWrap::Initialize(Local target, "onconnection"), Null(env->isolate())); + env->SetProtoMethod(t, "getAsyncId", AsyncWrap::GetAsyncId); env->SetProtoMethod(t, "close", HandleWrap::Close); @@ -98,6 +99,7 @@ void TCPWrap::Initialize(Local target, }; auto cwt = FunctionTemplate::New(env->isolate(), constructor); cwt->InstanceTemplate()->SetInternalFieldCount(1); + env->SetProtoMethod(cwt, "getAsyncId", AsyncWrap::GetAsyncId); cwt->SetClassName(FIXED_ONE_BYTE_STRING(env->isolate(), "TCPConnectWrap")); target->Set(FIXED_ONE_BYTE_STRING(env->isolate(), "TCPConnectWrap"), cwt->GetFunction()); diff --git a/src/timer_wrap.cc b/src/timer_wrap.cc index 843fde4673b071..497d156cd4754d 100644 --- a/src/timer_wrap.cc +++ b/src/timer_wrap.cc @@ -35,6 +35,8 @@ class TimerWrap : public HandleWrap { env->SetTemplateMethod(constructor, "now", Now); + env->SetProtoMethod(constructor, "getAsyncId", AsyncWrap::GetAsyncId); + env->SetProtoMethod(constructor, "close", HandleWrap::Close); env->SetProtoMethod(constructor, "ref", HandleWrap::Ref); env->SetProtoMethod(constructor, "unref", HandleWrap::Unref); diff --git a/src/tls_wrap.cc b/src/tls_wrap.cc index 505cadee8faa81..d089dab66c05a6 100644 --- a/src/tls_wrap.cc +++ b/src/tls_wrap.cc @@ -912,6 +912,7 @@ void TLSWrap::Initialize(Local target, t->InstanceTemplate()->SetInternalFieldCount(1); t->SetClassName(FIXED_ONE_BYTE_STRING(env->isolate(), "TLSWrap")); + env->SetProtoMethod(t, "getAsyncId", AsyncWrap::GetAsyncId); env->SetProtoMethod(t, "receive", Receive); env->SetProtoMethod(t, "start", Start); env->SetProtoMethod(t, "setVerifyMode", SetVerifyMode); diff --git a/src/tty_wrap.cc b/src/tty_wrap.cc index 26f061b99b34e8..ab90aa587d7cd3 100644 --- a/src/tty_wrap.cc +++ b/src/tty_wrap.cc @@ -32,6 +32,8 @@ void TTYWrap::Initialize(Local target, t->SetClassName(FIXED_ONE_BYTE_STRING(env->isolate(), "TTY")); t->InstanceTemplate()->SetInternalFieldCount(1); + env->SetProtoMethod(t, "getAsyncId", AsyncWrap::GetAsyncId); + env->SetProtoMethod(t, "close", HandleWrap::Close); env->SetProtoMethod(t, "unref", HandleWrap::Unref); env->SetProtoMethod(t, "hasRef", HandleWrap::HasRef); diff --git a/src/udp_wrap.cc b/src/udp_wrap.cc index 61884623755d48..50f28881394caa 100644 --- a/src/udp_wrap.cc +++ b/src/udp_wrap.cc @@ -108,6 +108,8 @@ void UDPWrap::Initialize(Local target, env->SetProtoMethod(t, "unref", HandleWrap::Unref); env->SetProtoMethod(t, "hasRef", HandleWrap::HasRef); + env->SetProtoMethod(t, "getAsyncId", AsyncWrap::GetAsyncId); + target->Set(FIXED_ONE_BYTE_STRING(env->isolate(), "UDP"), t->GetFunction()); env->set_udp_constructor_function(t->GetFunction()); @@ -115,6 +117,7 @@ void UDPWrap::Initialize(Local target, Local swt = FunctionTemplate::New(env->isolate(), NewSendWrap); swt->InstanceTemplate()->SetInternalFieldCount(1); + env->SetProtoMethod(swt, "getAsyncId", AsyncWrap::GetAsyncId); swt->SetClassName(FIXED_ONE_BYTE_STRING(env->isolate(), "SendWrap")); target->Set(FIXED_ONE_BYTE_STRING(env->isolate(), "SendWrap"), swt->GetFunction()); From 1c9154ccc2509f39052887307b24b613b8115aad Mon Sep 17 00:00:00 2001 From: Trevor Norris Date: Wed, 2 Nov 2016 16:46:13 -0600 Subject: [PATCH 08/40] async_wrap: only call SetupHooks() once The call to async_wrap.setupHooks() will soon become an internal call. While other public APIs will be exposed through async_hooks. In preparation for this, only allow SetupHooks() to be called once. If called again, or if a non-function is passed, throw. --- src/async-wrap.cc | 47 ++++++++++++++++++++++++++--------------------- src/env.h | 4 ++-- src/node.cc | 12 ++++++------ 3 files changed, 34 insertions(+), 29 deletions(-) diff --git a/src/async-wrap.cc b/src/async-wrap.cc index 8af2159b4255a9..242c3f871da606 100644 --- a/src/async-wrap.cc +++ b/src/async-wrap.cc @@ -130,32 +130,37 @@ static void SetupHooks(const FunctionCallbackInfo& args) { if (!args[0]->IsObject()) return env->ThrowTypeError("first argument must be an object"); + // All of init, before, after, destroy are supplied by async_hooks + // internally, so this should every only be called once. At which time all + // the functions should be set. Detect this by checking if init !IsEmpty() + // and returning early if that's the case. + if (!env->async_hooks_init_function().IsEmpty()) + return env->ThrowError("async_hook callbacks have already been setup"); + Local fn_obj = args[0].As(); Local init_v = fn_obj->Get( env->context(), FIXED_ONE_BYTE_STRING(env->isolate(), "init")).ToLocalChecked(); - Local pre_v = fn_obj->Get( + Local before_v = fn_obj->Get( env->context(), - FIXED_ONE_BYTE_STRING(env->isolate(), "pre")).ToLocalChecked(); - Local post_v = fn_obj->Get( + FIXED_ONE_BYTE_STRING(env->isolate(), "before")).ToLocalChecked(); + Local after_v = fn_obj->Get( env->context(), - FIXED_ONE_BYTE_STRING(env->isolate(), "post")).ToLocalChecked(); + FIXED_ONE_BYTE_STRING(env->isolate(), "after")).ToLocalChecked(); Local destroy_v = fn_obj->Get( env->context(), FIXED_ONE_BYTE_STRING(env->isolate(), "destroy")).ToLocalChecked(); - if (!init_v->IsFunction()) - return env->ThrowTypeError("init callback must be a function"); + if (!init_v->IsFunction() || !before_v->IsFunction() || + !after_v->IsFunction() || !destroy_v->IsFunction()) { + return env->ThrowTypeError("all callbacks must be functions"); + } env->set_async_hooks_init_function(init_v.As()); - - if (pre_v->IsFunction()) - env->set_async_hooks_pre_function(pre_v.As()); - if (post_v->IsFunction()) - env->set_async_hooks_post_function(post_v.As()); - if (destroy_v->IsFunction()) - env->set_async_hooks_destroy_function(destroy_v.As()); + env->set_async_hooks_before_function(before_v.As()); + env->set_async_hooks_after_function(after_v.As()); + env->set_async_hooks_destroy_function(destroy_v.As()); } @@ -179,8 +184,8 @@ void AsyncWrap::Initialize(Local target, target->Set(FIXED_ONE_BYTE_STRING(isolate, "Providers"), async_providers); env->set_async_hooks_init_function(Local()); - env->set_async_hooks_pre_function(Local()); - env->set_async_hooks_post_function(Local()); + env->set_async_hooks_before_function(Local()); + env->set_async_hooks_after_function(Local()); env->set_async_hooks_destroy_function(Local()); } @@ -307,8 +312,8 @@ Local AsyncWrap::MakeCallback(const Local cb, Local* argv) { CHECK(env()->context() == env()->isolate()->GetCurrentContext()); - Local pre_fn = env()->async_hooks_pre_function(); - Local post_fn = env()->async_hooks_post_function(); + Local before_fn = env()->async_hooks_before_function(); + Local after_fn = env()->async_hooks_after_function(); Local uid = Number::New(env()->isolate(), get_id()); Local context = object(); Local domain; @@ -336,9 +341,9 @@ Local AsyncWrap::MakeCallback(const Local cb, } } - if (ran_init_callback() && !pre_fn.IsEmpty()) { + if (ran_init_callback() && !before_fn.IsEmpty()) { TryCatch try_catch(env()->isolate()); - MaybeLocal ar = pre_fn->Call(env()->context(), context, 1, &uid); + MaybeLocal ar = before_fn->Call(env()->context(), context, 1, &uid); if (ar.IsEmpty()) { ClearFatalExceptionHandlers(env()); FatalException(env()->isolate(), try_catch); @@ -348,12 +353,12 @@ Local AsyncWrap::MakeCallback(const Local cb, Local ret = cb->Call(context, argc, argv); - if (ran_init_callback() && !post_fn.IsEmpty()) { + if (ran_init_callback() && !after_fn.IsEmpty()) { Local did_throw = Boolean::New(env()->isolate(), ret.IsEmpty()); Local vals[] = { uid, did_throw }; TryCatch try_catch(env()->isolate()); MaybeLocal ar = - post_fn->Call(env()->context(), context, arraysize(vals), vals); + after_fn->Call(env()->context(), context, arraysize(vals), vals); if (ar.IsEmpty()) { ClearFatalExceptionHandlers(env()); FatalException(env()->isolate(), try_catch); diff --git a/src/env.h b/src/env.h index 8683286faf360b..2ce8e8ea947900 100644 --- a/src/env.h +++ b/src/env.h @@ -231,8 +231,8 @@ namespace node { V(as_external, v8::External) \ V(async_hooks_destroy_function, v8::Function) \ V(async_hooks_init_function, v8::Function) \ - V(async_hooks_post_function, v8::Function) \ - V(async_hooks_pre_function, v8::Function) \ + V(async_hooks_before_function, v8::Function) \ + V(async_hooks_after_function, v8::Function) \ V(binding_cache_object, v8::Object) \ V(buffer_constructor_function, v8::Function) \ V(buffer_prototype_object, v8::Object) \ diff --git a/src/node.cc b/src/node.cc index 57293989029331..f3e8a6494e75df 100644 --- a/src/node.cc +++ b/src/node.cc @@ -1179,8 +1179,8 @@ Local MakeCallback(Environment* env, // If you hit this assertion, you forgot to enter the v8::Context first. CHECK_EQ(env->context(), env->isolate()->GetCurrentContext()); - Local pre_fn = env->async_hooks_pre_function(); - Local post_fn = env->async_hooks_post_function(); + Local before_fn = env->async_hooks_before_function(); + Local after_fn = env->async_hooks_after_function(); Local object, domain; bool ran_init_callback = false; bool has_domain = false; @@ -1217,9 +1217,9 @@ Local MakeCallback(Environment* env, } } - if (ran_init_callback && !pre_fn.IsEmpty()) { + if (ran_init_callback && !before_fn.IsEmpty()) { TryCatch try_catch(env->isolate()); - MaybeLocal ar = pre_fn->Call(env->context(), object, 0, nullptr); + MaybeLocal ar = before_fn->Call(env->context(), object, 0, nullptr); if (ar.IsEmpty()) { ClearFatalExceptionHandlers(env); FatalException(env->isolate(), try_catch); @@ -1229,7 +1229,7 @@ Local MakeCallback(Environment* env, Local ret = callback->Call(recv, argc, argv); - if (ran_init_callback && !post_fn.IsEmpty()) { + if (ran_init_callback && !after_fn.IsEmpty()) { Local did_throw = Boolean::New(env->isolate(), ret.IsEmpty()); // Currently there's no way to retrieve an uid from node::MakeCallback(). // This needs to be fixed. @@ -1237,7 +1237,7 @@ Local MakeCallback(Environment* env, { Undefined(env->isolate()).As(), did_throw }; TryCatch try_catch(env->isolate()); MaybeLocal ar = - post_fn->Call(env->context(), object, arraysize(vals), vals); + after_fn->Call(env->context(), object, arraysize(vals), vals); if (ar.IsEmpty()) { ClearFatalExceptionHandlers(env); FatalException(env->isolate(), try_catch); From 4c809dde62e11561f9bf724200a13f317d851b92 Mon Sep 17 00:00:00 2001 From: Trevor Norris Date: Wed, 2 Nov 2016 16:30:21 -0600 Subject: [PATCH 09/40] async_hooks: introduce async_hooks.js Getting mechanics flushed out for both the necessary JS and C++ APIs. Currenty builds and runs, but complete implementation is coming. --- lib/_stream_wrap.js | 2 + lib/async_hooks.js | 400 ++++++++++++++++++++++++++++++++++++++++ lib/internal/module.js | 8 +- node.gyp | 1 + src/async-wrap-inl.h | 12 +- src/async-wrap.cc | 285 +++++++++++++++++----------- src/async-wrap.h | 31 ++-- src/cares_wrap.cc | 13 ++ src/connect_wrap.cc | 5 + src/connect_wrap.h | 1 + src/connection_wrap.cc | 12 +- src/connection_wrap.h | 3 +- src/env-inl.h | 72 ++++++-- src/env.h | 107 +++++++++-- src/handle_wrap.cc | 5 +- src/handle_wrap.h | 3 +- src/js_stream.cc | 17 +- src/js_stream.h | 2 +- src/node.cc | 54 ++---- src/node_file.cc | 41 +++- src/node_http_parser.cc | 2 + src/pipe_wrap.cc | 19 +- src/pipe_wrap.h | 3 +- src/req-wrap-inl.h | 1 - src/stream_base-inl.h | 3 + src/stream_base.cc | 13 ++ src/stream_base.h | 8 + src/stream_wrap.cc | 6 +- src/stream_wrap.h | 3 +- src/tcp_wrap.cc | 25 +-- src/tcp_wrap.h | 2 +- src/udp_wrap.cc | 29 +-- src/udp_wrap.h | 2 +- 33 files changed, 908 insertions(+), 282 deletions(-) create mode 100644 lib/async_hooks.js diff --git a/lib/_stream_wrap.js b/lib/_stream_wrap.js index fbc32965980e96..1367818ad7ba93 100644 --- a/lib/_stream_wrap.js +++ b/lib/_stream_wrap.js @@ -9,6 +9,8 @@ const uv = process.binding('uv'); const debug = util.debuglog('stream_wrap'); function StreamWrap(stream) { + // TODO(trevnorris): Track down everything that calls StreamWrap and make + // sure the trigger id is correctly set. const handle = new JSStream(); this.stream = stream; diff --git a/lib/async_hooks.js b/lib/async_hooks.js new file mode 100644 index 00000000000000..ebea6c85fba9e7 --- /dev/null +++ b/lib/async_hooks.js @@ -0,0 +1,400 @@ +'use strict'; + +const async_wrap = process.binding('async_wrap'); +// A Uint32Array wrapping node::Environment::AsyncHooks::fields_. An array that +// communicates between JS and C++ how many of a given type of callback are +// available to be called. +const async_hook_fields = async_wrap.async_hook_fields; +// A Float64Array wrapping node::Environment::AsyncHooks::uid_fields_. An array +// that communicates the state of the currentId and triggerId. Fields are as +// follows: +// kAsyncUidCntr: Maintain state of next unique id. +// kCurrentId: Read/write the id of the current execution context. +// kTriggerId: Read/write the id of the resource responsible for the current +// execution context firing. +// kInitTriggerId: Written to just before creating a new resource, so the +// constructor knows what other resource is responsible for its init(). +// kScopedTriggerId: Hold the init triggerId for all constructors that run +// within triggerIdScope(). +const async_uid_fields = async_wrap.async_uid_fields; +// Array of all AsyncHooks that will be iterated whenever an async event fires. +const active_hooks_array = []; +// Stack of scoped trigger id's for triggerIdScope() +const trigger_scope_stack = []; +// Array that holds the current and trigger id's for between before/after. +const current_trigger_id_stack = []; + +const async_id_symbol = Symbol('_asyncId'); +const trigger_id_symbol = Symbol('_triggerId'); +const init_symbol = Symbol('init'); +const before_symbol = Symbol('before'); +const after_symbol = Symbol('after'); +const destroy_symbol = Symbol('destroy'); + +// Each constant tracks how many callbacks there are for any given step of +// async execution. These are tracked so if the user didn't include callbacks +// for a given step, that step can bail out early. +// The exception is kActiveHooks. Which tracks the total number of AsyncEvents +// that exist on "active_hooks_array". +const { kInit, kBefore, kAfter, kDestroy, kActiveHooks, kAsyncUidCntr, + kCurrentId, kTriggerId, kScopedTriggerId, kInitTriggerId } = + async_wrap.constants; + +// Setup the callbacks that node::AsyncWrap will call when there are hooks to +// process. They use the same functions as the JS embedder API. +async_wrap.setupHooks({ init, + before: emitBeforeS, + after: emitAfterS, + destroy: emitDestroyS }); + + +// Public API // + +class AsyncHook { + constructor(fns) { + this[init_symbol] = fns.init; + this[before_symbol] = fns.before; + this[after_symbol] = fns.after; + this[destroy_symbol] = fns.destroy; + } + + // TODO(trevnorris): Both enable and disable need to check if 1) callbacks + // are in the middle of being processed or 2) async destroy's are queue'd + // to be called. In either case a copy of active_hooks_array needs to be + // duplicated then replaced with the altered array once the operation is + // complete. + enable() { + if (active_hooks_array.indexOf(this) !== -1) + return; + // createHook() has already enforced that the callbacks are all functions, + // so here simply increment the count of whether each callbacks exists or + // not. + async_hook_fields[kInit] += +!!this[init_symbol]; + async_hook_fields[kBefore] += +!!this[before_symbol]; + async_hook_fields[kAfter] += +!!this[after_symbol]; + async_hook_fields[kDestroy] += +!!this[destroy_symbol]; + async_hook_fields[kActiveHooks]++; + active_hooks_array.push(this); + return this; + } + + disable() { + const index = active_hooks_array.indexOf(this); + if (index === -1) + return; + async_hook_fields[kInit] -= +!!this[init_symbol]; + async_hook_fields[kBefore] -= +!!this[before_symbol]; + async_hook_fields[kAfter] -= +!!this[after_symbol]; + async_hook_fields[kDestroy] -= +!!this[destroy_symbol]; + async_hook_fields[kActiveHooks]--; + active_hooks_array.splice(index, 1); + return this; + } +} + + +function createHook(fns) { + if (fns.init !== undefined && typeof fns.init !== 'function') + throw new TypeError('init must be a function'); + if (fns.before !== undefined && typeof fns.before !== 'function') + throw new TypeError('before must be a function'); + if (fns.after !== undefined && typeof fns.after !== 'function') + throw new TypeError('after must be a function'); + if (fns.destroy !== undefined && typeof fns.destroy !== 'function') + throw new TypeError('destroy must be a function'); + + return new AsyncHook(fns); +} + + +function currentId() { + return async_uid_fields[kCurrentId]; +} + + +function triggerId() { + return async_uid_fields[kTriggerId]; +} + + +// Embedder API // + +class AsyncEvent { + constructor(type, triggerId) { + this[async_id_symbol] = ++async_uid_fields[kAsyncUidCntr]; + // Do this before early return to null any potential kInitTriggerId. + if (triggerId === undefined) + triggerId = initTriggerId(); + // If a triggerId was passed, any kInitTriggerId still must be null'd. + else + async_uid_fields[kInitTriggerId] = 0; + this[trigger_id_symbol] = triggerId; + + // Return immediately if there's nothing to do. Checking for kInit covers + // checking for kActiveHooks. Doing this before the checks for performance. + if (async_hook_fields[kInit] === 0) + return; + + if (typeof type !== 'string' || type.length <= 0) + throw new TypeError('type must be a string with length > 0'); + if (!Number.isSafeInteger(triggerId) || triggerId < 0) + throw new RangeError('triggerId must be an unsigned integer'); + + for (var i = 0; i < active_hooks_array.length; i++) { + if (typeof active_hooks_array[i][init_symbol] === 'function') { + runInitCallback(active_hooks_array[i][init_symbol], + this[async_id_symbol], + type, + triggerId, + this); + } + } + } + + emitBefore() { + emitBeforeS(this[async_id_symbol], this[trigger_id_symbol]); + return this; + } + + emitAfter() { + emitAfterS(this[async_id_symbol]); + return this; + } + + emitDestroy() { + emitDestroyS(this[async_id_symbol]); + return this; + } + + asyncId() { + return this[async_id_symbol]; + } + + triggerId() { + return this[trigger_id_symbol]; + } +} + + +function triggerIdScope(id, cb) { + if (async_uid_fields[kScopedTriggerId] > 0) + trigger_scope_stack.push(async_uid_fields[kScopedTriggerId]); + try { + cb(); + } finally { + if (trigger_scope_stack.length > 0) + trigger_scope_stack.pop(); + } +} + + +// Sensitive Embedder API // + +// Increment the internal id counter and return the value. Important that the +// counter increment first. Since it's done the same way in +// Environment::new_async_uid() +function newUid() { + return ++async_uid_fields[kAsyncUidCntr]; +} + + +// Return the triggerId meant for the constructor calling it. It's up to the +// user to safeguard this call and make sure it's zero'd out when the +// constructor is complete. +function initTriggerId() { + var tId = async_uid_fields[kInitTriggerId]; + if (tId <= 0) tId = async_uid_fields[kScopedTriggerId]; + // Reset value after it's been called so the next constructor doesn't + // inherit it by accident. + else async_uid_fields[kInitTriggerId] = 0; + if (tId < 0) tId = async_uid_fields[kCurrentId]; + return tId; +} + + +function setInitTriggerId(id) { + async_uid_fields[kInitTriggerId] = id; +} + + +// Usage: +// emitInit(number, string[, number[, handle]]) +// +// Short circuit all checks for the common case. Which is that no hooks have +// been set. Do this to remove performance impact for embedders (and core). +// Even though it bypasses all the argument checks. The performance savings +// here is critical. +// +// Note: All the emit* below end with "S" so they can be called by the methods +// on AsyncEvent, but are not exported that way. +function emitInitS(id, type, triggerId, handle) { + // Return immediately if there's nothing to do. Checking for kInit covers + // checking for kActiveHooks. Doing this before the checks for performance. + if (async_hook_fields[kInit] === 0) + return; + + // This can run after the early return check b/c running this function + // manually means that the embedder must have used initTriggerId(). + if (triggerId === undefined) + triggerId = initTriggerId(); + + // I'd prefer allowing these checks to not exist, or only throw in a debug + // build, to improve performance. + if (!Number.isSafeInteger(id) || id < 0) + throw new RangeError('id must be an unsigned integer'); + if (typeof type !== 'string' || type.length <= 0) + throw new TypeError('type must be a string with length > 0'); + if (!Number.isSafeInteger(triggerId) || triggerId < 0) + throw new RangeError('triggerId must be an unsigned integer'); + + for (var i = 0; i < active_hooks_array.length; i++) { + if (typeof active_hooks_array[i][init_symbol] === 'function') { + runInitCallback( + active_hooks_array[i][init_symbol], id, type, triggerId, handle); + } + } +} + + +// Usage: emitBeforeS(id[, triggerId]). If triggerId is omitted then id will be +// used instead. +function emitBeforeS(id, triggerId) { + // First setup the currentId and triggerId for the coming callback. + const currentCurrentId = async_uid_fields[kCurrentId]; + const currentTriggerId = async_uid_fields[kTriggerId]; + if (currentCurrentId > 0 || currentTriggerId > 0 || + current_trigger_id_stack.length > 0) { + current_trigger_id_stack.push(currentCurrentId, currentTriggerId); + } + async_uid_fields[kCurrentId] = id; + async_uid_fields[kTriggerId] = triggerId === undefined ? id : triggerId; + + if (active_hooks_array[kBefore] === 0) { + return; + } + for (var i = 0; i < active_hooks_array.length; i++) { + if (typeof active_hooks_array[i][before_symbol] === 'function') { + runCallback(active_hooks_array[i][before_symbol], id); + } + } +} + + +function emitAfterS(id) { + // Remove state after the call has completed. + // TODO(trevnorris): Clearing the native counters is easy, but also must + // track the length of the array manually so the native side can empty it + // in case there's an error. Or, export the array in such a way that + // fatalException can empty it as well. + if (current_trigger_id_stack.length > 0) { + async_uid_fields[kTriggerId] = current_trigger_id_stack.pop(); + async_uid_fields[kCurrentId] = current_trigger_id_stack.pop(); + } else { + async_uid_fields[kTriggerId] = 0; + async_uid_fields[kCurrentId] = 0; + } + + if (active_hooks_array[kAfter] === 0) { + return; + } + for (var i = 0; i < active_hooks_array.length; i++) { + if (typeof active_hooks_array[i][after_symbol] === 'function') { + runCallback(active_hooks_array[i][after_symbol], id); + } + } +} + + +// TODO(trevnorris): If these are delayed until ran then we will have a list +// of id's that may need to run. So instead of forcing native to call into JS +// for every handle, instead have this call back into C++ for the next id. +function emitDestroyS(id) { + if (active_hooks_array[kDestroy] === 0) { + return; + } + for (var i = 0; i < active_hooks_array.length; i++) { + if (typeof active_hooks_array[i][destroy_symbol] === 'function') { + runCallback(active_hooks_array[i][destroy_symbol], id); + } + } +} + + +// Emit callbacks for native calls. Since some state can be setup directly from +// C++ there's no need to perform all the work here. + +// This should only be called if hooks_array has kInit > 0. There are no global +// values to setup. Though hooks_array will be cloned if C++ needed to visit +// here since it's faster to do in JS. +// TODO(trevnorris): Perhaps have MakeCallback call a single JS function that +// does the before/callback/after calls to remove two additional calls to JS. +function init(id, type, handle) { + // Use initTriggerId() here so that native versions of initTriggerId() and + // triggerIdScope(), or so any triggerId set in JS propagates properly. + const triggerId = initTriggerId(); + for (var i = 0; i < active_hooks_array.length; i++) { + if (typeof active_hooks_array[i][init_symbol] === 'function') { + runInitCallback( + active_hooks_array[i][init_symbol], id, type, triggerId, handle); + } + } +} + + +// Generalized caller for all callbacks that handles error handling. + +function runInitCallback(cb, id, type, triggerId, handle) { + var fail = true; + try { + cb(id, type, triggerId, handle); + fail = false; + } finally { + // Force the application to shutdown if one of the callbacks throws. This + // may change in the future depending on whether it can be determined if + // there's a slim chance of the application remaining stable after handling + // one of these exceptions. + if (fail) { + process._events.uncaughtException = undefined; + process.domain = undefined; + } + } +} + + +function runCallback(cb, id) { + var fail = true; + try { + cb(id); + fail = false; + } finally { + // Force the application to shutdown if one of the callbacks throws. This + // may change in the future depending on whether it can be determined if + // there's a slim chance of the application remaining stable after handling + // one of these exceptions. + if (fail) { + process._events.uncaughtException = undefined; + process.domain = undefined; + } + } +} + + +// Placing all exports down here because the exported classes won't export +// otherwise. +module.exports = { + // Public API + createHook, + currentId, + triggerId, + // Embedder API + AsyncEvent, + triggerIdScope, + // Sensitive Embedder API + newUid, + initTriggerId, + setInitTriggerId, + emitInit: emitInitS, + emitBefore: emitBeforeS, + emitAfter: emitAfterS, + emitDestroy: emitDestroyS, +}; diff --git a/lib/internal/module.js b/lib/internal/module.js index 2f38618daac5f7..f2c32b51e1106a 100644 --- a/lib/internal/module.js +++ b/lib/internal/module.js @@ -52,10 +52,10 @@ function stripBOM(content) { } exports.builtinLibs = [ - 'assert', 'buffer', 'child_process', 'cluster', 'crypto', 'dgram', 'dns', - 'domain', 'events', 'fs', 'http', 'https', 'net', 'os', 'path', 'punycode', - 'querystring', 'readline', 'repl', 'stream', 'string_decoder', 'tls', 'tty', - 'url', 'util', 'v8', 'vm', 'zlib' + 'assert', 'async_hooks', 'buffer', 'child_process', 'cluster', 'crypto', + 'dgram', 'dns', 'domain', 'events', 'fs', 'http', 'https', 'net', 'os', + 'path', 'punycode', 'querystring', 'readline', 'repl', 'stream', + 'string_decoder', 'tls', 'tty', 'url', 'util', 'v8', 'vm', 'zlib' ]; function addBuiltinLibsToObject(object) { diff --git a/node.gyp b/node.gyp index 7d03d1df08a7a2..17022b14103804 100644 --- a/node.gyp +++ b/node.gyp @@ -24,6 +24,7 @@ 'lib/internal/bootstrap_node.js', 'lib/_debug_agent.js', 'lib/_debugger.js', + 'lib/async_hooks.js', 'lib/assert.js', 'lib/buffer.js', 'lib/child_process.js', diff --git a/src/async-wrap-inl.h b/src/async-wrap-inl.h index a9bb90a8aa7fa2..f7da73edd22f62 100644 --- a/src/async-wrap-inl.h +++ b/src/async-wrap-inl.h @@ -15,13 +15,8 @@ namespace node { -inline bool AsyncWrap::ran_init_callback() const { - return static_cast(bits_ & 1); -} - - inline AsyncWrap::ProviderType AsyncWrap::provider_type() const { - return static_cast(bits_ >> 1); + return provider_type_; } @@ -30,6 +25,11 @@ inline double AsyncWrap::get_id() const { } +inline double AsyncWrap::get_trigger_id() const { + return trigger_id_; +} + + inline v8::Local AsyncWrap::MakeCallback( const v8::Local symbol, int argc, diff --git a/src/async-wrap.cc b/src/async-wrap.cc index 242c3f871da606..5e1d9a8a2889e2 100644 --- a/src/async-wrap.cc +++ b/src/async-wrap.cc @@ -9,23 +9,28 @@ #include "v8.h" #include "v8-profiler.h" -using v8::Boolean; +using v8::ArrayBuffer; using v8::Context; +using v8::Float64Array; using v8::Function; using v8::FunctionCallbackInfo; using v8::HandleScope; using v8::HeapProfiler; -using v8::Int32; using v8::Integer; using v8::Isolate; using v8::Local; using v8::MaybeLocal; +using v8::NewStringType; using v8::Number; using v8::Object; using v8::RetainedObjectInfo; +using v8::String; using v8::TryCatch; +using v8::Uint32Array; using v8::Value; +using AsyncHooks = node::Environment::AsyncHooks; + namespace node { static const char* const provider_names[] = { @@ -36,6 +41,8 @@ static const char* const provider_names[] = { }; +// Report correct information in a heapdump. + class RetainedAsyncInfo: public RetainedObjectInfo { public: explicit RetainedAsyncInfo(uint16_t class_id, AsyncWrap* wrap); @@ -107,35 +114,16 @@ RetainedObjectInfo* WrapperInfo(uint16_t class_id, Local wrapper) { // end RetainedAsyncInfo -static void EnableHooksJS(const FunctionCallbackInfo& args) { - Environment* env = Environment::GetCurrent(args); - Local init_fn = env->async_hooks_init_function(); - if (init_fn.IsEmpty() || !init_fn->IsFunction()) - return env->ThrowTypeError("init callback is not assigned to a function"); - env->async_hooks()->set_enable_callbacks(1); -} - - -static void DisableHooksJS(const FunctionCallbackInfo& args) { - Environment* env = Environment::GetCurrent(args); - env->async_hooks()->set_enable_callbacks(0); -} - - static void SetupHooks(const FunctionCallbackInfo& args) { Environment* env = Environment::GetCurrent(args); - if (env->async_hooks()->callbacks_enabled()) - return env->ThrowError("hooks should not be set while also enabled"); if (!args[0]->IsObject()) return env->ThrowTypeError("first argument must be an object"); // All of init, before, after, destroy are supplied by async_hooks // internally, so this should every only be called once. At which time all - // the functions should be set. Detect this by checking if init !IsEmpty() - // and returning early if that's the case. - if (!env->async_hooks_init_function().IsEmpty()) - return env->ThrowError("async_hook callbacks have already been setup"); + // the functions should be set. Detect this by checking if init !IsEmpty(). + CHECK(env->async_hooks_init_function().IsEmpty()); Local fn_obj = args[0].As(); @@ -152,10 +140,10 @@ static void SetupHooks(const FunctionCallbackInfo& args) { env->context(), FIXED_ONE_BYTE_STRING(env->isolate(), "destroy")).ToLocalChecked(); - if (!init_v->IsFunction() || !before_v->IsFunction() || - !after_v->IsFunction() || !destroy_v->IsFunction()) { - return env->ThrowTypeError("all callbacks must be functions"); - } + CHECK(init_v->IsFunction()); + CHECK(before_v->IsFunction()); + CHECK(after_v->IsFunction()); + CHECK(destroy_v->IsFunction()); env->set_async_hooks_init_function(init_v.As()); env->set_async_hooks_before_function(before_v.As()); @@ -172,8 +160,77 @@ void AsyncWrap::Initialize(Local target, HandleScope scope(isolate); env->SetMethod(target, "setupHooks", SetupHooks); - env->SetMethod(target, "disable", DisableHooksJS); - env->SetMethod(target, "enable", EnableHooksJS); + + // Attach the uint32_t[] where each slot contains the count of the number of + // callbacks waiting to be called on a particular event. It can then be + // incremented/decremented from JS quickly to communicate to C++ if there are + // any callbacks waiting to be called. + uint32_t* fields_ptr = env->async_hooks()->fields(); + int fields_count = env->async_hooks()->fields_count(); + Local fields_ab = ArrayBuffer::New( + isolate, + fields_ptr, + fields_count * sizeof(*fields_ptr)); + Local fields = + Uint32Array::New(fields_ab, 0, fields_count); + target->Set(context, + FIXED_ONE_BYTE_STRING(isolate, "async_hook_fields"), + fields).FromJust(); + + // The following v8::Float64Array has 5 fields. These fields are shared in + // this way to allow JS and C++ to read/write each value as quickly as + // possible. The fields are represented as follows: + // + // kAsyncUid: Maintains the state of the next unique id to be assigned. + // + // kCurrentId: Is the id of the resource responsible for the current + // execution context. A currentId == 0 means the "void", or that there is + // no JS stack above the init() call (happens when a new handle is created + // for an incoming TCP socket). A currentId == 1 means "root". Or the + // execution context of node::StartNodeInstance. + // + // kTriggerId: Is the id of the resource responsible for init() being called. + // For example, the trigger id of a new connection's TCP handle would be + // the server handle. Whereas the current id at that time would be 0. + // + // kInitTriggerId: Write the id of the resource resource responsible for a + // handle's creation just before calling the new handle's constructor. + // After the new handle is constructed kInitTriggerId is set back to 0. + // + // kScopedTriggerId: triggerId for all constructors created within the + // execution scope of the JS function triggerIdScope(). This value is + // superseded by kInitTriggerId, if set. + double* uid_fields_ptr = env->async_hooks()->uid_fields(); + int uid_fields_count = env->async_hooks()->uid_fields_count(); + Local uid_fields_ab = ArrayBuffer::New( + isolate, + uid_fields_ptr, + uid_fields_count * sizeof(*uid_fields_ptr)); + Local uid_fields = + Float64Array::New(uid_fields_ab, 0, uid_fields_count); + target->Set(context, + FIXED_ONE_BYTE_STRING(isolate, "async_uid_fields"), + uid_fields).FromJust(); + + Local constants = Object::New(isolate); +#define SET_HOOKS_CONSTANT(name) \ + constants->ForceSet(context, \ + FIXED_ONE_BYTE_STRING(isolate, #name), \ + Integer::New(isolate, AsyncHooks::name), \ + v8::ReadOnly).FromJust() + SET_HOOKS_CONSTANT(kInit); + SET_HOOKS_CONSTANT(kBefore); + SET_HOOKS_CONSTANT(kAfter); + SET_HOOKS_CONSTANT(kDestroy); + SET_HOOKS_CONSTANT(kActiveHooks); + SET_HOOKS_CONSTANT(kAsyncUidCntr); + SET_HOOKS_CONSTANT(kCurrentId); + SET_HOOKS_CONSTANT(kTriggerId); + SET_HOOKS_CONSTANT(kInitTriggerId); + SET_HOOKS_CONSTANT(kScopedTriggerId); +#undef SET_HOOKS_CONSTANT + target->Set(context, FIXED_ONE_BYTE_STRING(isolate, "constants"), constants) + .FromJust(); Local async_providers = Object::New(isolate); #define V(PROVIDER) \ @@ -190,20 +247,23 @@ void AsyncWrap::Initialize(Local target, } +void AsyncWrap::GetAsyncId(const FunctionCallbackInfo& args) { + AsyncWrap* wrap; + args.GetReturnValue().Set(-1); + ASSIGN_OR_RETURN_UNWRAP(&wrap, args.Holder()); + args.GetReturnValue().Set(wrap->get_id()); +} + + void AsyncWrap::DestroyIdsCb(uv_idle_t* handle) { uv_idle_stop(handle); Environment* env = Environment::from_destroy_ids_idle_handle(handle); - // None of the V8 calls done outside the HandleScope leak a handle. If this - // changes in the future then the SealHandleScope wrapping the uv_run() - // will catch this can cause the process to abort. + HandleScope handle_scope(env->isolate()); Context::Scope context_scope(env->context()); Local fn = env->async_hooks_destroy_function(); - if (fn.IsEmpty()) - return env->destroy_ids_list()->clear(); - TryCatch try_catch(env->isolate()); std::vector destroy_ids_list; @@ -236,74 +296,78 @@ void LoadAsyncWrapperInfo(Environment* env) { } +// TODO(trevnorris): Look into the overhead of using this. Can't use it anway +// if it switches to using persistent strings instead. +static const char* GetProviderName(AsyncWrap::ProviderType provider) { + CHECK_GT(provider, 0); + CHECK_LE(provider, AsyncWrap::PROVIDERS_LENGTH); + return provider_names[provider]; +} + + AsyncWrap::AsyncWrap(Environment* env, Local object, - ProviderType provider, - AsyncWrap* parent) - : BaseObject(env, object), bits_(static_cast(provider) << 1), - id_(env->get_async_wrap_uid()) { + ProviderType provider) + : BaseObject(env, object), + provider_type_(provider) { CHECK_NE(provider, PROVIDER_NONE); CHECK_GE(object->InternalFieldCount(), 1); // Shift provider value over to prevent id collision. persistent().SetWrapperClassId(NODE_ASYNC_ID_OFFSET + provider); - Local init_fn = env->async_hooks_init_function(); + // Use ther Reset() call to call the init() callbacks. + Reset(); +} - // No init callback exists, no reason to go on. - if (init_fn.IsEmpty()) - return; - // If async wrap callbacks are disabled and no parent was passed that has - // run the init callback then return. - if (!env->async_wrap_callbacks_enabled() && - (parent == nullptr || !parent->ran_init_callback())) +AsyncWrap::~AsyncWrap() { + if (env()->async_hooks()->fields()[AsyncHooks::kDestroy] == 0) { return; - - HandleScope scope(env->isolate()); - - Local argv[] = { - Number::New(env->isolate(), get_id()), - Int32::New(env->isolate(), provider), - Null(env->isolate()), - Null(env->isolate()) - }; - - if (parent != nullptr) { - argv[2] = Number::New(env->isolate(), parent->get_id()); - argv[3] = parent->object(); } - TryCatch try_catch(env->isolate()); - - MaybeLocal ret = - init_fn->Call(env->context(), object, arraysize(argv), argv); - - if (ret.IsEmpty()) { - ClearFatalExceptionHandlers(env); - FatalException(env->isolate(), try_catch); - } + if (env()->destroy_ids_list()->empty()) + uv_idle_start(env()->destroy_ids_idle_handle(), DestroyIdsCb); - bits_ |= 1; // ran_init_callback() is true now. + env()->destroy_ids_list()->push_back(get_id()); } +// Generalized call for both the constructor and for handles that are pooled +// and reused over their lifetime. This way a new uid can be assigned when +// the resource is pulled out of the pool and put back into use. +void AsyncWrap::Reset() { + AsyncHooks* async_hooks = env()->async_hooks(); + id_ = env()->new_async_uid(); + trigger_id_ = env()->exchange_init_trigger_id(0); -AsyncWrap::~AsyncWrap() { - if (!ran_init_callback()) + // Nothing to execute, so can continue normally. + if (async_hooks->fields()[AsyncHooks::kInit] == 0) { return; + } - if (env()->destroy_ids_list()->empty()) - uv_idle_start(env()->destroy_ids_idle_handle(), DestroyIdsCb); + HandleScope scope(env()->isolate()); + Local init_fn = env()->async_hooks_init_function(); - env()->destroy_ids_list()->push_back(get_id()); -} + Local argv[] = { + Number::New(env()->isolate(), get_id()), + // TODO(trevnorris): Very slow and bad. Use another way to more quickly get + // the correct provider string. Something like storing them on + // PER_ISOLATE_STRING_PROPERTIES in env.h + String::NewFromUtf8(env()->isolate(), + GetProviderName(provider_type()), + NewStringType::kNormal).ToLocalChecked(), + object(), + Number::New(env()->isolate(), get_trigger_id()), + }; + TryCatch try_catch(env()->isolate()); + MaybeLocal ret = init_fn->Call( + env()->context(), object(), arraysize(argv), argv); -void AsyncWrap::GetAsyncId(const FunctionCallbackInfo& args) { - AsyncWrap* wrap; - args.GetReturnValue().Set(-1); - ASSIGN_OR_RETURN_UNWRAP(&wrap, args.Holder()); - args.GetReturnValue().Set(wrap->get_id()); + if (ret.IsEmpty()) { + ClearFatalExceptionHandlers(env()); + FatalException(env()->isolate(), try_catch); + } } @@ -312,11 +376,10 @@ Local AsyncWrap::MakeCallback(const Local cb, Local* argv) { CHECK(env()->context() == env()->isolate()->GetCurrentContext()); - Local before_fn = env()->async_hooks_before_function(); - Local after_fn = env()->async_hooks_after_function(); - Local uid = Number::New(env()->isolate(), get_id()); + AsyncHooks* async_hooks = env()->async_hooks(); Local context = object(); Local domain; + Local uid; bool has_domain = false; Environment::AsyncCallbackScope callback_scope(env()); @@ -341,9 +404,15 @@ Local AsyncWrap::MakeCallback(const Local cb, } } - if (ran_init_callback() && !before_fn.IsEmpty()) { + // Want currentId() to return the correct value from the callbacks. + AsyncHooks::ExecScope exec_scope(env(), get_id(), get_trigger_id()); + + if (async_hooks->fields()[AsyncHooks::kBefore] > 0) { + uid = Number::New(env()->isolate(), get_id()); + Local fn = env()->async_hooks_before_function(); TryCatch try_catch(env()->isolate()); - MaybeLocal ar = before_fn->Call(env()->context(), context, 1, &uid); + MaybeLocal ar = fn->Call( + env()->context(), Undefined(env()->isolate()), 1, &uid); if (ar.IsEmpty()) { ClearFatalExceptionHandlers(env()); FatalException(env()->isolate(), try_catch); @@ -351,14 +420,24 @@ Local AsyncWrap::MakeCallback(const Local cb, } } - Local ret = cb->Call(context, argc, argv); + // Finally... Get to running the user's callback. + MaybeLocal ret = cb->Call(env()->context(), context, argc, argv); + + Local ret_v; + if (!ret.ToLocal(&ret_v)) { + return Local(); + } - if (ran_init_callback() && !after_fn.IsEmpty()) { - Local did_throw = Boolean::New(env()->isolate(), ret.IsEmpty()); - Local vals[] = { uid, did_throw }; + // TODO(trevnorris): It will be confusing for developers if there's a caught + // uncaught exception. Which leads to none of their after() callbacks being + // called. + if (async_hooks->fields()[AsyncHooks::kAfter] > 0) { + if (uid.IsEmpty()) + uid = Number::New(env()->isolate(), get_id()); + Local fn = env()->async_hooks_after_function(); TryCatch try_catch(env()->isolate()); - MaybeLocal ar = - after_fn->Call(env()->context(), context, arraysize(vals), vals); + MaybeLocal ar = fn->Call( + env()->context(), Undefined(env()->isolate()), 1, &uid); if (ar.IsEmpty()) { ClearFatalExceptionHandlers(env()); FatalException(env()->isolate(), try_catch); @@ -366,10 +445,6 @@ Local AsyncWrap::MakeCallback(const Local cb, } } - if (ret.IsEmpty()) { - return ret; - } - if (has_domain) { Local exit_v = domain->Get(env()->exit_string()); if (exit_v->IsFunction()) { @@ -380,8 +455,11 @@ Local AsyncWrap::MakeCallback(const Local cb, } } + // The execution scope of the id and trigger_id only go this far. + exec_scope.Dispose(); + if (callback_scope.in_makecallback()) { - return ret; + return ret_v; } Environment::TickInfo* tick_info = env()->tick_info(); @@ -394,14 +472,15 @@ Local AsyncWrap::MakeCallback(const Local cb, if (tick_info->length() == 0) { tick_info->set_index(0); - return ret; - } - - if (env()->tick_callback_function()->Call(process, 0, nullptr).IsEmpty()) { - return Local(); + return ret_v; } - return ret; + MaybeLocal rcheck = + env()->tick_callback_function()->Call(env()->context(), + process, + 0, + nullptr); + return rcheck.IsEmpty() ? Local() : ret_v; } } // namespace node diff --git a/src/async-wrap.h b/src/async-wrap.h index 70df8faaa74d9c..abb2fb5be36b67 100644 --- a/src/async-wrap.h +++ b/src/async-wrap.h @@ -55,8 +55,7 @@ class AsyncWrap : public BaseObject { AsyncWrap(Environment* env, v8::Local object, - ProviderType provider, - AsyncWrap* parent = nullptr); + ProviderType provider); virtual ~AsyncWrap(); @@ -72,28 +71,30 @@ class AsyncWrap : public BaseObject { inline double get_id() const; + inline double get_trigger_id() const; + + void Reset(); + // Only call these within a valid HandleScope. + // TODO(trevnorris): These should return a MaybeLocal. v8::Local MakeCallback(const v8::Local cb, - int argc, - v8::Local* argv); + int argc, + v8::Local* argv); inline v8::Local MakeCallback(const v8::Local symbol, - int argc, - v8::Local* argv); + int argc, + v8::Local* argv); inline v8::Local MakeCallback(uint32_t index, - int argc, - v8::Local* argv); + int argc, + v8::Local* argv); virtual size_t self_size() const = 0; private: inline AsyncWrap(); - inline bool ran_init_callback() const; - - // When the async hooks init JS function is called from the constructor it is - // expected the context object will receive a _asyncQueue object property - // that will be used to call pre/post in MakeCallback. - uint32_t bits_; - const double id_; + const ProviderType provider_type_; + // Because the values may be Reset(), cannot be made const. + double id_; + double trigger_id_; }; void LoadAsyncWrapperInfo(Environment* env); diff --git a/src/cares_wrap.cc b/src/cares_wrap.cc index c571cccea8f1ef..32c08b6bb708de 100644 --- a/src/cares_wrap.cc +++ b/src/cares_wrap.cc @@ -82,6 +82,7 @@ inline const char* ToErrorCodeString(int status) { class GetAddrInfoReqWrap : public ReqWrap { public: GetAddrInfoReqWrap(Environment* env, Local req_wrap_obj); + ~GetAddrInfoReqWrap(); size_t self_size() const override { return sizeof(*this); } }; @@ -93,6 +94,11 @@ GetAddrInfoReqWrap::GetAddrInfoReqWrap(Environment* env, } +GetAddrInfoReqWrap::~GetAddrInfoReqWrap() { + ClearWrap(object()); +} + + static void NewGetAddrInfoReqWrap(const FunctionCallbackInfo& args) { CHECK(args.IsConstructCall()); } @@ -101,6 +107,7 @@ static void NewGetAddrInfoReqWrap(const FunctionCallbackInfo& args) { class GetNameInfoReqWrap : public ReqWrap { public: GetNameInfoReqWrap(Environment* env, Local req_wrap_obj); + ~GetNameInfoReqWrap(); size_t self_size() const override { return sizeof(*this); } }; @@ -112,6 +119,11 @@ GetNameInfoReqWrap::GetNameInfoReqWrap(Environment* env, } +GetNameInfoReqWrap::~GetNameInfoReqWrap() { + ClearWrap(object()); +} + + static void NewGetNameInfoReqWrap(const FunctionCallbackInfo& args) { CHECK(args.IsConstructCall()); } @@ -286,6 +298,7 @@ class QueryWrap : public AsyncWrap { : AsyncWrap(env, req_wrap_obj, AsyncWrap::PROVIDER_QUERYWRAP) { if (env->in_domain()) req_wrap_obj->Set(env->domain_string(), env->domain_array()->Get(0)); + Wrap(req_wrap_obj, this); } ~QueryWrap() override { diff --git a/src/connect_wrap.cc b/src/connect_wrap.cc index df3f093e732972..e373b5a36e33e6 100644 --- a/src/connect_wrap.cc +++ b/src/connect_wrap.cc @@ -19,4 +19,9 @@ ConnectWrap::ConnectWrap(Environment* env, Wrap(req_wrap_obj, this); } + +ConnectWrap::~ConnectWrap() { + ClearWrap(object()); +} + } // namespace node diff --git a/src/connect_wrap.h b/src/connect_wrap.h index 28d4872d7ed416..7b16a5448745aa 100644 --- a/src/connect_wrap.h +++ b/src/connect_wrap.h @@ -15,6 +15,7 @@ class ConnectWrap : public ReqWrap { ConnectWrap(Environment* env, v8::Local req_wrap_obj, AsyncWrap::ProviderType provider); + ~ConnectWrap(); size_t self_size() const override { return sizeof(*this); } }; diff --git a/src/connection_wrap.cc b/src/connection_wrap.cc index 020fe8b4c9508c..30cf4bc11e6086 100644 --- a/src/connection_wrap.cc +++ b/src/connection_wrap.cc @@ -23,13 +23,11 @@ using v8::Value; template ConnectionWrap::ConnectionWrap(Environment* env, Local object, - ProviderType provider, - AsyncWrap* parent) + ProviderType provider) : StreamWrap(env, object, reinterpret_cast(&handle_), - provider, - parent) {} + provider) {} template @@ -115,14 +113,12 @@ void ConnectionWrap::AfterConnect(uv_connect_t* req, template ConnectionWrap::ConnectionWrap( Environment* env, Local object, - ProviderType provider, - AsyncWrap* parent); + ProviderType provider); template ConnectionWrap::ConnectionWrap( Environment* env, Local object, - ProviderType provider, - AsyncWrap* parent); + ProviderType provider); template void ConnectionWrap::OnConnection( uv_stream_t* handle, int status); diff --git a/src/connection_wrap.h b/src/connection_wrap.h index 7af97fd3f05e1b..99fe5697ed91fa 100644 --- a/src/connection_wrap.h +++ b/src/connection_wrap.h @@ -22,8 +22,7 @@ class ConnectionWrap : public StreamWrap { protected: ConnectionWrap(Environment* env, v8::Local object, - ProviderType provider, - AsyncWrap* parent); + ProviderType provider); ~ConnectionWrap() { } diff --git a/src/env-inl.h b/src/env-inl.h index ea67fa20221c51..41aef8b7009aea 100644 --- a/src/env-inl.h +++ b/src/env-inl.h @@ -59,8 +59,7 @@ inline uint32_t* IsolateData::zero_fill_field() const { return zero_fill_field_; } -inline Environment::AsyncHooks::AsyncHooks() { - for (int i = 0; i < kFieldsCount; i++) fields_[i] = 0; +inline Environment::AsyncHooks::AsyncHooks() : fields_(), uid_fields_() { } inline uint32_t* Environment::AsyncHooks::fields() { @@ -71,12 +70,12 @@ inline int Environment::AsyncHooks::fields_count() const { return kFieldsCount; } -inline bool Environment::AsyncHooks::callbacks_enabled() { - return fields_[kEnableCallbacks] != 0; +inline double* Environment::AsyncHooks::uid_fields() { + return uid_fields_; } -inline void Environment::AsyncHooks::set_enable_callbacks(uint32_t flag) { - fields_[kEnableCallbacks] = flag; +inline int Environment::AsyncHooks::uid_fields_count() const { + return kUidFieldsCount; } inline Environment::AsyncCallbackScope::AsyncCallbackScope(Environment* env) @@ -171,7 +170,6 @@ inline Environment::Environment(IsolateData* isolate_data, printed_error_(false), trace_sync_io_(false), makecallback_cntr_(0), - async_wrap_id_(0), debugger_agent_(this), #if HAVE_INSPECTOR inspector_agent_(this), @@ -237,11 +235,6 @@ inline v8::Isolate* Environment::isolate() const { return isolate_; } -inline bool Environment::async_wrap_callbacks_enabled() const { - // The const_cast is okay, it doesn't violate conceptual const-ness. - return const_cast(this)->async_hooks()->callbacks_enabled(); -} - inline bool Environment::in_domain() const { // The const_cast is okay, it doesn't violate conceptual const-ness. return using_domains() && @@ -320,14 +313,61 @@ inline void Environment::set_trace_sync_io(bool value) { trace_sync_io_ = value; } -inline double Environment::get_async_wrap_uid() { - return ++async_wrap_id_; -} - inline std::vector* Environment::destroy_ids_list() { return &destroy_ids_list_; } +inline double Environment::new_async_uid() { + return ++async_hooks()->uid_fields()[AsyncHooks::kAsyncUidCntr]; +} + +inline double Environment::current_async_id() { + return async_hooks()->uid_fields()[AsyncHooks::kCurrentId]; +} + +inline double Environment::exchange_current_async_id(const double id) { + const double oid = async_hooks()->uid_fields()[AsyncHooks::kCurrentId]; + async_hooks()->uid_fields()[AsyncHooks::kCurrentId] = id; + return oid; +} + +inline double Environment::trigger_id() { + return async_hooks()->uid_fields()[AsyncHooks::kTriggerId]; +} + +inline double Environment::exchange_trigger_id(const double id) { + const double oid = async_hooks()->uid_fields()[AsyncHooks::kTriggerId]; + async_hooks()->uid_fields()[AsyncHooks::kTriggerId] = id; + return oid; +} + +inline double Environment::exchange_init_trigger_id(const double id) { + auto uid_fields = async_hooks()->uid_fields(); + double tid = uid_fields[AsyncHooks::kInitTriggerId]; + uid_fields[AsyncHooks::kInitTriggerId] = id; + if (tid <= 0) tid = uid_fields[AsyncHooks::kScopedTriggerId]; + if (tid <= 0) tid = uid_fields[AsyncHooks::kCurrentId]; + return tid; +} + +inline void Environment::set_init_trigger_id(const double id) { + async_hooks()->uid_fields()[AsyncHooks::kInitTriggerId] = id; +} + +inline void Environment::erase_fd_async_id(int fd) { + fd_async_id_map_.erase(fd); +} + +inline node_fd_async_ids Environment::get_fd_async_id(int fd) { + return fd_async_id_map_[fd]; +} + +inline void Environment::insert_fd_async_ids(int fd, + double async_id, + double trigger_id) { + fd_async_id_map_[fd] = { async_id, trigger_id }; +} + inline double* Environment::heap_statistics_buffer() const { CHECK_NE(heap_statistics_buffer_, nullptr); return heap_statistics_buffer_; diff --git a/src/env.h b/src/env.h index 2ce8e8ea947900..72d59b78e29661 100644 --- a/src/env.h +++ b/src/env.h @@ -17,6 +17,7 @@ #include #include +#include // Caveat emptor: we're going slightly crazy with macros here but the end // hopefully justifies the means. We have a lot of per-context properties @@ -69,7 +70,6 @@ namespace node { V(address_string, "address") \ V(args_string, "args") \ V(async, "async") \ - V(async_queue_string, "_asyncQueue") \ V(buffer_string, "buffer") \ V(bytes_string, "bytes") \ V(bytes_parsed_string, "bytesParsed") \ @@ -268,6 +268,11 @@ struct node_ares_task { RB_ENTRY(node_ares_task) node; }; +struct node_fd_async_ids { + double async_id; + double trigger_id; +}; + RB_HEAD(node_ares_task_list, node_ares_task); class IsolateData { @@ -308,22 +313,90 @@ class Environment { public: class AsyncHooks { public: + // Reason for both UidFields and Fields are that one is stored as a double* + // and the other as a uint32_t*. + enum UidFields { + kAsyncUidCntr, + kCurrentId, + kTriggerId, + kInitTriggerId, + kScopedTriggerId, + kUidFieldsCount, + }; + + enum Fields { + kInit, + kBefore, + kAfter, + kDestroy, + kActiveHooks, + kFieldsCount, + }; + inline uint32_t* fields(); inline int fields_count() const; - inline bool callbacks_enabled(); - inline void set_enable_callbacks(uint32_t flag); + inline double* uid_fields(); + inline int uid_fields_count() const; + + class InitScope { + public: + explicit InitScope(Environment* env, double init_trigger_id) + : uid_fields_(env->async_hooks()->uid_fields()), + init_trigger_id_(uid_fields_[AsyncHooks::kScopedTriggerId]) { + uid_fields_[AsyncHooks::kScopedTriggerId] = init_trigger_id; + } + ~InitScope() { + uid_fields_[AsyncHooks::kScopedTriggerId] = init_trigger_id_; + } + private: + double* uid_fields_; + const double init_trigger_id_; + + DISALLOW_COPY_AND_ASSIGN(InitScope); + }; + + // ExecScope is meant for use in MakeCallback, to maintained stacked + // state. + // TODO(trevnorris): This conflicts with how emitBefore/emitAfter work + // (manually tracking the stacks in a JS array). Technically they should + // play nicely together, but write tests to prove this. + class ExecScope { + public: + explicit ExecScope(Environment* env, double id, double trigger_id) + : uid_fields_(env->async_hooks()->uid_fields()), + id_(uid_fields_[AsyncHooks::kCurrentId]), + trigger_id_(uid_fields_[AsyncHooks::kTriggerId]), + disposed_(false) { + uid_fields_[AsyncHooks::kCurrentId] = id; + uid_fields_[AsyncHooks::kTriggerId] = trigger_id; + } + ~ExecScope() { + if (disposed_) return; + Dispose(); + } + void Dispose() { + disposed_ = true; + uid_fields_[AsyncHooks::kCurrentId] = id_; + uid_fields_[AsyncHooks::kTriggerId] = trigger_id_; + } + + private: + double* uid_fields_; + const double id_; + const double trigger_id_; + bool disposed_; + + DISALLOW_COPY_AND_ASSIGN(ExecScope); + }; private: friend class Environment; // So we can call the constructor. inline AsyncHooks(); - enum Fields { - // Set this to not zero if the init hook should be called. - kEnableCallbacks, - kFieldsCount - }; - uint32_t fields_[kFieldsCount]; + // Gives us 2^53-1 unique ids. Good enough for now and makes the operation + // cheaper in JS. + double uid_fields_[kUidFieldsCount]; DISALLOW_COPY_AND_ASSIGN(AsyncHooks); }; @@ -428,7 +501,6 @@ class Environment { inline v8::Isolate* isolate() const; inline uv_loop_t* event_loop() const; - inline bool async_wrap_callbacks_enabled() const; inline bool in_domain() const; inline uint32_t watched_providers() const; @@ -465,7 +537,19 @@ class Environment { void PrintSyncTrace() const; inline void set_trace_sync_io(bool value); - inline double get_async_wrap_uid(); + // The necessary API for async_hooks. + inline double new_async_uid(); + inline double current_async_id(); + inline double exchange_current_async_id(const double id); + inline double trigger_id(); + inline double exchange_trigger_id(const double id); + inline double exchange_init_trigger_id(const double id); + inline void set_init_trigger_id(const double id); + + // For propagating hook id's with a file descriptor. + inline void erase_fd_async_id(int fd); + inline node_fd_async_ids get_fd_async_id(int fd); + inline void insert_fd_async_ids(int fd, double async_id, double trigger_id); // List of id's that have been destroyed and need the destroy() cb called. inline std::vector* destroy_ids_list(); @@ -569,6 +653,7 @@ class Environment { size_t makecallback_cntr_; double async_wrap_id_; std::vector destroy_ids_list_; + std::unordered_map fd_async_id_map_; debugger::Agent debugger_agent_; #if HAVE_INSPECTOR inspector::Agent inspector_agent_; diff --git a/src/handle_wrap.cc b/src/handle_wrap.cc index 317fb48b1d28e3..6c25d0eccbae24 100644 --- a/src/handle_wrap.cc +++ b/src/handle_wrap.cc @@ -69,9 +69,8 @@ void HandleWrap::Close(const FunctionCallbackInfo& args) { HandleWrap::HandleWrap(Environment* env, Local object, uv_handle_t* handle, - AsyncWrap::ProviderType provider, - AsyncWrap* parent) - : AsyncWrap(env, object, provider, parent), + AsyncWrap::ProviderType provider) + : AsyncWrap(env, object, provider), state_(kInitialized), handle_(handle) { handle_->data = this; diff --git a/src/handle_wrap.h b/src/handle_wrap.h index 2a128dd8b1679d..da69a44bff7bdb 100644 --- a/src/handle_wrap.h +++ b/src/handle_wrap.h @@ -53,8 +53,7 @@ class HandleWrap : public AsyncWrap { HandleWrap(Environment* env, v8::Local object, uv_handle_t* handle, - AsyncWrap::ProviderType provider, - AsyncWrap* parent = nullptr); + AsyncWrap::ProviderType provider); ~HandleWrap() override; private: diff --git a/src/js_stream.cc b/src/js_stream.cc index 1d20e1c6d77dfb..2644a6a451a00f 100644 --- a/src/js_stream.cc +++ b/src/js_stream.cc @@ -12,7 +12,6 @@ namespace node { using v8::Array; using v8::Context; -using v8::External; using v8::FunctionCallbackInfo; using v8::FunctionTemplate; using v8::HandleScope; @@ -21,8 +20,8 @@ using v8::Object; using v8::Value; -JSStream::JSStream(Environment* env, Local obj, AsyncWrap* parent) - : AsyncWrap(env, obj, AsyncWrap::PROVIDER_JSSTREAM, parent), +JSStream::JSStream(Environment* env, Local obj) + : AsyncWrap(env, obj, AsyncWrap::PROVIDER_JSSTREAM), StreamBase(env) { node::Wrap(obj, this); MakeWeak(this); @@ -115,17 +114,7 @@ void JSStream::New(const FunctionCallbackInfo& args) { // normal function. CHECK(args.IsConstructCall()); Environment* env = Environment::GetCurrent(args); - JSStream* wrap; - - if (args.Length() == 0) { - wrap = new JSStream(env, args.This(), nullptr); - } else if (args[0]->IsExternal()) { - void* ptr = args[0].As()->Value(); - wrap = new JSStream(env, args.This(), static_cast(ptr)); - } else { - UNREACHABLE(); - } - CHECK(wrap); + new JSStream(env, args.This()); } diff --git a/src/js_stream.h b/src/js_stream.h index 5a1244bc463e36..fc0b7abe15a633 100644 --- a/src/js_stream.h +++ b/src/js_stream.h @@ -33,7 +33,7 @@ class JSStream : public AsyncWrap, public StreamBase { size_t self_size() const override { return sizeof(*this); } protected: - JSStream(Environment* env, v8::Local obj, AsyncWrap* parent); + JSStream(Environment* env, v8::Local obj); AsyncWrap* GetAsyncWrap() override; diff --git a/src/node.cc b/src/node.cc index f3e8a6494e75df..b247db816ef9a4 100644 --- a/src/node.cc +++ b/src/node.cc @@ -1179,21 +1179,13 @@ Local MakeCallback(Environment* env, // If you hit this assertion, you forgot to enter the v8::Context first. CHECK_EQ(env->context(), env->isolate()->GetCurrentContext()); - Local before_fn = env->async_hooks_before_function(); - Local after_fn = env->async_hooks_after_function(); Local object, domain; - bool ran_init_callback = false; bool has_domain = false; Environment::AsyncCallbackScope callback_scope(env); - // TODO(trevnorris): Adding "_asyncQueue" to the "this" in the init callback - // is a horrible way to detect usage. Rethink how detection should happen. if (recv->IsObject()) { object = recv.As(); - Local async_queue_v = object->Get(env->async_queue_string()); - if (async_queue_v->IsObject()) - ran_init_callback = true; } if (env->using_domains()) { @@ -1217,34 +1209,8 @@ Local MakeCallback(Environment* env, } } - if (ran_init_callback && !before_fn.IsEmpty()) { - TryCatch try_catch(env->isolate()); - MaybeLocal ar = before_fn->Call(env->context(), object, 0, nullptr); - if (ar.IsEmpty()) { - ClearFatalExceptionHandlers(env); - FatalException(env->isolate(), try_catch); - return Local(); - } - } - Local ret = callback->Call(recv, argc, argv); - if (ran_init_callback && !after_fn.IsEmpty()) { - Local did_throw = Boolean::New(env->isolate(), ret.IsEmpty()); - // Currently there's no way to retrieve an uid from node::MakeCallback(). - // This needs to be fixed. - Local vals[] = - { Undefined(env->isolate()).As(), did_throw }; - TryCatch try_catch(env->isolate()); - MaybeLocal ar = - after_fn->Call(env->context(), object, arraysize(vals), vals); - if (ar.IsEmpty()) { - ClearFatalExceptionHandlers(env); - FatalException(env->isolate(), try_catch); - return Local(); - } - } - if (ret.IsEmpty()) { // NOTE: For backwards compatibility with public API we return Undefined() // if the top level call threw. @@ -1288,10 +1254,10 @@ Local MakeCallback(Environment* env, Local MakeCallback(Environment* env, - Local recv, - Local symbol, - int argc, - Local argv[]) { + Local recv, + Local symbol, + int argc, + Local argv[]) { Local cb_v = recv->Get(symbol); CHECK(cb_v->IsFunction()); return MakeCallback(env, recv.As(), cb_v.As(), argc, argv); @@ -1299,10 +1265,10 @@ Local MakeCallback(Environment* env, Local MakeCallback(Environment* env, - Local recv, - const char* method, - int argc, - Local argv[]) { + Local recv, + const char* method, + int argc, + Local argv[]) { Local method_string = OneByteString(env->isolate(), method); return MakeCallback(env, recv, method_string, argc, argv); } @@ -4398,6 +4364,10 @@ inline int Start(Isolate* isolate, IsolateData* isolate_data, } env.set_trace_sync_io(trace_sync_io); + // A current_async_id() of 1 means bootstrap phase. Now that the bootstrap + // phase is complete set the global id to 0 to let the internals know that + // everything points to the void. + env.exchange_current_async_id(0); // Enable debugger if (debug_enabled) diff --git a/src/node_file.cc b/src/node_file.cc index 544512dc7a7715..6646da09c86fb6 100644 --- a/src/node_file.cc +++ b/src/node_file.cc @@ -29,6 +29,7 @@ namespace node { using v8::Array; using v8::Context; using v8::EscapableHandleScope; +using v8::Float64Array; using v8::Function; using v8::FunctionCallbackInfo; using v8::FunctionTemplate; @@ -87,7 +88,10 @@ class FSReqWrap: public ReqWrap { Wrap(object(), this); } - ~FSReqWrap() { ReleaseEarly(); } + ~FSReqWrap() { + ReleaseEarly(); + ClearWrap(object()); + } void* operator new(size_t size) = delete; void* operator new(size_t size, char* storage) { return storage; } @@ -194,6 +198,9 @@ static void After(uv_fs_t *req) { break; case UV_FS_OPEN: + env->insert_fd_async_ids(req->result, + req_wrap->get_id(), + env->exchange_init_trigger_id(0)); argv[1] = Integer::New(env->isolate(), req->result); break; @@ -409,6 +416,9 @@ static void Close(const FunctionCallbackInfo& args) { return TYPE_ERROR("fd must be a file descriptor"); int fd = args[0]->Int32Value(); + // TODO(trevnorris): Won't these values be needed for the destroy callbacks? + // XXX Wouldn't fd_async_id_map_[n][1] be the triggerId of the close() call? + env->erase_fd_async_id(fd); if (args[1]->IsObject()) { ASYNC_CALL(close, args[1], UTF8, fd) @@ -1010,6 +1020,10 @@ static void Open(const FunctionCallbackInfo& args) { ASYNC_CALL(open, args[3], UTF8, *path, flags, mode) } else { SYNC_CALL(open, *path, *path, flags, mode) + // TODO(trevnorris): Is this really necessary for sync calls? + env->insert_fd_async_ids(SYNC_RESULT, + env->current_async_id(), + env->trigger_id()); args.GetReturnValue().Set(SYNC_RESULT); } } @@ -1433,6 +1447,29 @@ static void Mkdtemp(const FunctionCallbackInfo& args) { } } + +// To use this pass in a Float64Array(2) as the second argument. The asyncId +// and triggerId will be written to both indexes. This is much faster than +// creating and returning an Array. +static void GetIdsFromFd(const FunctionCallbackInfo& args) { + Environment* env = Environment::GetCurrent(args); + if (!args[0]->IsInt32()) + return TYPE_ERROR("fd must be an int"); + if (!args[1]->IsFloat64Array()) + return TYPE_ERROR("arr must be a Float64Array"); + + Local arr = args[1].As(); + if (arr->Length() < 2) + return env->ThrowRangeError("typed array is not large enough"); + + double* ptr = reinterpret_cast(arr->Buffer()->GetContents().Data()); + node_fd_async_ids slot = env->get_fd_async_id(args[0]->Int32Value()); + ptr[0] = slot.async_id; + ptr[1] = slot.trigger_id; + args.GetReturnValue().Set(arr); +} + + void FSInitialize(const FunctionCallbackInfo& args) { Local stats_constructor = args[0].As(); CHECK(stats_constructor->IsFunction()); @@ -1489,6 +1526,8 @@ void InitFs(Local target, env->SetMethod(target, "mkdtemp", Mkdtemp); + env->SetMethod(target, "getIdsFromFd", GetIdsFromFd); + StatWatcher::Initialize(env, target); // Create FunctionTemplate for FSReqWrap diff --git a/src/node_http_parser.cc b/src/node_http_parser.cc index 6d759639275ce4..3bcc5139f73cd8 100644 --- a/src/node_http_parser.cc +++ b/src/node_http_parser.cc @@ -454,6 +454,8 @@ class Parser : public AsyncWrap { ASSIGN_OR_RETURN_UNWRAP(&parser, args.Holder()); // Should always be called from the same context. CHECK_EQ(env, parser->env()); + // The parser is being reused. Reset the uid and call init() callbacks. + parser->Reset(); parser->Init(type); } diff --git a/src/pipe_wrap.cc b/src/pipe_wrap.cc index f41fc8dc80e505..9eb1aa03b2d888 100644 --- a/src/pipe_wrap.cc +++ b/src/pipe_wrap.cc @@ -17,7 +17,6 @@ namespace node { using v8::Context; using v8::EscapableHandleScope; -using v8::External; using v8::Function; using v8::FunctionCallbackInfo; using v8::FunctionTemplate; @@ -26,15 +25,16 @@ using v8::Local; using v8::Object; using v8::Value; +using AsyncHooks = Environment::AsyncHooks; Local PipeWrap::Instantiate(Environment* env, AsyncWrap* parent) { EscapableHandleScope handle_scope(env->isolate()); + AsyncHooks::InitScope init_scope(env, parent->get_id()); CHECK_EQ(false, env->pipe_constructor_template().IsEmpty()); Local constructor = env->pipe_constructor_template()->GetFunction(); CHECK_EQ(false, constructor.IsEmpty()); - Local ptr = External::New(env->isolate(), parent); Local instance = - constructor->NewInstance(env->context(), 1, &ptr).ToLocalChecked(); + constructor->NewInstance(env->context()).ToLocalChecked(); return handle_scope.Escape(instance); } @@ -92,23 +92,16 @@ void PipeWrap::New(const FunctionCallbackInfo& args) { // normal function. CHECK(args.IsConstructCall()); Environment* env = Environment::GetCurrent(args); - if (args[0]->IsExternal()) { - void* ptr = args[0].As()->Value(); - new PipeWrap(env, args.This(), false, static_cast(ptr)); - } else { - new PipeWrap(env, args.This(), args[0]->IsTrue(), nullptr); - } + new PipeWrap(env, args.This(), args[0]->IsTrue()); } PipeWrap::PipeWrap(Environment* env, Local object, - bool ipc, - AsyncWrap* parent) + bool ipc) : ConnectionWrap(env, object, - AsyncWrap::PROVIDER_PIPEWRAP, - parent) { + AsyncWrap::PROVIDER_PIPEWRAP) { int r = uv_pipe_init(env->event_loop(), &handle_, ipc); CHECK_EQ(r, 0); // How do we proxy this error up to javascript? // Suggestion: uv_pipe_init() returns void. diff --git a/src/pipe_wrap.h b/src/pipe_wrap.h index 9dcfa91bac8579..17a9dc5403c7ad 100644 --- a/src/pipe_wrap.h +++ b/src/pipe_wrap.h @@ -21,8 +21,7 @@ class PipeWrap : public ConnectionWrap { private: PipeWrap(Environment* env, v8::Local object, - bool ipc, - AsyncWrap* parent); + bool ipc); static void New(const v8::FunctionCallbackInfo& args); static void Bind(const v8::FunctionCallbackInfo& args); diff --git a/src/req-wrap-inl.h b/src/req-wrap-inl.h index 84af22023dc3b9..e21fb1bdad9363 100644 --- a/src/req-wrap-inl.h +++ b/src/req-wrap-inl.h @@ -30,7 +30,6 @@ template ReqWrap::~ReqWrap() { CHECK_EQ(req_.data, this); // Assert that someone has called Dispatched(). CHECK_EQ(false, persistent().IsEmpty()); - ClearWrap(object()); persistent().Reset(); } diff --git a/src/stream_base-inl.h b/src/stream_base-inl.h index da636909b695f3..8b5b15420703ef 100644 --- a/src/stream_base-inl.h +++ b/src/stream_base-inl.h @@ -23,6 +23,8 @@ using v8::PropertyCallbackInfo; using v8::String; using v8::Value; +using AsyncHooks = Environment::AsyncHooks; + template void StreamBase::AddMethods(Environment* env, Local t, @@ -134,6 +136,7 @@ void StreamBase::JSMethod(const FunctionCallbackInfo& args) { if (!wrap->IsAlive()) return args.GetReturnValue().Set(UV_EINVAL); + AsyncHooks::InitScope init_scope(handle->env(), handle->get_id()); args.GetReturnValue().Set((wrap->*Method)(args)); } diff --git a/src/stream_base.cc b/src/stream_base.cc index 3ed622d7ef35a2..76d2be1b9a21c8 100644 --- a/src/stream_base.cc +++ b/src/stream_base.cc @@ -53,6 +53,9 @@ int StreamBase::Shutdown(const FunctionCallbackInfo& args) { CHECK(args[0]->IsObject()); Local req_wrap_obj = args[0].As(); + AsyncWrap* wrap = GetAsyncWrap(); + if (wrap != nullptr) + env->set_init_trigger_id(wrap->get_id()); ShutdownWrap* req_wrap = new ShutdownWrap(env, req_wrap_obj, this, @@ -129,6 +132,10 @@ int StreamBase::Writev(const FunctionCallbackInfo& args) { if (storage_size > INT_MAX) return UV_ENOBUFS; + AsyncWrap* wrap = GetAsyncWrap(); + // TODO(trevnorris): Would like to catalog every case when wrap == nullptr. + if (wrap != nullptr) + env->set_init_trigger_id(wrap->get_id()); WriteWrap* req_wrap = WriteWrap::New(env, req_wrap_obj, this, @@ -192,6 +199,7 @@ int StreamBase::WriteBuffer(const FunctionCallbackInfo& args) { CHECK(Buffer::HasInstance(args[1])); Environment* env = Environment::GetCurrent(args); + AsyncWrap* wrap = GetAsyncWrap(); Local req_wrap_obj = args[0].As(); const char* data = Buffer::Data(args[1]); size_t length = Buffer::Length(args[1]); @@ -211,6 +219,8 @@ int StreamBase::WriteBuffer(const FunctionCallbackInfo& args) { goto done; CHECK_EQ(count, 1); + if (wrap != nullptr) + env->set_init_trigger_id(wrap->get_id()); // Allocate, or write rest req_wrap = WriteWrap::New(env, req_wrap_obj, this, AfterWrite); @@ -239,6 +249,7 @@ int StreamBase::WriteString(const FunctionCallbackInfo& args) { CHECK(args[0]->IsObject()); CHECK(args[1]->IsString()); + AsyncWrap* wrap = GetAsyncWrap(); Local req_wrap_obj = args[0].As(); Local string = args[1].As(); Local send_handle_obj; @@ -292,6 +303,8 @@ int StreamBase::WriteString(const FunctionCallbackInfo& args) { CHECK_EQ(count, 1); } + if (wrap != nullptr) + env->set_init_trigger_id(wrap->get_id()); req_wrap = WriteWrap::New(env, req_wrap_obj, this, AfterWrite, storage_size); data = req_wrap->Extra(); diff --git a/src/stream_base.h b/src/stream_base.h index 35929750bfbc54..e38e6c34078dd8 100644 --- a/src/stream_base.h +++ b/src/stream_base.h @@ -53,6 +53,10 @@ class ShutdownWrap : public ReqWrap, Wrap(req_wrap_obj, this); } + ~ShutdownWrap() { + ClearWrap(object()); + } + static void NewShutdownWrap(const v8::FunctionCallbackInfo& args) { CHECK(args.IsConstructCall()); } @@ -106,6 +110,10 @@ class WriteWrap: public ReqWrap, Wrap(obj, this); } + ~WriteWrap() { + ClearWrap(object()); + } + void* operator new(size_t size) = delete; void* operator new(size_t size, char* storage) { return storage; } diff --git a/src/stream_wrap.cc b/src/stream_wrap.cc index 563d2e4b28f943..73ff6b0e5d7a70 100644 --- a/src/stream_wrap.cc +++ b/src/stream_wrap.cc @@ -60,13 +60,11 @@ void StreamWrap::Initialize(Local target, StreamWrap::StreamWrap(Environment* env, Local object, uv_stream_t* stream, - AsyncWrap::ProviderType provider, - AsyncWrap* parent) + AsyncWrap::ProviderType provider) : HandleWrap(env, object, reinterpret_cast(stream), - provider, - parent), + provider), StreamBase(env), stream_(stream) { set_after_write_cb({ OnAfterWriteImpl, this }); diff --git a/src/stream_wrap.h b/src/stream_wrap.h index e930670202d2d8..543a269add93a3 100644 --- a/src/stream_wrap.h +++ b/src/stream_wrap.h @@ -60,8 +60,7 @@ class StreamWrap : public HandleWrap, public StreamBase { StreamWrap(Environment* env, v8::Local object, uv_stream_t* stream, - AsyncWrap::ProviderType provider, - AsyncWrap* parent = nullptr); + AsyncWrap::ProviderType provider); ~StreamWrap() { } diff --git a/src/tcp_wrap.cc b/src/tcp_wrap.cc index 8cb7e5aa1af4bd..4c6508624e2d5e 100644 --- a/src/tcp_wrap.cc +++ b/src/tcp_wrap.cc @@ -19,7 +19,6 @@ namespace node { using v8::Boolean; using v8::Context; using v8::EscapableHandleScope; -using v8::External; using v8::Function; using v8::FunctionCallbackInfo; using v8::FunctionTemplate; @@ -30,15 +29,17 @@ using v8::Object; using v8::String; using v8::Value; +using AsyncHooks = Environment::AsyncHooks; + Local TCPWrap::Instantiate(Environment* env, AsyncWrap* parent) { EscapableHandleScope handle_scope(env->isolate()); + AsyncHooks::InitScope init_scope(env, parent->get_id()); CHECK_EQ(env->tcp_constructor_template().IsEmpty(), false); Local constructor = env->tcp_constructor_template()->GetFunction(); CHECK_EQ(constructor.IsEmpty(), false); - Local ptr = External::New(env->isolate(), parent); Local instance = - constructor->NewInstance(env->context(), 1, &ptr).ToLocalChecked(); + constructor->NewInstance(env->context()).ToLocalChecked(); return handle_scope.Escape(instance); } @@ -112,24 +113,14 @@ void TCPWrap::New(const FunctionCallbackInfo& args) { // normal function. CHECK(args.IsConstructCall()); Environment* env = Environment::GetCurrent(args); - TCPWrap* wrap; - if (args.Length() == 0) { - wrap = new TCPWrap(env, args.This(), nullptr); - } else if (args[0]->IsExternal()) { - void* ptr = args[0].As()->Value(); - wrap = new TCPWrap(env, args.This(), static_cast(ptr)); - } else { - UNREACHABLE(); - } - CHECK(wrap); + new TCPWrap(env, args.This()); } -TCPWrap::TCPWrap(Environment* env, Local object, AsyncWrap* parent) +TCPWrap::TCPWrap(Environment* env, Local object) : ConnectionWrap(env, object, - AsyncWrap::PROVIDER_TCPWRAP, - parent) { + AsyncWrap::PROVIDER_TCPWRAP) { int r = uv_tcp_init(env->event_loop(), &handle_); CHECK_EQ(r, 0); // How do we proxy this error up to javascript? // Suggestion: uv_tcp_init() returns void. @@ -257,6 +248,7 @@ void TCPWrap::Connect(const FunctionCallbackInfo& args) { int err = uv_ip4_addr(*ip_address, port, &addr); if (err == 0) { + env->set_init_trigger_id(wrap->get_id()); ConnectWrap* req_wrap = new ConnectWrap(env, req_wrap_obj, AsyncWrap::PROVIDER_TCPCONNECTWRAP); err = uv_tcp_connect(req_wrap->req(), @@ -292,6 +284,7 @@ void TCPWrap::Connect6(const FunctionCallbackInfo& args) { int err = uv_ip6_addr(*ip_address, port, &addr); if (err == 0) { + env->set_init_trigger_id(wrap->get_id()); ConnectWrap* req_wrap = new ConnectWrap(env, req_wrap_obj, AsyncWrap::PROVIDER_TCPCONNECTWRAP); err = uv_tcp_connect(req_wrap->req(), diff --git a/src/tcp_wrap.h b/src/tcp_wrap.h index 2b9e288dced6aa..750e92d5f438d3 100644 --- a/src/tcp_wrap.h +++ b/src/tcp_wrap.h @@ -25,7 +25,7 @@ class TCPWrap : public ConnectionWrap { int (*F)(const typename T::HandleType*, sockaddr*, int*)> friend void GetSockOrPeerName(const v8::FunctionCallbackInfo&); - TCPWrap(Environment* env, v8::Local object, AsyncWrap* parent); + TCPWrap(Environment* env, v8::Local object); ~TCPWrap(); static void New(const v8::FunctionCallbackInfo& args); diff --git a/src/udp_wrap.cc b/src/udp_wrap.cc index 50f28881394caa..ee3b75ab63bbf9 100644 --- a/src/udp_wrap.cc +++ b/src/udp_wrap.cc @@ -16,7 +16,6 @@ namespace node { using v8::Array; using v8::Context; using v8::EscapableHandleScope; -using v8::External; using v8::FunctionCallbackInfo; using v8::FunctionTemplate; using v8::HandleScope; @@ -29,10 +28,13 @@ using v8::String; using v8::Undefined; using v8::Value; +using AsyncHooks = Environment::AsyncHooks; + class SendWrap : public ReqWrap { public: SendWrap(Environment* env, Local req_wrap_obj, bool have_callback); + ~SendWrap(); inline bool have_callback() const; size_t msg_size; size_t self_size() const override { return sizeof(*this); } @@ -41,6 +43,11 @@ class SendWrap : public ReqWrap { }; +SendWrap::~SendWrap() { + ClearWrap(object()); +} + + SendWrap::SendWrap(Environment* env, Local req_wrap_obj, bool have_callback) @@ -60,7 +67,7 @@ static void NewSendWrap(const FunctionCallbackInfo& args) { } -UDPWrap::UDPWrap(Environment* env, Local object, AsyncWrap* parent) +UDPWrap::UDPWrap(Environment* env, Local object) : HandleWrap(env, object, reinterpret_cast(&handle_), @@ -127,15 +134,7 @@ void UDPWrap::Initialize(Local target, void UDPWrap::New(const FunctionCallbackInfo& args) { CHECK(args.IsConstructCall()); Environment* env = Environment::GetCurrent(args); - if (args.Length() == 0) { - new UDPWrap(env, args.This(), nullptr); - } else if (args[0]->IsExternal()) { - new UDPWrap(env, - args.This(), - static_cast(args[0].As()->Value())); - } else { - UNREACHABLE(); - } + new UDPWrap(env, args.This()); } @@ -275,6 +274,7 @@ void UDPWrap::DoSend(const FunctionCallbackInfo& args, int family) { node::Utf8Value address(env->isolate(), args[4]); const bool have_callback = args[5]->IsTrue(); + env->set_init_trigger_id(wrap->get_id()); SendWrap* req_wrap = new SendWrap(env, req_wrap_obj, have_callback); size_t msg_size = 0; @@ -422,11 +422,12 @@ void UDPWrap::OnRecv(uv_udp_t* handle, Local UDPWrap::Instantiate(Environment* env, AsyncWrap* parent) { EscapableHandleScope scope(env->isolate()); + AsyncHooks::InitScope init_scope(env, parent->get_id()); // If this assert fires then Initialize hasn't been called yet. CHECK_EQ(env->udp_constructor_function().IsEmpty(), false); - Local ptr = External::New(env->isolate(), parent); - return scope.Escape(env->udp_constructor_function() - ->NewInstance(env->context(), 1, &ptr).ToLocalChecked()); + Local instance = env->udp_constructor_function() + ->NewInstance(env->context()).ToLocalChecked(); + return scope.Escape(instance); } diff --git a/src/udp_wrap.h b/src/udp_wrap.h index ad5f92c0f52bcb..4d5f08a2a1c2f1 100644 --- a/src/udp_wrap.h +++ b/src/udp_wrap.h @@ -47,7 +47,7 @@ class UDPWrap: public HandleWrap { int (*F)(const typename T::HandleType*, sockaddr*, int*)> friend void GetSockOrPeerName(const v8::FunctionCallbackInfo&); - UDPWrap(Environment* env, v8::Local object, AsyncWrap* parent); + UDPWrap(Environment* env, v8::Local object); static void DoBind(const v8::FunctionCallbackInfo& args, int family); From 3611f6aa058dada56beab5f76c781859285f0faf Mon Sep 17 00:00:00 2001 From: Trevor Norris Date: Thu, 17 Nov 2016 15:55:51 -0700 Subject: [PATCH 10/40] async_hooks: check correct array Check the correct array to see if hooks can be executed. --- lib/async_hooks.js | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/lib/async_hooks.js b/lib/async_hooks.js index ebea6c85fba9e7..8200b050efaee8 100644 --- a/lib/async_hooks.js +++ b/lib/async_hooks.js @@ -269,7 +269,7 @@ function emitBeforeS(id, triggerId) { async_uid_fields[kCurrentId] = id; async_uid_fields[kTriggerId] = triggerId === undefined ? id : triggerId; - if (active_hooks_array[kBefore] === 0) { + if (async_hook_fields[kBefore] === 0) { return; } for (var i = 0; i < active_hooks_array.length; i++) { @@ -309,7 +309,7 @@ function emitAfterS(id) { // of id's that may need to run. So instead of forcing native to call into JS // for every handle, instead have this call back into C++ for the next id. function emitDestroyS(id) { - if (active_hooks_array[kDestroy] === 0) { + if (async_hook_fields[kDestroy] === 0) { return; } for (var i = 0; i < active_hooks_array.length; i++) { From d244d55d53e75e3a83db354ce96e0f893c06155a Mon Sep 17 00:00:00 2001 From: Trevor Norris Date: Mon, 14 Nov 2016 16:49:52 -0700 Subject: [PATCH 11/40] next_tick: add async_hooks support Also did: * Turn TickObject() into a function * Split the function in half so it can be inlined * Fix how uid flags are set * Export kScopedTriggerId for use in setupInit() --- lib/async_hooks.js | 2 +- lib/internal/process/next_tick.js | 94 ++++++++++++++++++++++++++++--- 2 files changed, 86 insertions(+), 10 deletions(-) diff --git a/lib/async_hooks.js b/lib/async_hooks.js index 8200b050efaee8..b305e00d3cce34 100644 --- a/lib/async_hooks.js +++ b/lib/async_hooks.js @@ -37,7 +37,7 @@ const destroy_symbol = Symbol('destroy'); // The exception is kActiveHooks. Which tracks the total number of AsyncEvents // that exist on "active_hooks_array". const { kInit, kBefore, kAfter, kDestroy, kActiveHooks, kAsyncUidCntr, - kCurrentId, kTriggerId, kScopedTriggerId, kInitTriggerId } = + kCurrentId, kTriggerId, kInitTriggerId, kScopedTriggerId } = async_wrap.constants; // Setup the callbacks that node::AsyncWrap will call when there are hooks to diff --git a/lib/internal/process/next_tick.js b/lib/internal/process/next_tick.js index f27ef622a96e6a..2aaa03443ba2a3 100644 --- a/lib/internal/process/next_tick.js +++ b/lib/internal/process/next_tick.js @@ -3,8 +3,16 @@ exports.setup = setupNextTick; function setupNextTick() { + const async_wrap = process.binding('async_wrap'); + const async_hooks = require('async_hooks'); const promises = require('internal/process/promises'); const emitPendingUnhandledRejections = promises.setup(scheduleMicrotasks); + const initTriggerId = async_hooks.initTriggerId; + // Two arrays that share state between C++ and JS. + const { async_uid_fields, async_hook_fields } = async_wrap; + // Grab the constants necessary for working with internal arrays. + const { kInit, kBefore, kAfter, kDestroy, kActiveHooks, kAsyncUidCntr, + kCurrentId, kTriggerId } = async_wrap.constants; var nextTickQueue = []; var microtasksScheduled = false; @@ -44,10 +52,8 @@ function setupNextTick() { if (microtasksScheduled) return; - nextTickQueue.push({ - callback: runMicrotasksCallback, - domain: null - }); + nextTickQueue.push( + new TickObject(runMicrotasksCallback, undefined, null, true)); tickInfo[kLength]++; microtasksScheduled = true; @@ -89,13 +95,40 @@ function setupNextTick() { do { while (tickInfo[kIndex] < tickInfo[kLength]) { + const has_hooks = async_hook_fields[kActiveHooks] > 0; + // TODO(trevnorris): These should always be zero if at the bottom of + // the stack. Add check somewhere cheaper than here to make sure that's + // the case. + const prev_id = async_uid_fields[kCurrentId]; + const prev_trigger_id = async_uid_fields[kTriggerId]; + tock = nextTickQueue[tickInfo[kIndex]++]; callback = tock.callback; args = tock.args; + + // Setting these fields manually with the expectation that has_hooks + // will almost always be false. Overhead for doing it twice if it is + // true is negligible anyway. + async_uid_fields[kCurrentId] = tock._asyncId; + async_uid_fields[kTriggerId] = tock._triggerId; + if (has_hooks && async_hook_fields[kBefore]) + async_hooks.emitBefore(tock._asyncId, tock._triggerId); + // Using separate callback execution functions allows direct // callback invocation with small numbers of arguments to avoid the // performance hit associated with using `fn.apply()` _combinedTickCallback(args, callback); + + if (has_hooks) { + if (async_hook_fields[kAfter]) + async_hooks.emitAfter(tock._asyncId); + if (async_hook_fields[kDestroy]) + async_hooks.emitDestroy(tock._asyncId); + } + // TODO(trevnorris): A caught uncaught exception will mess this up. + async_uid_fields[kCurrentId] = prev_id; + async_uid_fields[kTriggerId] = prev_trigger_id; + if (1e4 < tickInfo[kIndex]) tickDone(); } @@ -110,16 +143,42 @@ function setupNextTick() { do { while (tickInfo[kIndex] < tickInfo[kLength]) { + const has_hooks = async_hook_fields[kActiveHooks] > 0; + // TODO(trevnorris): These should always be zero if at the bottom of + // the stack. Add check somewhere cheaper than here to make sure that's + // the case. + const prev_id = async_uid_fields[kCurrentId]; + const prev_trigger_id = async_uid_fields[kTriggerId]; + tock = nextTickQueue[tickInfo[kIndex]++]; callback = tock.callback; domain = tock.domain; args = tock.args; if (domain) domain.enter(); + + // Setting these fields manually with the expectation that has_hooks + // will almost always be false. Overhead for doing it twice if it is + // true is negligible anyway. + async_uid_fields[kCurrentId] = tock._asyncId; + async_uid_fields[kTriggerId] = tock._triggerId; + if (has_hooks && async_hook_fields[kBefore]) + async_hooks.emitBefore(tock._asyncId, tock._triggerId); + // Using separate callback execution functions allows direct // callback invocation with small numbers of arguments to avoid the // performance hit associated with using `fn.apply()` _combinedTickCallback(args, callback); + + if (has_hooks) { + if (async_hook_fields[kAfter]) + async_hooks.emitAfter(tock._asyncId); + if (async_hook_fields[kDestroy]) + async_hooks.emitDestroy(tock._asyncId); + } + async_uid_fields[kCurrentId] = prev_id; + async_uid_fields[kTriggerId] = prev_trigger_id; + if (1e4 < tickInfo[kIndex]) tickDone(); if (domain) @@ -131,6 +190,26 @@ function setupNextTick() { } while (tickInfo[kLength] !== 0); } + function TickObject(callback, args, domain, isMicrotask) { + this.callback = callback; + this.domain = domain; + this.args = args; + this._asyncId = -1; + this._triggerId = -1; + if (!isMicrotask) + setupInit(this); + } + + // This function is for performance reasons. If the code here is placed + // directly in TickObject it would be over the maximum character count + // to be inlined, and it would suffer a penalty lose. + function setupInit(self) { + self._asyncId = ++async_uid_fields[kAsyncUidCntr]; + self._triggerId = initTriggerId(); + if (async_hook_fields[kInit] > 0) + async_hooks.emitInit(self._asyncId, 'TickObject', self._triggerId, self); + } + function nextTick(callback) { if (typeof callback !== 'function') throw new TypeError('callback is not a function'); @@ -145,11 +224,8 @@ function setupNextTick() { args[i - 1] = arguments[i]; } - nextTickQueue.push({ - callback, - domain: process.domain || null, - args - }); + nextTickQueue.push( + new TickObject(callback, args, process.domain || null, false)); tickInfo[kLength]++; } } From 977718eea6ed71807d4ce4fd9f56191b99a7b1ef Mon Sep 17 00:00:00 2001 From: Trevor Norris Date: Wed, 16 Nov 2016 15:10:34 -0700 Subject: [PATCH 12/40] timers: initial timers support Also change a couple things to retrieve the initTriggerId the same way in all cases. --- lib/timers.js | 65 ++++++++++++++++++++++++++++++++++++++++++++++++++- 1 file changed, 64 insertions(+), 1 deletion(-) diff --git a/lib/timers.js b/lib/timers.js index 6d456da36faf67..1f732500cb9c73 100644 --- a/lib/timers.js +++ b/lib/timers.js @@ -1,11 +1,19 @@ 'use strict'; +const async_wrap = process.binding('async_wrap'); const TimerWrap = process.binding('timer_wrap').Timer; const L = require('internal/linkedlist'); const assert = require('assert'); const util = require('util'); const debug = util.debuglog('timer'); const kOnTimeout = TimerWrap.kOnTimeout | 0; +// Two arrays that share state between C++ and JS. +const { async_uid_fields, async_hook_fields } = async_wrap; +const { emitInit, emitBefore, emitAfter, emitDestroy, + initTriggerId } = require('async_hooks'); +// Grab the constants necessary for working with internal arrays. +const { kInit, kBefore, kAfter, kDestroy, + kAsyncUidCntr } = async_wrap.constants; // Timeout values > TIMEOUT_MAX are set to 1. const TIMEOUT_MAX = 2147483647; // 2^31-1 @@ -131,6 +139,14 @@ function insert(item, unrefed) { lists[msecs] = list = createTimersList(msecs, unrefed); } + // No UID, assign a new one. + if (!item._asyncId) { + item._asyncId = ++async_uid_fields[kAsyncUidCntr]; + item._triggerId = initTriggerId(); + if (async_hook_fields[kInit] > 0) + emitInit(item._asyncId, 'Timeout', this._triggerId, this); + } + L.append(list, item); assert(!L.isEmpty(list)); // list is not empty } @@ -188,7 +204,12 @@ function listOnTimeout() { L.remove(timer); assert(timer !== L.peek(list)); - if (!timer._onTimeout) continue; + if (!timer._onTimeout) { + if (timer._asyncId > 0 && async_hook_fields[kDestroy]) + emitDestroy(timer._asyncId); + timer._asyncId = timer._triggerId = -1; + continue; + } var domain = timer.domain; if (domain) { @@ -204,8 +225,17 @@ function listOnTimeout() { domain.enter(); } + if (async_hook_fields[kBefore]) + emitBefore(timer._asyncId, timer._triggerId); + tryOnTimeout(timer, list); + if (async_hook_fields[kAfter]) + emitAfter(timer._asyncId); + if (!timer._repeat && async_hook_fields[kDestroy]) + emitDestroy(timer._asyncId); + timer._asyncId = timer._triggerId = -1; + if (domain) domain.exit(); } @@ -280,6 +310,12 @@ function reuse(item) { // Remove a timer. Cancels the timeout and resets the relevant timer properties. const unenroll = exports.unenroll = function(item) { + if (item._asyncId > 0) { + if (async_hook_fields[kDestroy]) + emitDestroy(item._asyncId); + item._asyncId = item._triggerId = -1; + } + var handle = reuse(item); if (handle) { debug('unenroll: list empty'); @@ -462,6 +498,8 @@ function Timeout(after, callback, args) { this._onTimeout = callback; this._timerArgs = args; this._repeat = null; + this._asyncId = -1; + this._triggerId = -1; } @@ -588,6 +626,10 @@ function processImmediate() { if (domain) domain.enter(); + if (immediate._asyncId > 0 && async_hook_fields[kBefore]) { + emitBefore(immediate._asyncId, immediate._triggerId); + } + immediate._callback = immediate._onImmediate; // Save next in case `clearImmediate(immediate)` is called from callback @@ -595,6 +637,17 @@ function processImmediate() { tryOnImmediate(immediate, tail); + if (immediate._asyncId > 0 && async_hook_fields[kAfter]) { + emitAfter(immediate._asyncId); + } + + if (immediate._asyncId > 0) { + if (async_hook_fields[kDestroy]) { + emitDestroy(immediate._asyncId); + } + immediate._asyncId = immediate._triggerId = -1; + } + if (domain) domain.exit(); @@ -671,6 +724,10 @@ function Immediate() { this._argv = null; this._onImmediate = null; this.domain = process.domain; + this._asyncId = ++async_uid_fields[kAsyncUidCntr]; + this._triggerId = initTriggerId(); + if (async_hook_fields[kInit]) + emitInit(this._asyncId, 'Immediate', this._triggerId, this); } exports.setImmediate = function(callback, arg1, arg2, arg3) { @@ -728,4 +785,10 @@ exports.clearImmediate = function(immediate) { if (!immediateQueue.head) { process._needImmediateCallback = false; } + + if (immediate._asyncId > 0) { + if (async_hook_fields[kDestroy]) + emitDestroy(immediate._asyncId); + immediate._asyncId = immediate._triggerId = -1; + } }; From 3e5ef643c91868a99b9f9a4b17d379e9646b92bb Mon Sep 17 00:00:00 2001 From: Trevor Norris Date: Thu, 17 Nov 2016 00:25:14 -0700 Subject: [PATCH 13/40] fs: add initial async_hooks support The placement of setting/updating the triggerId needs to be tested to make sure basic assumptions are met. Also fix propagation of the triggerId for fs.open(), remove adding information to the internal unordered_map for fs.openSync() and return the Float64Array to simplify usage. --- lib/fs.js | 44 ++++++++++++++++++++++++++++++++++++++++++++ src/node_file.cc | 7 +------ 2 files changed, 45 insertions(+), 6 deletions(-) diff --git a/lib/fs.js b/lib/fs.js index 573ffec05992aa..763b10b5036e7c 100644 --- a/lib/fs.js +++ b/lib/fs.js @@ -20,6 +20,8 @@ const assertEncoding = internalFS.assertEncoding; const stringToFlags = internalFS.stringToFlags; const SyncWriteStream = internalFS.SyncWriteStream; +const { currentId, setInitTriggerId } = require('async_hooks'); + Object.defineProperty(exports, 'constants', { configurable: false, enumerable: true, @@ -37,6 +39,9 @@ const isWindows = process.platform === 'win32'; const DEBUG = process.env.NODE_DEBUG && /fs/.test(process.env.NODE_DEBUG); const errnoException = util._errnoException; +// Used to pull the id and triggerId from getIdsFromFd(). +const f64array = new Float64Array(2); + function getOptions(options, defaultOptions) { if (options === null || options === undefined || typeof options === 'function') { @@ -129,6 +134,16 @@ function isFd(path) { return (path >>> 0) === path; } +// Helper function to reduce clutter. +function setActualInitTriggerId(self) { + if (self._asyncId > 0) { + setInitTriggerId(self._asyncId); + return; + } + binding.getIdsFromFd(self.fd, f64array); + setInitTriggerId(f64array[0] > 0 ? f64array[0] : currentId()); +} + // Static method to set the stats properties on a Stats object. fs.Stats = function( dev, @@ -265,16 +280,23 @@ fs.readFile = function(path, options, callback) { req.oncomplete = readFileAfterOpen; if (context.isUserFd) { + // If the fd was opened synchronously then there will be no asyncId. + binding.getIdsFromFd(path, f64array); + context._asyncId = f64array[0] > 0 ? f64array[0] : currentId(); + setInitTriggerId(context._asyncId); process.nextTick(function() { req.oncomplete(null, path); }); return; } + // No need to setInitTriggerId() here since the triggerId and currentId are + // the same. binding.open(pathModule._makeLong(path), stringToFlags(options.flag || 'r'), 0o666, req); + context._asyncId = req.getAsyncId(); }; const kReadFileBufferLength = 8 * 1024; @@ -289,6 +311,10 @@ function ReadFileContext(callback, encoding) { this.pos = 0; this.encoding = encoding; this.err = null; + // No need to store _triggerId because it won't be passed on to other + // resources, and the value is stored in the ReqWrap for the call to + // MakeCallback. + this._asyncId = -1; } ReadFileContext.prototype.read = function() { @@ -310,7 +336,12 @@ ReadFileContext.prototype.read = function() { req.oncomplete = readFileAfterRead; req.context = this; + setActualInitTriggerId(this); binding.read(this.fd, buffer, offset, length, -1, req); + // Since a ReadFileContext encapsulates many interactions with a file, update + // the _asyncId of the instance to the latest interaction so that it properly + // propagates to the next action's triggerId. + this._asyncId = req.getAsyncId(); }; ReadFileContext.prototype.close = function(err) { @@ -319,6 +350,9 @@ ReadFileContext.prototype.close = function(err) { req.context = this; this.err = err; + // This will be needed for either the nextTick() or .close(). + setActualInitTriggerId(this); + if (this.isUserFd) { process.nextTick(function() { req.oncomplete(null); @@ -327,6 +361,7 @@ ReadFileContext.prototype.close = function(err) { } binding.close(this.fd, req); + this._asyncId = req.getAsyncId(); }; function readFileAfterOpen(err, fd) { @@ -1656,6 +1691,10 @@ function ReadStream(path, options) { this.autoClose = options.autoClose === undefined ? true : options.autoClose; this.pos = undefined; this.bytesRead = 0; + // If no fd or asyncId is passed then assume that .open() will be called and + // that _asyncId will be assigned at that time. + this._asyncId = -1; + if (this.start !== undefined) { if (typeof this.start !== 'number') { @@ -1735,6 +1774,7 @@ ReadStream.prototype._read = function(n) { // the actual read. var self = this; + setActualInitTriggerId(this); fs.read(this.fd, pool, pool.used, toRead, this.pos, onread); // move the pool positions, and internal position for reading. @@ -1778,12 +1818,14 @@ ReadStream.prototype.close = function(cb) { this.once('open', close); return; } + setActualInitTriggerId(this); return process.nextTick(() => this.emit('close')); } this.closed = true; close(); function close(fd) { + setActualInitTriggerId(self); fs.close(fd || self.fd, function(er) { if (er) self.emit('error', er); @@ -1873,6 +1915,7 @@ WriteStream.prototype._write = function(data, encoding, cb) { }); var self = this; + setActualInitTriggerId(this); fs.write(this.fd, data, 0, data.length, this.pos, function(er, bytes) { if (er) { if (self.autoClose) { @@ -1919,6 +1962,7 @@ WriteStream.prototype._writev = function(data, cb) { size += chunk.length; } + setActualInitTriggerId(this); writev(this.fd, chunks, this.pos, function(er, bytes) { if (er) { self.destroy(); diff --git a/src/node_file.cc b/src/node_file.cc index 6646da09c86fb6..827fd44ab03fdb 100644 --- a/src/node_file.cc +++ b/src/node_file.cc @@ -200,7 +200,7 @@ static void After(uv_fs_t *req) { case UV_FS_OPEN: env->insert_fd_async_ids(req->result, req_wrap->get_id(), - env->exchange_init_trigger_id(0)); + req_wrap->get_trigger_id()); argv[1] = Integer::New(env->isolate(), req->result); break; @@ -417,7 +417,6 @@ static void Close(const FunctionCallbackInfo& args) { int fd = args[0]->Int32Value(); // TODO(trevnorris): Won't these values be needed for the destroy callbacks? - // XXX Wouldn't fd_async_id_map_[n][1] be the triggerId of the close() call? env->erase_fd_async_id(fd); if (args[1]->IsObject()) { @@ -1020,10 +1019,6 @@ static void Open(const FunctionCallbackInfo& args) { ASYNC_CALL(open, args[3], UTF8, *path, flags, mode) } else { SYNC_CALL(open, *path, *path, flags, mode) - // TODO(trevnorris): Is this really necessary for sync calls? - env->insert_fd_async_ids(SYNC_RESULT, - env->current_async_id(), - env->trigger_id()); args.GetReturnValue().Set(SYNC_RESULT); } } From 955d390734535a4e81d02117b64fc4853b97e7da Mon Sep 17 00:00:00 2001 From: Trevor Norris Date: Wed, 16 Nov 2016 16:59:40 -0700 Subject: [PATCH 14/40] net: add initial async_hooks net support cluster is still slightly messed up, as I figure out how to handle the "faux" handles passed to net. --- lib/internal/cluster/child.js | 8 +++++++- lib/internal/cluster/shared_handle.js | 6 ++++-- lib/net.js | 20 ++++++++++++++++++++ test/parallel/test-net-end-close.js | 3 ++- 4 files changed, 33 insertions(+), 4 deletions(-) diff --git a/lib/internal/cluster/child.js b/lib/internal/cluster/child.js index 3275eec720c738..919cb0728b8ef7 100644 --- a/lib/internal/cluster/child.js +++ b/lib/internal/cluster/child.js @@ -145,7 +145,13 @@ function rr(message, indexesKey, cb) { // with it. Fools net.Server into thinking that it's backed by a real // handle. Use a noop function for ref() and unref() because the control // channel is going to keep the worker alive anyway. - const handle = { close, listen, ref: noop, unref: noop }; + // + // Emulate AsyncWrap::GetAsyncId() by returning a value from getAsyncId(). + // TODO(trevnorris): Right now just return -2 so locations where this is + // propagating can be identified, if there are any. Since this is a faux + // handle not sure how to treat it (i.e. should it receive its own uid?). + const handle = { + close, listen, ref: noop, unref: noop, getAsyncId: () => -2 }; if (message.sockname) { handle.getsockname = getsockname; // TCP handles only. diff --git a/lib/internal/cluster/shared_handle.js b/lib/internal/cluster/shared_handle.js index c0663772426213..4d95b75659f0fc 100644 --- a/lib/internal/cluster/shared_handle.js +++ b/lib/internal/cluster/shared_handle.js @@ -19,10 +19,12 @@ function SharedHandle(key, address, port, addressType, fd, flags) { else rval = net._createServerHandle(address, port, addressType, fd); - if (typeof rval === 'number') + if (typeof rval === 'number') { this.errno = rval; - else + } else { this.handle = rval; + this.getAsyncId = this.handle.getAsyncId; + } } SharedHandle.prototype.add = function(worker, send) { diff --git a/lib/net.js b/lib/net.js index e4f97ab80debd5..ba1675f2ab33e3 100644 --- a/lib/net.js +++ b/lib/net.js @@ -19,6 +19,7 @@ const PipeConnectWrap = process.binding('pipe_wrap').PipeConnectWrap; const ShutdownWrap = process.binding('stream_wrap').ShutdownWrap; const WriteWrap = process.binding('stream_wrap').WriteWrap; +const setInitTriggerId = require('async_hooks').setInitTriggerId; var cluster; const errnoException = util._errnoException; @@ -114,6 +115,7 @@ function initSocketHandle(self) { if (self._handle) { self._handle.owner = self; self._handle.onread = onread; + self._asyncId = self._handle.getAsyncId(); // If handle doesn't support writev - neither do we if (!self._handle.writev) @@ -129,6 +131,7 @@ function Socket(options) { if (!(this instanceof Socket)) return new Socket(options); this.connecting = false; + this._asyncId = -1; this._hadError = false; this._handle = null; this._parent = null; @@ -143,9 +146,11 @@ function Socket(options) { if (options.handle) { this._handle = options.handle; // private + this._asyncId = this._handle.getAsyncId(); } else if (options.fd !== undefined) { this._handle = createHandle(options.fd); this._handle.open(options.fd); + this._asyncId = this._handle.getAsyncId(); if ((options.fd == 1 || options.fd == 2) && (this._handle instanceof Pipe) && process.platform === 'win32') { @@ -228,6 +233,7 @@ function onSocketFinish() { if (!this._handle || !this._handle.shutdown) return this.destroy(); + setInitTriggerId(this._asyncId); var req = new ShutdownWrap(); req.oncomplete = afterShutdown; req.handle = this._handle; @@ -296,6 +302,7 @@ function writeAfterFIN(chunk, encoding, cb) { // TODO: defer error events consistently everywhere, not just the cb this.emit('error', er); if (typeof cb === 'function') { + setInitTriggerId(this._asyncId); process.nextTick(cb, er); } } @@ -854,6 +861,7 @@ function connect(self, address, port, addressType, localAddress, localPort) { } if (addressType === 6 || addressType === 4) { + setInitTriggerId(self._asyncId); const req = new TCPConnectWrap(); req.oncomplete = afterConnect; req.address = address; @@ -867,6 +875,7 @@ function connect(self, address, port, addressType, localAddress, localPort) { err = self._handle.connect6(req, address, port); } else { + setInitTriggerId(self._asyncId); const req = new PipeConnectWrap(); req.address = address; req.oncomplete = afterConnect; @@ -968,6 +977,7 @@ function lookupAndConnect(self, options) { // If host is an IP, skip performing a lookup var addressType = exports.isIP(host); if (addressType) { + setInitTriggerId(self._asyncId); process.nextTick(function() { if (self.connecting) connect(self, host, port, addressType, localAddress, localPort); @@ -1061,6 +1071,7 @@ function afterConnect(status, handle, req, readable, writable) { // Update handle if it was wrapped // TODO(indutny): assert that the handle is actually an ancestor of old one + // TODO(trevnorris): Doesn't look like this does anything. Verify and remove. handle = self._handle; debug('afterConnect'); @@ -1137,6 +1148,7 @@ function Server(options, connectionListener) { configurable: true, enumerable: false }); + this._asyncId = -1; this._handle = null; this._usingSlaves = false; this._slaves = []; @@ -1252,6 +1264,7 @@ Server.prototype._listen2 = function(address, port, addressType, backlog, fd) { this._handle = rval; } + this._asyncId = this._handle.getAsyncId(); this._handle.onconnection = onconnection; this._handle.owner = this; @@ -1261,6 +1274,7 @@ Server.prototype._listen2 = function(address, port, addressType, backlog, fd) { var ex = exceptionWithHostPort(err, 'listen', address, port); this._handle.close(); this._handle = null; + setInitTriggerId(this._asyncId); process.nextTick(emitErrorNT, this, ex); return; } @@ -1272,6 +1286,7 @@ Server.prototype._listen2 = function(address, port, addressType, backlog, fd) { if (this._unref) this.unref(); + setInitTriggerId(this._asyncId); process.nextTick(emitListeningNT, this); }; @@ -1357,6 +1372,7 @@ Server.prototype.listen = function() { if (options instanceof TCP) { this._handle = options; + this._asyncId = this._handle.getAsyncId(); listen(this, null, -1, -1, backlog); } else if (typeof options.fd === 'number' && options.fd >= 0) { listen(this, null, null, null, backlog, options.fd); @@ -1455,7 +1471,10 @@ function onconnection(err, clientHandle) { Server.prototype.getConnections = function(cb) { + const self = this; + function end(err, connections) { + setInitTriggerId(self._asyncId); process.nextTick(cb, err, connections); } @@ -1534,6 +1553,7 @@ Server.prototype._emitCloseIfDrained = function() { return; } + setInitTriggerId(this._asyncId); process.nextTick(emitCloseNT, this); }; diff --git a/test/parallel/test-net-end-close.js b/test/parallel/test-net-end-close.js index d9f33fd7d8d1cf..36882f073da705 100644 --- a/test/parallel/test-net-end-close.js +++ b/test/parallel/test-net-end-close.js @@ -10,7 +10,8 @@ const s = new net.Socket({ readStart: function() { process.nextTick(() => this.onread(uv.UV_EOF, null)); }, - close: (cb) => process.nextTick(cb) + close: (cb) => process.nextTick(cb), + getAsyncId: () => 0, }, writable: false }); From da7c67c9ace11cc2845c6f880efca2c500a07991 Mon Sep 17 00:00:00 2001 From: Trevor Norris Date: Thu, 17 Nov 2016 00:35:15 -0700 Subject: [PATCH 15/40] lib: add triggerId support to various modules Properly propagate the triggerId in a few more modules. --- lib/_http_common.js | 10 +++++++++- lib/_http_outgoing.js | 6 +++++- lib/dgram.js | 3 +++ 3 files changed, 17 insertions(+), 2 deletions(-) diff --git a/lib/_http_common.js b/lib/_http_common.js index b8724f00ec6557..adb1658f44ffdc 100644 --- a/lib/_http_common.js +++ b/lib/_http_common.js @@ -18,6 +18,8 @@ exports.chunkExpression = /(?:^|\W)chunked(?:$|\W)/i; exports.continueExpression = /(?:^|\W)100-continue(?:$|\W)/i; exports.methods = methods; +const emitDestroy = require('async_hooks').emitDestroy; + const kOnHeaders = HTTPParser.kOnHeaders | 0; const kOnHeadersComplete = HTTPParser.kOnHeadersComplete | 0; const kOnBody = HTTPParser.kOnBody | 0; @@ -196,8 +198,14 @@ function freeParser(parser, req, socket) { parser.incoming = null; parser.outgoing = null; parser[kOnExecute] = null; - if (parsers.free(parser) === false) + if (parsers.free(parser) === false) { parser.close(); + } else { + // Since the Parser destructor isn't going to run the destroy() callbacks + // need to be triggered manually. + emitDestroy(parser.getAsyncId()); + } + parser = null; } if (req) { req.parser = null; diff --git a/lib/_http_outgoing.js b/lib/_http_outgoing.js index 30011e091525a3..25738a6220229f 100644 --- a/lib/_http_outgoing.js +++ b/lib/_http_outgoing.js @@ -9,6 +9,7 @@ const Buffer = require('buffer').Buffer; const common = require('_http_common'); const checkIsHttpToken = common._checkIsHttpToken; const checkInvalidHeaderChar = common._checkInvalidHeaderChar; +const setInitTriggerId = require('async_hooks').setInitTriggerId; const CRLF = common.CRLF; const debug = common.debug; @@ -153,8 +154,10 @@ function _writeRaw(data, encoding, callback) { if (this.output.length) { this._flushOutput(conn); } else if (!data.length) { - if (typeof callback === 'function') + if (typeof callback === 'function') { + setInitTriggerId(this.socket._asyncId); process.nextTick(callback); + } return true; } // Directly write to socket. @@ -477,6 +480,7 @@ const crlf_buf = Buffer.from('\r\n'); OutgoingMessage.prototype.write = function write(chunk, encoding, callback) { if (this.finished) { var err = new Error('write after end'); + setInitTriggerId(this.socket._asyncId); process.nextTick(writeAfterEndNT, this, err, callback); return true; diff --git a/lib/dgram.js b/lib/dgram.js index 39d43092e15832..1958ad728a545b 100644 --- a/lib/dgram.js +++ b/lib/dgram.js @@ -4,6 +4,7 @@ const assert = require('assert'); const Buffer = require('buffer').Buffer; const util = require('util'); const EventEmitter = require('events'); +const setInitTriggerId = require('async_hooks').setInitTriggerId; const UV_UDP_REUSEADDR = process.binding('constants').os.UV_UDP_REUSEADDR; const UDP = process.binding('udp_wrap').UDP; @@ -89,6 +90,7 @@ function Socket(type, listener) { this._handle = handle; this._receiving = false; this._bindState = BIND_STATE_UNBOUND; + this._asyncId = handle.getAsyncId(); this.type = type; this.fd = null; // compatibility hack @@ -378,6 +380,7 @@ function doSend(ex, self, ip, list, address, port, callback) { return; } + setInitTriggerId(self._asyncId); var req = new SendWrap(); req.list = list; // Keep reference alive. req.address = address; From b3721baee894c34cb84ff0e8527fd01257354c93 Mon Sep 17 00:00:00 2001 From: Trevor Norris Date: Thu, 17 Nov 2016 01:17:43 -0700 Subject: [PATCH 16/40] http: reset Agents on free pool Add function on native side to run AsyncWrap::Reset() from JS. Only add this to TCPWrap, since it's the only place necessary. --- lib/_http_agent.js | 8 ++++++++ lib/_http_client.js | 4 ++++ src/async-wrap.cc | 7 +++++++ src/async-wrap.h | 2 ++ src/tcp_wrap.cc | 1 + 5 files changed, 22 insertions(+) diff --git a/lib/_http_agent.js b/lib/_http_agent.js index eebdb242463b5d..91d9b6272ec137 100644 --- a/lib/_http_agent.js +++ b/lib/_http_agent.js @@ -4,6 +4,7 @@ const net = require('net'); const util = require('util'); const EventEmitter = require('events'); const debug = util.debuglog('http'); +const setInitTriggerId = require('async_hooks').setInitTriggerId; // New Agent code. @@ -72,6 +73,7 @@ function Agent(options) { self.freeSockets[name] = freeSockets; socket.setKeepAlive(true, self.keepAliveMsecs); socket.unref(); + socket._asyncId = -1; socket._httpMessage = null; self.removeSocket(socket, options); freeSockets.push(socket); @@ -142,6 +144,8 @@ Agent.prototype.addRequest = function addRequest(req, options) { if (freeLen) { // we have a free socket, so use that. var socket = this.freeSockets[name].shift(); + // Assign the handle a new asyncId and run any init() hooks. + socket._handle.asyncReset(); debug('have free socket'); // don't leak @@ -154,8 +158,11 @@ Agent.prototype.addRequest = function addRequest(req, options) { } else if (sockLen < this.maxSockets) { debug('call onSocket', sockLen, freeLen); // If we are under maxSockets create a new one. + // Agent#addRequest() is only called from the ClientRequest constructor. So + // it's unnecessary to set the parent id before calling createSocket(). this.createSocket(req, options, function(err, newSocket) { if (err) { + setInitTriggerId(newSocket._handle.getAsyncId()); process.nextTick(function() { req.emit('error', err); }); @@ -269,6 +276,7 @@ Agent.prototype.removeSocket = function removeSocket(s, options) { // If we have pending requests and a socket gets closed make a new one this.createSocket(req, options, function(err, newSocket) { if (err) { + setInitTriggerId(newSocket._handle.getAsyncId()); process.nextTick(function() { req.emit('error', err); }); diff --git a/lib/_http_client.js b/lib/_http_client.js index cc2435089a9f3e..bcd096c0558792 100644 --- a/lib/_http_client.js +++ b/lib/_http_client.js @@ -13,6 +13,7 @@ const debug = common.debug; const OutgoingMessage = require('_http_outgoing').OutgoingMessage; const Agent = require('_http_agent'); const Buffer = require('buffer').Buffer; +const setInitTriggerId = require('async_hooks').setInitTriggerId; const urlToOptions = require('internal/url').urlToOptions; // The actual list of disallowed characters in regexp form is more like: @@ -563,6 +564,9 @@ function responseKeepAlive(res, req) { socket.removeListener('close', socketCloseListener); socket.removeListener('error', socketErrorListener); socket.once('error', freeSocketErrorListener); + // There are cases where _handle === null. Avoid those. + if (socket._handle) + setInitTriggerId(socket._handle.getAsyncId()); // Mark this socket as available, AFTER user-added end // handlers have a chance to run. process.nextTick(emitFreeNT, socket); diff --git a/src/async-wrap.cc b/src/async-wrap.cc index 5e1d9a8a2889e2..20c6a07814278b 100644 --- a/src/async-wrap.cc +++ b/src/async-wrap.cc @@ -286,6 +286,13 @@ void AsyncWrap::DestroyIdsCb(uv_idle_t* handle) { } +void AsyncWrap::AsyncReset(const FunctionCallbackInfo& args) { + AsyncWrap* wrap; + ASSIGN_OR_RETURN_UNWRAP(&wrap, args.Holder()); + wrap->Reset(); +} + + void LoadAsyncWrapperInfo(Environment* env) { HeapProfiler* heap_profiler = env->isolate()->GetHeapProfiler(); #define V(PROVIDER) \ diff --git a/src/async-wrap.h b/src/async-wrap.h index abb2fb5be36b67..04ad876727acb3 100644 --- a/src/async-wrap.h +++ b/src/async-wrap.h @@ -67,6 +67,8 @@ class AsyncWrap : public BaseObject { static void DestroyIdsCb(uv_idle_t* handle); + static void AsyncReset(const v8::FunctionCallbackInfo& args); + inline ProviderType provider_type() const; inline double get_id() const; diff --git a/src/tcp_wrap.cc b/src/tcp_wrap.cc index 4c6508624e2d5e..e2dee8abfec010 100644 --- a/src/tcp_wrap.cc +++ b/src/tcp_wrap.cc @@ -65,6 +65,7 @@ void TCPWrap::Initialize(Local target, Null(env->isolate())); env->SetProtoMethod(t, "getAsyncId", AsyncWrap::GetAsyncId); + env->SetProtoMethod(t, "asyncReset", AsyncWrap::AsyncReset); env->SetProtoMethod(t, "close", HandleWrap::Close); From 78a2a7fec926ef766a76b2e7c9fd23913d1fac56 Mon Sep 17 00:00:00 2001 From: Trevor Norris Date: Thu, 17 Nov 2016 15:37:32 -0700 Subject: [PATCH 17/40] src: properly propagate triggerId in more places * Setup the currentId and triggerId during bootstrap. * Assign _asyncId and _triggerId on construction of Timeout * Always run emitBefore/emitAfter in timers to make sure ids are setup * Reset ids after after() hooks are called, not before --- lib/async_hooks.js | 21 +++++++++++---------- lib/timers.js | 39 ++++++++++++++------------------------- src/env-inl.h | 2 ++ src/node.cc | 1 + 4 files changed, 28 insertions(+), 35 deletions(-) diff --git a/lib/async_hooks.js b/lib/async_hooks.js index b305e00d3cce34..7d84cdfff626b4 100644 --- a/lib/async_hooks.js +++ b/lib/async_hooks.js @@ -207,7 +207,7 @@ function initTriggerId() { // Reset value after it's been called so the next constructor doesn't // inherit it by accident. else async_uid_fields[kInitTriggerId] = 0; - if (tId < 0) tId = async_uid_fields[kCurrentId]; + if (tId <= 0) tId = async_uid_fields[kCurrentId]; return tId; } @@ -281,11 +281,21 @@ function emitBeforeS(id, triggerId) { function emitAfterS(id) { + if (async_hook_fields[kAfter] > 0) { + for (var i = 0; i < active_hooks_array.length; i++) { + if (typeof active_hooks_array[i][after_symbol] === 'function') { + runCallback(active_hooks_array[i][after_symbol], id); + } + } + } + // Remove state after the call has completed. // TODO(trevnorris): Clearing the native counters is easy, but also must // track the length of the array manually so the native side can empty it // in case there's an error. Or, export the array in such a way that // fatalException can empty it as well. + // TODO(trevnorris): Add a check that kCurrentId === id to make sure the + // stack hasn't been corrupted. if (current_trigger_id_stack.length > 0) { async_uid_fields[kTriggerId] = current_trigger_id_stack.pop(); async_uid_fields[kCurrentId] = current_trigger_id_stack.pop(); @@ -293,15 +303,6 @@ function emitAfterS(id) { async_uid_fields[kTriggerId] = 0; async_uid_fields[kCurrentId] = 0; } - - if (active_hooks_array[kAfter] === 0) { - return; - } - for (var i = 0; i < active_hooks_array.length; i++) { - if (typeof active_hooks_array[i][after_symbol] === 'function') { - runCallback(active_hooks_array[i][after_symbol], id); - } - } } diff --git a/lib/timers.js b/lib/timers.js index 1f732500cb9c73..c0aba1a2bc0188 100644 --- a/lib/timers.js +++ b/lib/timers.js @@ -12,8 +12,7 @@ const { async_uid_fields, async_hook_fields } = async_wrap; const { emitInit, emitBefore, emitAfter, emitDestroy, initTriggerId } = require('async_hooks'); // Grab the constants necessary for working with internal arrays. -const { kInit, kBefore, kAfter, kDestroy, - kAsyncUidCntr } = async_wrap.constants; +const { kInit, kDestroy, kAsyncUidCntr } = async_wrap.constants; // Timeout values > TIMEOUT_MAX are set to 1. const TIMEOUT_MAX = 2147483647; // 2^31-1 @@ -144,7 +143,8 @@ function insert(item, unrefed) { item._asyncId = ++async_uid_fields[kAsyncUidCntr]; item._triggerId = initTriggerId(); if (async_hook_fields[kInit] > 0) - emitInit(item._asyncId, 'Timeout', this._triggerId, this); + // TODO(trevnorris): Is the "type" of 'Timeout' appropriate here? + emitInit(item._asyncId, 'Timeout', item._triggerId, item); } L.append(list, item); @@ -207,7 +207,6 @@ function listOnTimeout() { if (!timer._onTimeout) { if (timer._asyncId > 0 && async_hook_fields[kDestroy]) emitDestroy(timer._asyncId); - timer._asyncId = timer._triggerId = -1; continue; } @@ -225,16 +224,13 @@ function listOnTimeout() { domain.enter(); } - if (async_hook_fields[kBefore]) - emitBefore(timer._asyncId, timer._triggerId); + emitBefore(timer._asyncId, timer._triggerId); tryOnTimeout(timer, list); - if (async_hook_fields[kAfter]) - emitAfter(timer._asyncId); + emitAfter(timer._asyncId); if (!timer._repeat && async_hook_fields[kDestroy]) emitDestroy(timer._asyncId); - timer._asyncId = timer._triggerId = -1; if (domain) domain.exit(); @@ -313,7 +309,6 @@ const unenroll = exports.unenroll = function(item) { if (item._asyncId > 0) { if (async_hook_fields[kDestroy]) emitDestroy(item._asyncId); - item._asyncId = item._triggerId = -1; } var handle = reuse(item); @@ -498,8 +493,10 @@ function Timeout(after, callback, args) { this._onTimeout = callback; this._timerArgs = args; this._repeat = null; - this._asyncId = -1; - this._triggerId = -1; + this._asyncId = ++async_uid_fields[kAsyncUidCntr]; + this._triggerId = initTriggerId(); + if (async_hook_fields[kInit] > 0) + emitInit(this._asyncId, 'Timeout', this._triggerId, this); } @@ -626,9 +623,7 @@ function processImmediate() { if (domain) domain.enter(); - if (immediate._asyncId > 0 && async_hook_fields[kBefore]) { - emitBefore(immediate._asyncId, immediate._triggerId); - } + emitBefore(immediate._asyncId, immediate._triggerId); immediate._callback = immediate._onImmediate; @@ -637,16 +632,10 @@ function processImmediate() { tryOnImmediate(immediate, tail); - if (immediate._asyncId > 0 && async_hook_fields[kAfter]) { - emitAfter(immediate._asyncId); - } - - if (immediate._asyncId > 0) { - if (async_hook_fields[kDestroy]) { - emitDestroy(immediate._asyncId); - } - immediate._asyncId = immediate._triggerId = -1; - } + emitAfter(immediate._asyncId); + if (async_hook_fields[kDestroy]) + emitDestroy(immediate._asyncId); + immediate._asyncId = immediate._triggerId = -1; if (domain) domain.exit(); diff --git a/src/env-inl.h b/src/env-inl.h index 41aef8b7009aea..c912d7cfd20001 100644 --- a/src/env-inl.h +++ b/src/env-inl.h @@ -60,6 +60,8 @@ inline uint32_t* IsolateData::zero_fill_field() const { } inline Environment::AsyncHooks::AsyncHooks() : fields_(), uid_fields_() { + // kAsyncUidCntr should start at 1 because that'll be the id for bootstrap. + uid_fields_[AsyncHooks::kAsyncUidCntr] = 1; } inline uint32_t* Environment::AsyncHooks::fields() { diff --git a/src/node.cc b/src/node.cc index b247db816ef9a4..25008344059213 100644 --- a/src/node.cc +++ b/src/node.cc @@ -4360,6 +4360,7 @@ inline int Start(Isolate* isolate, IsolateData* isolate_data, { Environment::AsyncCallbackScope callback_scope(&env); + Environment::AsyncHooks::ExecScope exec_scope(&env, 1, 0); LoadEnvironment(&env); } From eab9e86e4771e5196bfed3587b9e0581ffd1e660 Mon Sep 17 00:00:00 2001 From: Trevor Norris Date: Thu, 17 Nov 2016 17:07:21 -0700 Subject: [PATCH 18/40] timers: manually set ids if no hooks exist Instead of always calling into emitBefore/emitAfter, set the ids manually if there are no hooks to call. Better performance. --- lib/internal/process/next_tick.js | 7 +++---- lib/timers.js | 20 ++++++++++++++++---- 2 files changed, 19 insertions(+), 8 deletions(-) diff --git a/lib/internal/process/next_tick.js b/lib/internal/process/next_tick.js index 2aaa03443ba2a3..a83e9ca5a59204 100644 --- a/lib/internal/process/next_tick.js +++ b/lib/internal/process/next_tick.js @@ -111,7 +111,7 @@ function setupNextTick() { // true is negligible anyway. async_uid_fields[kCurrentId] = tock._asyncId; async_uid_fields[kTriggerId] = tock._triggerId; - if (has_hooks && async_hook_fields[kBefore]) + if (has_hooks && async_hook_fields[kBefore] > 0) async_hooks.emitBefore(tock._asyncId, tock._triggerId); // Using separate callback execution functions allows direct @@ -120,12 +120,11 @@ function setupNextTick() { _combinedTickCallback(args, callback); if (has_hooks) { - if (async_hook_fields[kAfter]) + if (async_hook_fields[kAfter] > 0) async_hooks.emitAfter(tock._asyncId); - if (async_hook_fields[kDestroy]) + if (async_hook_fields[kDestroy] > 0) async_hooks.emitDestroy(tock._asyncId); } - // TODO(trevnorris): A caught uncaught exception will mess this up. async_uid_fields[kCurrentId] = prev_id; async_uid_fields[kTriggerId] = prev_trigger_id; diff --git a/lib/timers.js b/lib/timers.js index c0aba1a2bc0188..a2bc26c954e6c6 100644 --- a/lib/timers.js +++ b/lib/timers.js @@ -12,7 +12,8 @@ const { async_uid_fields, async_hook_fields } = async_wrap; const { emitInit, emitBefore, emitAfter, emitDestroy, initTriggerId } = require('async_hooks'); // Grab the constants necessary for working with internal arrays. -const { kInit, kDestroy, kAsyncUidCntr } = async_wrap.constants; +const { kInit, kBefore, kAfter, kDestroy, kCurrentId, kTriggerId, + kAsyncUidCntr } = async_wrap.constants; // Timeout values > TIMEOUT_MAX are set to 1. const TIMEOUT_MAX = 2147483647; // 2^31-1 @@ -224,13 +225,24 @@ function listOnTimeout() { domain.enter(); } - emitBefore(timer._asyncId, timer._triggerId); + // Setting these fields manually with the expectation that has_hooks + // will almost always be false. Overhead for doing it twice if it is + // true is negligible anyway. + const prev_id = async_uid_fields[kCurrentId]; + const prev_trigger_id = async_uid_fields[kTriggerId]; + async_uid_fields[kCurrentId] = timer._asyncId; + async_uid_fields[kTriggerId] = timer._triggerId; + if (async_hook_fields[kBefore] > 0) + emitBefore(timer._asyncId, timer._triggerId); tryOnTimeout(timer, list); - emitAfter(timer._asyncId); - if (!timer._repeat && async_hook_fields[kDestroy]) + if (async_hook_fields[kAfter] > 0) + emitAfter(timer._asyncId); + if (!timer._repeat && async_hook_fields[kDestroy] > 0) emitDestroy(timer._asyncId); + async_uid_fields[kCurrentId] = prev_id; + async_uid_fields[kTriggerId] = prev_trigger_id; if (domain) domain.exit(); From 43995472d2db86d8f22aeab127c2e98823db8b9f Mon Sep 17 00:00:00 2001 From: Trevor Norris Date: Tue, 22 Nov 2016 14:27:58 -0700 Subject: [PATCH 19/40] fs: add AsyncReset to TSLWrap --- src/tls_wrap.cc | 1 + 1 file changed, 1 insertion(+) diff --git a/src/tls_wrap.cc b/src/tls_wrap.cc index d089dab66c05a6..06c6db8128f3ba 100644 --- a/src/tls_wrap.cc +++ b/src/tls_wrap.cc @@ -913,6 +913,7 @@ void TLSWrap::Initialize(Local target, t->SetClassName(FIXED_ONE_BYTE_STRING(env->isolate(), "TLSWrap")); env->SetProtoMethod(t, "getAsyncId", AsyncWrap::GetAsyncId); + env->SetProtoMethod(t, "asyncReset", AsyncWrap::AsyncReset); env->SetProtoMethod(t, "receive", Receive); env->SetProtoMethod(t, "start", Start); env->SetProtoMethod(t, "setVerifyMode", SetVerifyMode); From 830428ac77fd18871ab0d771d35163442d1d4ae4 Mon Sep 17 00:00:00 2001 From: Trevor Norris Date: Thu, 15 Dec 2016 15:43:16 -0700 Subject: [PATCH 20/40] async_hooks: allow enable/disable while processing Safely enable()/disable() hooks while hooks are being processed. Otherwise the array length will change and hooks that should/shouldn't have been processed will be. --- lib/async_hooks.js | 133 +++++++++++++++++++++++++++++++++++---------- 1 file changed, 105 insertions(+), 28 deletions(-) diff --git a/lib/async_hooks.js b/lib/async_hooks.js index 7d84cdfff626b4..c7b756e76661c8 100644 --- a/lib/async_hooks.js +++ b/lib/async_hooks.js @@ -18,11 +18,21 @@ const async_hook_fields = async_wrap.async_hook_fields; // within triggerIdScope(). const async_uid_fields = async_wrap.async_uid_fields; // Array of all AsyncHooks that will be iterated whenever an async event fires. -const active_hooks_array = []; +// Using var instead of (preferably const) in order to assign +// tmp_active_hooks_array if a hook is enabled/disabled during hook execution. +var active_hooks_array = []; // Stack of scoped trigger id's for triggerIdScope() const trigger_scope_stack = []; // Array that holds the current and trigger id's for between before/after. const current_trigger_id_stack = []; +// Track if processing hook callbacks. Used to make sure active_hooks_array +// isn't altered in mid execution if another hook is added or removed. +var processing_hooks = false; +// Use to temporarily store and updated active_hooks_array if the user enables +// or disables a hook while hooks are being processed. +var tmp_active_hooks_array = null; +// Keep track of the field counds held in tmp_active_hooks_array. +var tmp_async_hook_fields = null; const async_id_symbol = Symbol('_asyncId'); const trigger_id_symbol = Symbol('_triggerId'); @@ -58,41 +68,71 @@ class AsyncHook { this[destroy_symbol] = fns.destroy; } - // TODO(trevnorris): Both enable and disable need to check if 1) callbacks - // are in the middle of being processed or 2) async destroy's are queue'd - // to be called. In either case a copy of active_hooks_array needs to be - // duplicated then replaced with the altered array once the operation is - // complete. enable() { - if (active_hooks_array.indexOf(this) !== -1) + // Use local reference in case the pointer to the array needs to change. + var hooks_array = active_hooks_array; + var hook_fields = async_hook_fields; + + if (processing_hooks) { + if (tmp_active_hooks_array === null) + setupTmpActiveHooks(); + hooks_array = tmp_active_hooks_array; + hook_fields = tmp_async_hook_fields; + } + + if (hooks_array.indexOf(this) !== -1) return; // createHook() has already enforced that the callbacks are all functions, // so here simply increment the count of whether each callbacks exists or // not. - async_hook_fields[kInit] += +!!this[init_symbol]; - async_hook_fields[kBefore] += +!!this[before_symbol]; - async_hook_fields[kAfter] += +!!this[after_symbol]; - async_hook_fields[kDestroy] += +!!this[destroy_symbol]; - async_hook_fields[kActiveHooks]++; - active_hooks_array.push(this); + hook_fields[kInit] += +!!this[init_symbol]; + hook_fields[kBefore] += +!!this[before_symbol]; + hook_fields[kAfter] += +!!this[after_symbol]; + hook_fields[kDestroy] += +!!this[destroy_symbol]; + hook_fields[kActiveHooks]++; + hooks_array.push(this); return this; } disable() { - const index = active_hooks_array.indexOf(this); + // Use local reference in case the pointer to the array needs to change. + var hooks_array = active_hooks_array; + var hook_fields = async_hook_fields; + + if (processing_hooks) { + if (tmp_active_hooks_array === null) + setupTmpActiveHooks(); + hooks_array = tmp_active_hooks_array; + hook_fields = tmp_async_hook_fields; + } + + const index = hooks_array.indexOf(this); if (index === -1) return; - async_hook_fields[kInit] -= +!!this[init_symbol]; - async_hook_fields[kBefore] -= +!!this[before_symbol]; - async_hook_fields[kAfter] -= +!!this[after_symbol]; - async_hook_fields[kDestroy] -= +!!this[destroy_symbol]; - async_hook_fields[kActiveHooks]--; - active_hooks_array.splice(index, 1); + hook_fields[kInit] -= +!!this[init_symbol]; + hook_fields[kBefore] -= +!!this[before_symbol]; + hook_fields[kAfter] -= +!!this[after_symbol]; + hook_fields[kDestroy] -= +!!this[destroy_symbol]; + hook_fields[kActiveHooks]--; + hooks_array.splice(index, 1); return this; } } +function setupTmpActiveHooks() { + tmp_active_hooks_array = active_hooks_array.slice(); + // Don't want to make the assumption that kInit to kActiveHooks are + // indexes 0 to 5. So do this the long way. + tmp_async_hook_fields = []; + tmp_async_hook_fields[kInit] = async_hook_fields[kInit]; + tmp_async_hook_fields[kBefore] = async_hook_fields[kBefore]; + tmp_async_hook_fields[kAfter] = async_hook_fields[kAfter]; + tmp_async_hook_fields[kDestroy] = async_hook_fields[kDestroy]; + tmp_async_hook_fields[kActiveHooks] = async_hook_fields[kActiveHooks]; +} + + function createHook(fns) { if (fns.init !== undefined && typeof fns.init !== 'function') throw new TypeError('init must be a function'); @@ -253,6 +293,10 @@ function emitInitS(id, type, triggerId, handle) { active_hooks_array[i][init_symbol], id, type, triggerId, handle); } } + + if (tmp_active_hooks_array !== null) { + restoreTmpHooks(); + } } @@ -277,6 +321,10 @@ function emitBeforeS(id, triggerId) { runCallback(active_hooks_array[i][before_symbol], id); } } + + if (tmp_active_hooks_array !== null) { + restoreTmpHooks(); + } } @@ -289,6 +337,10 @@ function emitAfterS(id) { } } + if (tmp_active_hooks_array !== null) { + restoreTmpHooks(); + } + // Remove state after the call has completed. // TODO(trevnorris): Clearing the native counters is easy, but also must // track the length of the array manually so the native side can empty it @@ -318,6 +370,10 @@ function emitDestroyS(id) { runCallback(active_hooks_array[i][destroy_symbol], id); } } + + if (tmp_active_hooks_array !== null) { + restoreTmpHooks(); + } } @@ -344,19 +400,25 @@ function init(id, type, handle) { // Generalized caller for all callbacks that handles error handling. +// If either runInitCallback() or runCallback() throw then force the +// application to shutdown if one of the callbacks throws. This may change in +// the future depending on whether it can be determined if there's a slim +// chance of the application remaining stable after handling one of these +// exceptions. + function runInitCallback(cb, id, type, triggerId, handle) { var fail = true; + processing_hooks = true; try { cb(id, type, triggerId, handle); fail = false; } finally { - // Force the application to shutdown if one of the callbacks throws. This - // may change in the future depending on whether it can be determined if - // there's a slim chance of the application remaining stable after handling - // one of these exceptions. + processing_hooks = false; if (fail) { process._events.uncaughtException = undefined; process.domain = undefined; + // NOTE: If async_hooks allowed for error recovery then restoreTmpHooks() + // would need to be run here if tmp_active_hooks_array !== null. } } } @@ -364,22 +426,37 @@ function runInitCallback(cb, id, type, triggerId, handle) { function runCallback(cb, id) { var fail = true; + processing_hooks = true; try { cb(id); fail = false; } finally { - // Force the application to shutdown if one of the callbacks throws. This - // may change in the future depending on whether it can be determined if - // there's a slim chance of the application remaining stable after handling - // one of these exceptions. + processing_hooks = false; if (fail) { process._events.uncaughtException = undefined; process.domain = undefined; + // NOTE: If async_hooks allowed for error recovery then restoreTmpHooks() + // would need to be run here if tmp_active_hooks_array !== null. } } } +// Then restore the correct hooks array in case any hooks were added/removed +// during hook callback execution. +function restoreTmpHooks() { + active_hooks_array = tmp_active_hooks_array; + async_hook_fields[kInit] = tmp_async_hook_fields[kInit]; + async_hook_fields[kBefore] = tmp_async_hook_fields[kBefore]; + async_hook_fields[kAfter] = tmp_async_hook_fields[kAfter]; + async_hook_fields[kDestroy] = tmp_async_hook_fields[kDestroy]; + async_hook_fields[kActiveHooks] = tmp_async_hook_fields[kActiveHooks]; + + tmp_active_hooks_array = null; + tmp_async_hook_fields = null; +} + + // Placing all exports down here because the exported classes won't export // otherwise. module.exports = { From ad382c5a8258f5e7fdcb05435771f2fb1cd88354 Mon Sep 17 00:00:00 2001 From: Trevor Norris Date: Tue, 20 Dec 2016 15:17:58 -0700 Subject: [PATCH 21/40] async_hooks: run after() in case of exception In the case of an exception being caught by _fatalException(), make sure to always run the after() callbacks to let the user know the callback has completed. Include test to make sure after() is triggered correctly, including having the correct currentId. --- lib/async_hooks.js | 10 ++-- lib/internal/bootstrap_node.js | 13 +++++ src/async-wrap.cc | 5 +- ...test-async-wrap-after-uncaughtexception.js | 50 +++++++++++++++++++ 4 files changed, 71 insertions(+), 7 deletions(-) create mode 100644 test/parallel/test-async-wrap-after-uncaughtexception.js diff --git a/lib/async_hooks.js b/lib/async_hooks.js index c7b756e76661c8..bfd06b906988e3 100644 --- a/lib/async_hooks.js +++ b/lib/async_hooks.js @@ -50,6 +50,11 @@ const { kInit, kBefore, kAfter, kDestroy, kActiveHooks, kAsyncUidCntr, kCurrentId, kTriggerId, kInitTriggerId, kScopedTriggerId } = async_wrap.constants; +// Expose the current_trigger_id_stack via the async_wrap binding for internal +// usage. Specifically so that _fatalException() can empty it if the exception +// is caught. +async_wrap.current_trigger_id_stack = current_trigger_id_stack; + // Setup the callbacks that node::AsyncWrap will call when there are hooks to // process. They use the same functions as the JS embedder API. async_wrap.setupHooks({ init, @@ -294,6 +299,7 @@ function emitInitS(id, type, triggerId, handle) { } } + // Isn't null if hooks were added/removed while the hooks were running. if (tmp_active_hooks_array !== null) { restoreTmpHooks(); } @@ -342,10 +348,6 @@ function emitAfterS(id) { } // Remove state after the call has completed. - // TODO(trevnorris): Clearing the native counters is easy, but also must - // track the length of the array manually so the native side can empty it - // in case there's an error. Or, export the array in such a way that - // fatalException can empty it as well. // TODO(trevnorris): Add a check that kCurrentId === id to make sure the // stack hasn't been corrupted. if (current_trigger_id_stack.length > 0) { diff --git a/lib/internal/bootstrap_node.js b/lib/internal/bootstrap_node.js index 71a6c529041b47..8cab6f45da3479 100644 --- a/lib/internal/bootstrap_node.js +++ b/lib/internal/bootstrap_node.js @@ -281,6 +281,9 @@ } function setupProcessFatal() { + const async_wrap = process.binding('async_wrap'); + const async_uid_fields = async_wrap.async_uid_fields; + const { kCurrentId, kTriggerId } = async_wrap.constants; process._fatalException = function(er) { var caught; @@ -306,6 +309,16 @@ // if we handled an error, then make sure any ticks get processed } else { NativeModule.require('timers').setImmediate(process._tickCallback); + + // Emit the after() hooks now that the exception has been delt with. + NativeModule.require('async_hooks').emitAfter( + async_uid_fields[kCurrentId]); + async_uid_fields[kCurrentId] = 0; + async_uid_fields[kTriggerId] = 0; + async_wrap.current_trigger_id_stack.length = 0; + // No need to worry about running restoreTmpHooks() because that's + // only used while other hooks are running and if a hook throws then + // the application is forced to shut down. } return caught; diff --git a/src/async-wrap.cc b/src/async-wrap.cc index 20c6a07814278b..625e2265cbdb49 100644 --- a/src/async-wrap.cc +++ b/src/async-wrap.cc @@ -435,9 +435,8 @@ Local AsyncWrap::MakeCallback(const Local cb, return Local(); } - // TODO(trevnorris): It will be confusing for developers if there's a caught - // uncaught exception. Which leads to none of their after() callbacks being - // called. + // If the callback failed then the after() hooks will be called at the end + // of _fatalException(). if (async_hooks->fields()[AsyncHooks::kAfter] > 0) { if (uid.IsEmpty()) uid = Number::New(env()->isolate(), get_id()); diff --git a/test/parallel/test-async-wrap-after-uncaughtexception.js b/test/parallel/test-async-wrap-after-uncaughtexception.js new file mode 100644 index 00000000000000..5cb47f0615007f --- /dev/null +++ b/test/parallel/test-async-wrap-after-uncaughtexception.js @@ -0,0 +1,50 @@ +'use strict'; + +const common = require('../common'); +const assert = require('assert'); +const async_hooks = require('async_hooks'); +const thrown_ids = {}; + +process.on('exit', () => { + process.removeAllListeners('uncaughtException'); + assert.strictEqual(Object.keys(thrown_ids).length, 0, 'ids remain'); +}); + +process.on('uncaughtException', common.mustCall((err) => { + assert.strictEqual(err.message, thrown_ids[async_hooks.currentId()]); +}, 5)); + +async_hooks.createHook({ + after(id) { + delete thrown_ids[id]; + }, +}).enable(); + +const simId = setImmediate(() => { + throw new Error('setImmediate'); +})._asyncId; +thrown_ids[simId] = 'setImmediate'; + +const stId = setTimeout(() => { + throw new Error('setTimeout'); +}, 10)._asyncId; +thrown_ids[stId] = 'setTimeout'; + +const sinId = setInterval(function() { + clearInterval(this); + throw new Error('setInterval'); +}, 10)._asyncId; +thrown_ids[sinId] = 'setInterval'; + +const rbId = require('crypto').randomBytes(1, () => { + throw new Error('RANDOMBYTESREQUEST'); +}).getAsyncId(); +thrown_ids[rbId] = 'RANDOMBYTESREQUEST'; + +const tcpId = require('net').createServer(function(c) { + c.end(); + setImmediate(() => this.close()); + throw new Error('TCPWRAP'); +}).listen(common.PORT)._handle.getAsyncId(); +thrown_ids[tcpId] = 'TCPWRAP'; +require('net').connect(common.PORT, () => {}).resume(); From 6e5af4a8553f3b0f5a645e8d1e5c4e0548c35833 Mon Sep 17 00:00:00 2001 From: Trevor Norris Date: Tue, 20 Dec 2016 16:09:01 -0700 Subject: [PATCH 22/40] streams: set initTriggerId before nextTick() Properly set the triggerId for the nextTick() call by using the _asyncId set on the stream. --- lib/_stream_writable.js | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/lib/_stream_writable.js b/lib/_stream_writable.js index af97fc2d8180c6..7d6c9dfc3ca03f 100644 --- a/lib/_stream_writable.js +++ b/lib/_stream_writable.js @@ -7,6 +7,7 @@ module.exports = Writable; Writable.WritableState = WritableState; +const async_hooks = require('async_hooks'); const util = require('util'); const internalUtil = require('internal/util'); const Stream = require('stream'); @@ -366,6 +367,9 @@ function onwrite(stream, er) { } if (sync) { + // TODO(trevnorris): Want to know how often _asyncId isn't a number. + if (typeof stream._asyncId === 'number') + async_hooks.setInitTriggerId(stream._asyncId); process.nextTick(afterWrite, stream, state, finished, cb); } else { afterWrite(stream, state, finished, cb); From 32b52889250b66165b39c1af64a9df0a72cd3476 Mon Sep 17 00:00:00 2001 From: Trevor Norris Date: Tue, 20 Dec 2016 16:10:20 -0700 Subject: [PATCH 23/40] next_tick: set MicrotaskCallback to the void Point the _asyncId and _triggerId of the TickObject used by runMicrotasksCallback() to the void. When the PromiseHook API is implemented in V8 will address and properly propagate the ids. --- lib/internal/process/next_tick.js | 9 +++++++-- 1 file changed, 7 insertions(+), 2 deletions(-) diff --git a/lib/internal/process/next_tick.js b/lib/internal/process/next_tick.js index a83e9ca5a59204..55c3800e9972c9 100644 --- a/lib/internal/process/next_tick.js +++ b/lib/internal/process/next_tick.js @@ -52,8 +52,13 @@ function setupNextTick() { if (microtasksScheduled) return; - nextTickQueue.push( - new TickObject(runMicrotasksCallback, undefined, null, true)); + const tickObject = + new TickObject(runMicrotasksCallback, undefined, null, true); + // For the moment all microtasks come from the void. Until the PromiseHook + // API is implemented in V8. + tickObject._asyncId = 0; + tickObject._triggerId = 0; + nextTickQueue.push(tickObject); tickInfo[kLength]++; microtasksScheduled = true; From bd660a4494c231d2d0f1b2f339b6a630d2cbb1e5 Mon Sep 17 00:00:00 2001 From: Trevor Norris Date: Tue, 20 Dec 2016 16:33:22 -0700 Subject: [PATCH 24/40] test: allow running with async_hook noops Add NODE_TEST_WITH_ASYNC_HOOKS environment variable to allow running tests with init/before/after/destroy noops to make sure that basic mechanics work properly across all tests. --- test/common.js | 10 ++++++++++ 1 file changed, 10 insertions(+) diff --git a/test/common.js b/test/common.js index f111a2939b45de..e9bc3203af9d73 100644 --- a/test/common.js +++ b/test/common.js @@ -14,6 +14,16 @@ const execSync = require('child_process').execSync; const testRoot = process.env.NODE_TEST_DIR ? fs.realpathSync(process.env.NODE_TEST_DIR) : __dirname; +// If env var is set then enable noop async_hook hooks for testing. +if (process.env.NODE_TEST_WITH_ASYNC_HOOKS) { + require('async_hooks').createHook({ + init() { }, + before() { }, + after() { }, + destroy() { }, + }).enable(); +} + exports.fixturesDir = path.join(__dirname, 'fixtures'); exports.tmpDirName = 'tmp'; // PORT should match the definition in test/testpy/__init__.py. From e8c27f8df8880a8a3191135251c95cb5ce6a266f Mon Sep 17 00:00:00 2001 From: Trevor Norris Date: Thu, 22 Dec 2016 15:41:44 -0700 Subject: [PATCH 25/40] async_hooks: move location of restoreTmpHooks() Place restoreTmpHooks() closer to setupTmpActiveHooks() to make it more apparent what each is used for. --- lib/async_hooks.js | 30 +++++++++++++++--------------- 1 file changed, 15 insertions(+), 15 deletions(-) diff --git a/lib/async_hooks.js b/lib/async_hooks.js index bfd06b906988e3..a6fd2e47629a54 100644 --- a/lib/async_hooks.js +++ b/lib/async_hooks.js @@ -138,6 +138,21 @@ function setupTmpActiveHooks() { } +// Then restore the correct hooks array in case any hooks were added/removed +// during hook callback execution. +function restoreTmpHooks() { + active_hooks_array = tmp_active_hooks_array; + async_hook_fields[kInit] = tmp_async_hook_fields[kInit]; + async_hook_fields[kBefore] = tmp_async_hook_fields[kBefore]; + async_hook_fields[kAfter] = tmp_async_hook_fields[kAfter]; + async_hook_fields[kDestroy] = tmp_async_hook_fields[kDestroy]; + async_hook_fields[kActiveHooks] = tmp_async_hook_fields[kActiveHooks]; + + tmp_active_hooks_array = null; + tmp_async_hook_fields = null; +} + + function createHook(fns) { if (fns.init !== undefined && typeof fns.init !== 'function') throw new TypeError('init must be a function'); @@ -444,21 +459,6 @@ function runCallback(cb, id) { } -// Then restore the correct hooks array in case any hooks were added/removed -// during hook callback execution. -function restoreTmpHooks() { - active_hooks_array = tmp_active_hooks_array; - async_hook_fields[kInit] = tmp_async_hook_fields[kInit]; - async_hook_fields[kBefore] = tmp_async_hook_fields[kBefore]; - async_hook_fields[kAfter] = tmp_async_hook_fields[kAfter]; - async_hook_fields[kDestroy] = tmp_async_hook_fields[kDestroy]; - async_hook_fields[kActiveHooks] = tmp_async_hook_fields[kActiveHooks]; - - tmp_active_hooks_array = null; - tmp_async_hook_fields = null; -} - - // Placing all exports down here because the exported classes won't export // otherwise. module.exports = { From 0ab9547d64331a7453798ad81e181434ab179c02 Mon Sep 17 00:00:00 2001 From: Trevor Norris Date: Tue, 27 Dec 2016 14:37:24 -0700 Subject: [PATCH 26/40] async_wrap: use v8::Eternal for provider strings Switch to using a v8::Eternal array to retrieve the provider strings passed as the "type" to the init() callback. --- src/async-wrap.cc | 18 +----------------- src/env-inl.h | 21 ++++++++++++++++++++- src/env.h | 5 +++++ 3 files changed, 26 insertions(+), 18 deletions(-) diff --git a/src/async-wrap.cc b/src/async-wrap.cc index 625e2265cbdb49..a698880905ce21 100644 --- a/src/async-wrap.cc +++ b/src/async-wrap.cc @@ -20,11 +20,9 @@ using v8::Integer; using v8::Isolate; using v8::Local; using v8::MaybeLocal; -using v8::NewStringType; using v8::Number; using v8::Object; using v8::RetainedObjectInfo; -using v8::String; using v8::TryCatch; using v8::Uint32Array; using v8::Value; @@ -303,15 +301,6 @@ void LoadAsyncWrapperInfo(Environment* env) { } -// TODO(trevnorris): Look into the overhead of using this. Can't use it anway -// if it switches to using persistent strings instead. -static const char* GetProviderName(AsyncWrap::ProviderType provider) { - CHECK_GT(provider, 0); - CHECK_LE(provider, AsyncWrap::PROVIDERS_LENGTH); - return provider_names[provider]; -} - - AsyncWrap::AsyncWrap(Environment* env, Local object, ProviderType provider) @@ -357,12 +346,7 @@ void AsyncWrap::Reset() { Local argv[] = { Number::New(env()->isolate(), get_id()), - // TODO(trevnorris): Very slow and bad. Use another way to more quickly get - // the correct provider string. Something like storing them on - // PER_ISOLATE_STRING_PROPERTIES in env.h - String::NewFromUtf8(env()->isolate(), - GetProviderName(provider_type()), - NewStringType::kNormal).ToLocalChecked(), + env()->async_hooks()->provider_string(env()->isolate(), provider_type()), object(), Number::New(env()->isolate(), get_trigger_id()), }; diff --git a/src/env-inl.h b/src/env-inl.h index c912d7cfd20001..8ec8215e55d8dd 100644 --- a/src/env-inl.h +++ b/src/env-inl.h @@ -59,9 +59,21 @@ inline uint32_t* IsolateData::zero_fill_field() const { return zero_fill_field_; } -inline Environment::AsyncHooks::AsyncHooks() : fields_(), uid_fields_() { +inline Environment::AsyncHooks::AsyncHooks(v8::Isolate* isolate) + : fields_(), + uid_fields_() { // kAsyncUidCntr should start at 1 because that'll be the id for bootstrap. uid_fields_[AsyncHooks::kAsyncUidCntr] = 1; +#define V(Provider) \ + providers_[AsyncWrap::PROVIDER_ ## Provider].Set( \ + isolate, \ + v8::String::NewFromOneByte( \ + isolate, \ + reinterpret_cast(#Provider), \ + v8::NewStringType::kInternalized, \ + sizeof(#Provider) - 1).ToLocalChecked()); + NODE_ASYNC_PROVIDER_TYPES(V) +#undef V } inline uint32_t* Environment::AsyncHooks::fields() { @@ -76,6 +88,12 @@ inline double* Environment::AsyncHooks::uid_fields() { return uid_fields_; } +inline v8::Local Environment::AsyncHooks::provider_string( + v8::Isolate* isolate, + int idx) { + return providers_[idx].Get(isolate); +} + inline int Environment::AsyncHooks::uid_fields_count() const { return kUidFieldsCount; } @@ -167,6 +185,7 @@ inline Environment::Environment(IsolateData* isolate_data, v8::Local context) : isolate_(context->GetIsolate()), isolate_data_(isolate_data), + async_hooks_(context->GetIsolate()), timer_base_(uv_now(isolate_data->event_loop())), using_domains_(false), printed_error_(false), diff --git a/src/env.h b/src/env.h index 72d59b78e29661..737fd32cb62a70 100644 --- a/src/env.h +++ b/src/env.h @@ -337,6 +337,7 @@ class Environment { inline int fields_count() const; inline double* uid_fields(); inline int uid_fields_count() const; + inline v8::Local provider_string(v8::Isolate* isolate, int idx); class InitScope { public: @@ -392,6 +393,10 @@ class Environment { private: friend class Environment; // So we can call the constructor. inline AsyncHooks(); + inline explicit AsyncHooks(v8::Isolate* isolate); + + // Keep a list of all Persistent strings used for Provider types. + v8::Eternal providers_[AsyncWrap::PROVIDERS_LENGTH]; uint32_t fields_[kFieldsCount]; // Gives us 2^53-1 unique ids. Good enough for now and makes the operation From 352c89ad3baa6b15ac82e4c843e433ddea0ff51c Mon Sep 17 00:00:00 2001 From: Trevor Norris Date: Thu, 29 Dec 2016 16:56:40 -0700 Subject: [PATCH 27/40] fs: use preallocated Float64Array Instead of always passing in a Float64Array to getIdsFromFd() preallocate one during initialization and write to those fields directly. --- lib/fs.js | 10 +++++----- src/env-inl.h | 5 +++++ src/env.h | 2 ++ src/node_file.cc | 30 ++++++++++++++++++------------ 4 files changed, 30 insertions(+), 17 deletions(-) diff --git a/lib/fs.js b/lib/fs.js index 763b10b5036e7c..ae8f71592b4db4 100644 --- a/lib/fs.js +++ b/lib/fs.js @@ -40,7 +40,7 @@ const DEBUG = process.env.NODE_DEBUG && /fs/.test(process.env.NODE_DEBUG); const errnoException = util._errnoException; // Used to pull the id and triggerId from getIdsFromFd(). -const f64array = new Float64Array(2); +const async_ids = binding.fd_async_ids; function getOptions(options, defaultOptions) { if (options === null || options === undefined || @@ -140,8 +140,8 @@ function setActualInitTriggerId(self) { setInitTriggerId(self._asyncId); return; } - binding.getIdsFromFd(self.fd, f64array); - setInitTriggerId(f64array[0] > 0 ? f64array[0] : currentId()); + binding.getIdsFromFd(self.fd); + setInitTriggerId(async_ids[0] > 0 ? async_ids[0] : currentId()); } // Static method to set the stats properties on a Stats object. @@ -281,8 +281,8 @@ fs.readFile = function(path, options, callback) { if (context.isUserFd) { // If the fd was opened synchronously then there will be no asyncId. - binding.getIdsFromFd(path, f64array); - context._asyncId = f64array[0] > 0 ? f64array[0] : currentId(); + binding.getIdsFromFd(path); + context._asyncId = async_ids[0] > 0 ? async_ids[0] : currentId(); setInitTriggerId(context._asyncId); process.nextTick(function() { req.oncomplete(null, path); diff --git a/src/env-inl.h b/src/env-inl.h index 8ec8215e55d8dd..88b73569537e0d 100644 --- a/src/env-inl.h +++ b/src/env-inl.h @@ -191,6 +191,7 @@ inline Environment::Environment(IsolateData* isolate_data, printed_error_(false), trace_sync_io_(false), makecallback_cntr_(0), + fd_async_ids_inst_(), debugger_agent_(this), #if HAVE_INSPECTOR inspector_agent_(this), @@ -389,6 +390,10 @@ inline void Environment::insert_fd_async_ids(int fd, fd_async_id_map_[fd] = { async_id, trigger_id }; } +inline node_fd_async_ids* Environment::get_fd_async_ids_inst() { + return &fd_async_ids_inst_; +} + inline double* Environment::heap_statistics_buffer() const { CHECK_NE(heap_statistics_buffer_, nullptr); return heap_statistics_buffer_; diff --git a/src/env.h b/src/env.h index 737fd32cb62a70..d12f9ff1d3ec50 100644 --- a/src/env.h +++ b/src/env.h @@ -555,6 +555,7 @@ class Environment { inline void erase_fd_async_id(int fd); inline node_fd_async_ids get_fd_async_id(int fd); inline void insert_fd_async_ids(int fd, double async_id, double trigger_id); + inline node_fd_async_ids* get_fd_async_ids_inst(); // List of id's that have been destroyed and need the destroy() cb called. inline std::vector* destroy_ids_list(); @@ -658,6 +659,7 @@ class Environment { size_t makecallback_cntr_; double async_wrap_id_; std::vector destroy_ids_list_; + node_fd_async_ids fd_async_ids_inst_; std::unordered_map fd_async_id_map_; debugger::Agent debugger_agent_; #if HAVE_INSPECTOR diff --git a/src/node_file.cc b/src/node_file.cc index 827fd44ab03fdb..87204d57203208 100644 --- a/src/node_file.cc +++ b/src/node_file.cc @@ -27,6 +27,7 @@ namespace node { using v8::Array; +using v8::ArrayBuffer; using v8::Context; using v8::EscapableHandleScope; using v8::Float64Array; @@ -1443,25 +1444,18 @@ static void Mkdtemp(const FunctionCallbackInfo& args) { } -// To use this pass in a Float64Array(2) as the second argument. The asyncId -// and triggerId will be written to both indexes. This is much faster than +// The asyncId and triggerId will be written to both indexes of the +// Float64Array stored on Environment::AsyncHooks. This is much faster than // creating and returning an Array. static void GetIdsFromFd(const FunctionCallbackInfo& args) { Environment* env = Environment::GetCurrent(args); if (!args[0]->IsInt32()) return TYPE_ERROR("fd must be an int"); - if (!args[1]->IsFloat64Array()) - return TYPE_ERROR("arr must be a Float64Array"); - Local arr = args[1].As(); - if (arr->Length() < 2) - return env->ThrowRangeError("typed array is not large enough"); - - double* ptr = reinterpret_cast(arr->Buffer()->GetContents().Data()); + auto ptr = env->get_fd_async_ids_inst(); node_fd_async_ids slot = env->get_fd_async_id(args[0]->Int32Value()); - ptr[0] = slot.async_id; - ptr[1] = slot.trigger_id; - args.GetReturnValue().Set(arr); + ptr->async_id = slot.async_id; + ptr->trigger_id = slot.trigger_id; } @@ -1473,6 +1467,7 @@ void FSInitialize(const FunctionCallbackInfo& args) { env->set_fs_stats_constructor_function(stats_constructor); } + void InitFs(Local target, Local unused, Local context, @@ -1523,6 +1518,17 @@ void InitFs(Local target, env->SetMethod(target, "getIdsFromFd", GetIdsFromFd); + // Set Float64Array used by GetIdsFromFd() to quickly retrieve an asyncId + // from the unordered_map. + node_fd_async_ids* fd_ids_inst = env->get_fd_async_ids_inst(); + Local ab = + ArrayBuffer::New(env->isolate(), + reinterpret_cast(fd_ids_inst), + 2); + Local name = FIXED_ONE_BYTE_STRING(env->isolate(), "fd_async_ids"); + Local value = Float64Array::New(ab, 0, 2); + target->Set(env->context(), name, value).ToChecked(); + StatWatcher::Initialize(env, target); // Create FunctionTemplate for FSReqWrap From e9ae576d584e77c42ebbdab6dac392f3fd28a021 Mon Sep 17 00:00:00 2001 From: Trevor Norris Date: Tue, 3 Jan 2017 16:21:44 -0700 Subject: [PATCH 28/40] PARTIAL remove all applicable TODOs --- lib/_stream_wrap.js | 2 -- lib/_stream_writable.js | 4 ---- lib/async_hooks.js | 11 +++++++++++ lib/internal/bootstrap_node.js | 3 +++ lib/internal/process/next_tick.js | 6 ------ lib/timers.js | 4 +++- src/async-wrap.cc | 11 ++++++++--- src/node.cc | 4 ++++ src/node_file.cc | 3 ++- src/stream_base.cc | 7 ++++--- 10 files changed, 35 insertions(+), 20 deletions(-) diff --git a/lib/_stream_wrap.js b/lib/_stream_wrap.js index 1367818ad7ba93..fbc32965980e96 100644 --- a/lib/_stream_wrap.js +++ b/lib/_stream_wrap.js @@ -9,8 +9,6 @@ const uv = process.binding('uv'); const debug = util.debuglog('stream_wrap'); function StreamWrap(stream) { - // TODO(trevnorris): Track down everything that calls StreamWrap and make - // sure the trigger id is correctly set. const handle = new JSStream(); this.stream = stream; diff --git a/lib/_stream_writable.js b/lib/_stream_writable.js index 7d6c9dfc3ca03f..af97fc2d8180c6 100644 --- a/lib/_stream_writable.js +++ b/lib/_stream_writable.js @@ -7,7 +7,6 @@ module.exports = Writable; Writable.WritableState = WritableState; -const async_hooks = require('async_hooks'); const util = require('util'); const internalUtil = require('internal/util'); const Stream = require('stream'); @@ -367,9 +366,6 @@ function onwrite(stream, er) { } if (sync) { - // TODO(trevnorris): Want to know how often _asyncId isn't a number. - if (typeof stream._asyncId === 'number') - async_hooks.setInitTriggerId(stream._asyncId); process.nextTick(afterWrite, stream, state, finished, cb); } else { afterWrite(stream, state, finished, cb); diff --git a/lib/async_hooks.js b/lib/async_hooks.js index a6fd2e47629a54..886996b6cf107b 100644 --- a/lib/async_hooks.js +++ b/lib/async_hooks.js @@ -277,6 +277,8 @@ function setInitTriggerId(id) { } +// TODO(trevnorris): Change the API to be: +// emitInit(number, string[, number][, resource]) // Usage: // emitInit(number, string[, number[, handle]]) // @@ -324,6 +326,15 @@ function emitInitS(id, type, triggerId, handle) { // Usage: emitBeforeS(id[, triggerId]). If triggerId is omitted then id will be // used instead. function emitBeforeS(id, triggerId) { + // Validate the ids. + // TODO(trevnorris): Verify this check doesn't have adverse performance + // implications on the global API. + if (id < 0 || triggerId < 0) { + process._events.uncaughtException = undefined; + process.domain = undefined; + throw new Error(`id (${id}) or triggerId (${triggerId}) is < 0`); + } + // First setup the currentId and triggerId for the coming callback. const currentCurrentId = async_uid_fields[kCurrentId]; const currentTriggerId = async_uid_fields[kTriggerId]; diff --git a/lib/internal/bootstrap_node.js b/lib/internal/bootstrap_node.js index 8cab6f45da3479..7ed5c77a74a617 100644 --- a/lib/internal/bootstrap_node.js +++ b/lib/internal/bootstrap_node.js @@ -313,6 +313,9 @@ // Emit the after() hooks now that the exception has been delt with. NativeModule.require('async_hooks').emitAfter( async_uid_fields[kCurrentId]); + // TODO(trevnorris): This is too blunt a weapon. If there was a stack + // of ids that needed to be processed then after() should be called + // for all of them. async_uid_fields[kCurrentId] = 0; async_uid_fields[kTriggerId] = 0; async_wrap.current_trigger_id_stack.length = 0; diff --git a/lib/internal/process/next_tick.js b/lib/internal/process/next_tick.js index 55c3800e9972c9..8f26819791f3f8 100644 --- a/lib/internal/process/next_tick.js +++ b/lib/internal/process/next_tick.js @@ -101,9 +101,6 @@ function setupNextTick() { do { while (tickInfo[kIndex] < tickInfo[kLength]) { const has_hooks = async_hook_fields[kActiveHooks] > 0; - // TODO(trevnorris): These should always be zero if at the bottom of - // the stack. Add check somewhere cheaper than here to make sure that's - // the case. const prev_id = async_uid_fields[kCurrentId]; const prev_trigger_id = async_uid_fields[kTriggerId]; @@ -148,9 +145,6 @@ function setupNextTick() { do { while (tickInfo[kIndex] < tickInfo[kLength]) { const has_hooks = async_hook_fields[kActiveHooks] > 0; - // TODO(trevnorris): These should always be zero if at the bottom of - // the stack. Add check somewhere cheaper than here to make sure that's - // the case. const prev_id = async_uid_fields[kCurrentId]; const prev_trigger_id = async_uid_fields[kTriggerId]; diff --git a/lib/timers.js b/lib/timers.js index a2bc26c954e6c6..b67feca81ec36f 100644 --- a/lib/timers.js +++ b/lib/timers.js @@ -143,8 +143,10 @@ function insert(item, unrefed) { if (!item._asyncId) { item._asyncId = ++async_uid_fields[kAsyncUidCntr]; item._triggerId = initTriggerId(); + // NOTE: While all "item" passed to insert() are emitted with type + // 'Timeout', the actual object can be created outside of the Timeout + // constructor. Even so, they'll all be emitted as 'Timeout'. if (async_hook_fields[kInit] > 0) - // TODO(trevnorris): Is the "type" of 'Timeout' appropriate here? emitInit(item._asyncId, 'Timeout', item._triggerId, item); } diff --git a/src/async-wrap.cc b/src/async-wrap.cc index a698880905ce21..4806a803143049 100644 --- a/src/async-wrap.cc +++ b/src/async-wrap.cc @@ -435,6 +435,9 @@ Local AsyncWrap::MakeCallback(const Local cb, } } + // The execution scope of the id and trigger_id only go this far. + exec_scope.Dispose(); + if (has_domain) { Local exit_v = domain->Get(env()->exit_string()); if (exit_v->IsFunction()) { @@ -445,9 +448,6 @@ Local AsyncWrap::MakeCallback(const Local cb, } } - // The execution scope of the id and trigger_id only go this far. - exec_scope.Dispose(); - if (callback_scope.in_makecallback()) { return ret_v; } @@ -470,6 +470,11 @@ Local AsyncWrap::MakeCallback(const Local cb, process, 0, nullptr); + + // Make sure the stack unwound properly. + CHECK_EQ(env()->current_async_id(), 0); + CHECK_EQ(env()->trigger_id(), 0); + return rcheck.IsEmpty() ? Local() : ret_v; } diff --git a/src/node.cc b/src/node.cc index 25008344059213..31d52c601745cc 100644 --- a/src/node.cc +++ b/src/node.cc @@ -1240,6 +1240,10 @@ Local MakeCallback(Environment* env, Local process = env->process_object(); + // Make sure the stack unwound properly. + CHECK_EQ(env->current_async_id(), 0); + CHECK_EQ(env->trigger_id(), 0); + if (tick_info->length() == 0) { tick_info->set_index(0); return ret; diff --git a/src/node_file.cc b/src/node_file.cc index 87204d57203208..0f8fc7c4b516df 100644 --- a/src/node_file.cc +++ b/src/node_file.cc @@ -417,7 +417,8 @@ static void Close(const FunctionCallbackInfo& args) { return TYPE_ERROR("fd must be a file descriptor"); int fd = args[0]->Int32Value(); - // TODO(trevnorris): Won't these values be needed for the destroy callbacks? + // Can remove the async_id here because it's only used from JS when setting + // the init trigger id. env->erase_fd_async_id(fd); if (args[1]->IsObject()) { diff --git a/src/stream_base.cc b/src/stream_base.cc index 76d2be1b9a21c8..88cd6ebf997cfc 100644 --- a/src/stream_base.cc +++ b/src/stream_base.cc @@ -133,9 +133,10 @@ int StreamBase::Writev(const FunctionCallbackInfo& args) { return UV_ENOBUFS; AsyncWrap* wrap = GetAsyncWrap(); - // TODO(trevnorris): Would like to catalog every case when wrap == nullptr. - if (wrap != nullptr) - env->set_init_trigger_id(wrap->get_id()); + // NOTE: All tests show that GetAsyncWrap() never returns nullptr here. If it + // can then replace the CHECK_NE() with if (wrap != nullptr). + CHECK_NE(wrap, nullptr); + env->set_init_trigger_id(wrap->get_id()); WriteWrap* req_wrap = WriteWrap::New(env, req_wrap_obj, this, From 015e6653cf8489d71922deedc448613a02779692 Mon Sep 17 00:00:00 2001 From: Trevor Norris Date: Wed, 11 Jan 2017 15:26:59 -0700 Subject: [PATCH 29/40] async_hooks: verify stack integrity When emitAfterS() is called, make sure the id passed in is the same id that's on the stack. --- lib/async_hooks.js | 6 ++++++ 1 file changed, 6 insertions(+) diff --git a/lib/async_hooks.js b/lib/async_hooks.js index 886996b6cf107b..65a68a0cd0d1c1 100644 --- a/lib/async_hooks.js +++ b/lib/async_hooks.js @@ -373,6 +373,12 @@ function emitAfterS(id) { restoreTmpHooks(); } + if (id !== async_uid_fields[kCurrentId]) { + throw new Error('async hook stack has become corrupted: ' + + id + ' ' + + async_uid_fields[kCurrentId]); + } + // Remove state after the call has completed. // TODO(trevnorris): Add a check that kCurrentId === id to make sure the // stack hasn't been corrupted. From e2af82f6078368b68d29d4beb462dc6061d742a5 Mon Sep 17 00:00:00 2001 From: Trevor Norris Date: Tue, 17 Jan 2017 14:14:53 -0700 Subject: [PATCH 30/40] async_hooks: place destroy ids on native list destroy() hooks from JS should also be called async, but instead of trying to do this in a setImmediate, just use the same list that is used for native ids. --- lib/async_hooks.js | 8 +++++++- src/async-wrap.cc | 20 ++++++++++++++++++++ src/async-wrap.h | 3 +++ 3 files changed, 30 insertions(+), 1 deletion(-) diff --git a/lib/async_hooks.js b/lib/async_hooks.js index 65a68a0cd0d1c1..a7065709b6baeb 100644 --- a/lib/async_hooks.js +++ b/lib/async_hooks.js @@ -60,7 +60,7 @@ async_wrap.current_trigger_id_stack = current_trigger_id_stack; async_wrap.setupHooks({ init, before: emitBeforeS, after: emitAfterS, - destroy: emitDestroyS }); + destroy: emitDestroyFromNative }); // Public API // @@ -399,6 +399,12 @@ function emitDestroyS(id) { if (async_hook_fields[kDestroy] === 0) { return; } + + async_wrap.addIdToDestroyList(id); +} + + +function emitDestroyFromNative(id) { for (var i = 0; i < active_hooks_array.length; i++) { if (typeof active_hooks_array[i][destroy_symbol] === 'function') { runCallback(active_hooks_array[i][destroy_symbol], id); diff --git a/src/async-wrap.cc b/src/async-wrap.cc index 4806a803143049..a924d9152a1e15 100644 --- a/src/async-wrap.cc +++ b/src/async-wrap.cc @@ -158,6 +158,7 @@ void AsyncWrap::Initialize(Local target, HandleScope scope(isolate); env->SetMethod(target, "setupHooks", SetupHooks); + env->SetMethod(target, "addIdToDestroyList", AddIdToDestroyList); // Attach the uint32_t[] where each slot contains the count of the number of // callbacks waiting to be called on a particular event. It can then be @@ -291,6 +292,24 @@ void AsyncWrap::AsyncReset(const FunctionCallbackInfo& args) { } +void AsyncWrap::AddIdToDestroyList(const FunctionCallbackInfo& args) { + Environment* env = Environment::GetCurrent(args); + + // This technically shouldn't be needed, since kDestroy should have been + // checked before calling this function. So make this a CHECK instead? + if (env->async_hooks()->fields()[AsyncHooks::kDestroy] == 0) { + return; + } + + CHECK(args[0]->IsNumber()); + + if (env->destroy_ids_list()->empty()) + uv_idle_start(env->destroy_ids_idle_handle(), DestroyIdsCb); + + env->destroy_ids_list()->push_back(args[0]->NumberValue()); +} + + void LoadAsyncWrapperInfo(Environment* env) { HeapProfiler* heap_profiler = env->isolate()->GetHeapProfiler(); #define V(PROVIDER) \ @@ -328,6 +347,7 @@ AsyncWrap::~AsyncWrap() { env()->destroy_ids_list()->push_back(get_id()); } + // Generalized call for both the constructor and for handles that are pooled // and reused over their lifetime. This way a new uid can be assigned when // the resource is pulled out of the pool and put back into use. diff --git a/src/async-wrap.h b/src/async-wrap.h index 04ad876727acb3..d17517719d0243 100644 --- a/src/async-wrap.h +++ b/src/async-wrap.h @@ -69,6 +69,9 @@ class AsyncWrap : public BaseObject { static void AsyncReset(const v8::FunctionCallbackInfo& args); + static void AddIdToDestroyList( + const v8::FunctionCallbackInfo& args); + inline ProviderType provider_type() const; inline double get_id() const; From 1b7438f023ac6d4ff14b031c6a005d6350c92563 Mon Sep 17 00:00:00 2001 From: Trevor Norris Date: Tue, 17 Jan 2017 14:19:02 -0700 Subject: [PATCH 31/40] test: add more async hook tests Add NODE_TEST_HANDLE_ACCESS, which tests if a resource aborts if inspected at the end of the process (specifically if the resource has been cleaned up on the C++ side), and NODE_CHECK_ASYNC_DESTROY, which checks if destroy has been called on the same id twice. --- test/common.js | 37 +++++++++++++++++++++++++++++++++++-- 1 file changed, 35 insertions(+), 2 deletions(-) diff --git a/test/common.js b/test/common.js index e9bc3203af9d73..df2c4da06213a2 100644 --- a/test/common.js +++ b/test/common.js @@ -16,11 +16,44 @@ const testRoot = process.env.NODE_TEST_DIR ? // If env var is set then enable noop async_hook hooks for testing. if (process.env.NODE_TEST_WITH_ASYNC_HOOKS) { + const destroydIdsList = {}; + const destroyListList = {}; + const initHandles = {}; + const async_wrap = process.binding('async_wrap'); + + if (process.env.NODE_TEST_HANDLE_ACCESS) { + process.on('exit', () => { + // itterate through handles to make sure nothing crashes + for (const k in initHandles) + util.inspect(initHandles[k]); + }); + } + + const _addIdToDestroyList = async_wrap.addIdToDestroyList; + async_wrap.addIdToDestroyList = function addIdToDestroyList(id) { + if (!process.env.NODE_CHECK_ASYNC_DESTROY) + return _addIdToDestroyList.call(this, id); + if (destroyListList[id] !== undefined) { + process._rawDebug(destroyListList[id]); + process._rawDebug(); + throw new Error(`same id added twice (${id})`); + } + destroyListList[id] = new Error().stack; + }; + require('async_hooks').createHook({ - init() { }, + init(id, ty, tr, h) { initHandles[id] = h; }, before() { }, after() { }, - destroy() { }, + destroy(id) { + if (!process.env.NODE_CHECK_ASYNC_DESTROY) return; + if (destroydIdsList[id] !== undefined) { + process._rawDebug(destroydIdsList[id]); + process._rawDebug(); + throw new Error(`destroy called for same id(${id})`); + } + destroydIdsList[id] = new Error().stack; + }, }).enable(); } From ca8d6dbdb2bdbf70ad1cf31aa60ad0437fb863d1 Mon Sep 17 00:00:00 2001 From: Trevor Norris Date: Tue, 17 Jan 2017 16:02:57 -0700 Subject: [PATCH 32/40] timers: don't reset _asyncId/_triggerId This was originally done as a safety measure, but fails in practice if a clear*() is run on the same handle that's currently executing its callback. --- lib/timers.js | 2 -- 1 file changed, 2 deletions(-) diff --git a/lib/timers.js b/lib/timers.js index b67feca81ec36f..ebffd2fb46f406 100644 --- a/lib/timers.js +++ b/lib/timers.js @@ -649,7 +649,6 @@ function processImmediate() { emitAfter(immediate._asyncId); if (async_hook_fields[kDestroy]) emitDestroy(immediate._asyncId); - immediate._asyncId = immediate._triggerId = -1; if (domain) domain.exit(); @@ -792,6 +791,5 @@ exports.clearImmediate = function(immediate) { if (immediate._asyncId > 0) { if (async_hook_fields[kDestroy]) emitDestroy(immediate._asyncId); - immediate._asyncId = immediate._triggerId = -1; } }; From ac2e1374d20b925243d6d47a0ada3f4f288c78bf Mon Sep 17 00:00:00 2001 From: Trevor Norris Date: Tue, 17 Jan 2017 16:42:25 -0700 Subject: [PATCH 33/40] async_hooks: return early if emitting the void In some cases, like triggering the microtask queue, there is no id (though that should change in the future). So instead trigger that execution is happening in the void, and return early if destroy is emitted on the void. --- lib/async_hooks.js | 6 +++--- src/async-wrap.cc | 4 +++- 2 files changed, 6 insertions(+), 4 deletions(-) diff --git a/lib/async_hooks.js b/lib/async_hooks.js index a7065709b6baeb..7a4195a076d1ef 100644 --- a/lib/async_hooks.js +++ b/lib/async_hooks.js @@ -396,10 +396,10 @@ function emitAfterS(id) { // of id's that may need to run. So instead of forcing native to call into JS // for every handle, instead have this call back into C++ for the next id. function emitDestroyS(id) { - if (async_hook_fields[kDestroy] === 0) { + // Return early if there are no destroy callbacks, or on attempt to emit + // destroy on the void. + if (async_hook_fields[kDestroy] === 0 || id === 0) return; - } - async_wrap.addIdToDestroyList(id); } diff --git a/src/async-wrap.cc b/src/async-wrap.cc index a924d9152a1e15..f79821aa0934b0 100644 --- a/src/async-wrap.cc +++ b/src/async-wrap.cc @@ -302,11 +302,13 @@ void AsyncWrap::AddIdToDestroyList(const FunctionCallbackInfo& args) { } CHECK(args[0]->IsNumber()); + double async_id = args[0]->NumberValue(); + CHECK_GT(async_id, 0); if (env->destroy_ids_list()->empty()) uv_idle_start(env->destroy_ids_idle_handle(), DestroyIdsCb); - env->destroy_ids_list()->push_back(args[0]->NumberValue()); + env->destroy_ids_list()->push_back(async_id); } From da138cf6ff7bdb06f73761a00422f99f2cb185cd Mon Sep 17 00:00:00 2001 From: Trevor Norris Date: Tue, 17 Jan 2017 17:26:21 -0700 Subject: [PATCH 34/40] async_hooks: really always exit on exception async_hooks does not allow recovery from errors because it's currently unknown what will happen to the state of the application if that happens. So instead, force the application to print an error message then either exit or abort. Depending on whether the user passed --abort-on-uncaught-exception. exit() is used instead of reallyExit() to allow any 'exit' events to fire. --- lib/async_hooks.js | 52 ++++++++++++++++++++-------------------------- 1 file changed, 22 insertions(+), 30 deletions(-) diff --git a/lib/async_hooks.js b/lib/async_hooks.js index 7a4195a076d1ef..40a8f25743ab3c 100644 --- a/lib/async_hooks.js +++ b/lib/async_hooks.js @@ -62,6 +62,20 @@ async_wrap.setupHooks({ init, after: emitAfterS, destroy: emitDestroyFromNative }); +// Used to fatally abort the process if a callback throws. +function fatalError(e) { + if (e instanceof Error) { + process._rawDebug(e.stack); + } else { + const o = { message: e }; + Error.captureStackTrace(o, fatalError); + process._rawDebug(o.stack); + } + if (process.execArgv.indexOf('--abort-on-uncaught-exception') >= 0) + process.abort(); + process.exit(1); +} + // Public API // @@ -327,13 +341,8 @@ function emitInitS(id, type, triggerId, handle) { // used instead. function emitBeforeS(id, triggerId) { // Validate the ids. - // TODO(trevnorris): Verify this check doesn't have adverse performance - // implications on the global API. - if (id < 0 || triggerId < 0) { - process._events.uncaughtException = undefined; - process.domain = undefined; - throw new Error(`id (${id}) or triggerId (${triggerId}) is < 0`); - } + if (id < 0 || triggerId < 0) + fatalError(`before(): id or triggerId < 0 (${id}, ${triggerId})`); // First setup the currentId and triggerId for the coming callback. const currentCurrentId = async_uid_fields[kCurrentId]; @@ -374,9 +383,8 @@ function emitAfterS(id) { } if (id !== async_uid_fields[kCurrentId]) { - throw new Error('async hook stack has become corrupted: ' + - id + ' ' + - async_uid_fields[kCurrentId]); + fatalError(`async hook stack has become corrupted: ${id}, ` + + async_uid_fields[kCurrentId]); } // Remove state after the call has completed. @@ -447,37 +455,21 @@ function init(id, type, handle) { // exceptions. function runInitCallback(cb, id, type, triggerId, handle) { - var fail = true; processing_hooks = true; try { cb(id, type, triggerId, handle); - fail = false; - } finally { - processing_hooks = false; - if (fail) { - process._events.uncaughtException = undefined; - process.domain = undefined; - // NOTE: If async_hooks allowed for error recovery then restoreTmpHooks() - // would need to be run here if tmp_active_hooks_array !== null. - } + } catch (e) { + fatalError(e); } } function runCallback(cb, id) { - var fail = true; processing_hooks = true; try { cb(id); - fail = false; - } finally { - processing_hooks = false; - if (fail) { - process._events.uncaughtException = undefined; - process.domain = undefined; - // NOTE: If async_hooks allowed for error recovery then restoreTmpHooks() - // would need to be run here if tmp_active_hooks_array !== null. - } + } catch (e) { + fatalError(e); } } From 25dd8af16d23011d64824d00163fe33adb782d01 Mon Sep 17 00:00:00 2001 From: Trevor Norris Date: Wed, 18 Jan 2017 15:07:14 -0700 Subject: [PATCH 35/40] async_wrap: don't allow exports to be overwritten ForceSet() everything with v8::ReadOnly | v8::DontDelete to make sure none of the object properties can be overridden. --- src/async-wrap.cc | 43 +++++++++++++++++++++++++++---------------- 1 file changed, 27 insertions(+), 16 deletions(-) diff --git a/src/async-wrap.cc b/src/async-wrap.cc index f79821aa0934b0..3d5912daf5daeb 100644 --- a/src/async-wrap.cc +++ b/src/async-wrap.cc @@ -160,21 +160,23 @@ void AsyncWrap::Initialize(Local target, env->SetMethod(target, "setupHooks", SetupHooks); env->SetMethod(target, "addIdToDestroyList", AddIdToDestroyList); + v8::PropertyAttribute ReadOnlyDontDelete = + static_cast(v8::ReadOnly | v8::DontDelete); + // Attach the uint32_t[] where each slot contains the count of the number of // callbacks waiting to be called on a particular event. It can then be // incremented/decremented from JS quickly to communicate to C++ if there are // any callbacks waiting to be called. uint32_t* fields_ptr = env->async_hooks()->fields(); int fields_count = env->async_hooks()->fields_count(); - Local fields_ab = ArrayBuffer::New( - isolate, - fields_ptr, - fields_count * sizeof(*fields_ptr)); + Local fields_ab = + ArrayBuffer::New(isolate, fields_ptr, fields_count * sizeof(*fields_ptr)); Local fields = Uint32Array::New(fields_ab, 0, fields_count); - target->Set(context, - FIXED_ONE_BYTE_STRING(isolate, "async_hook_fields"), - fields).FromJust(); + target->ForceSet(context, + FIXED_ONE_BYTE_STRING(isolate, "async_hook_fields"), + fields, + ReadOnlyDontDelete).FromJust(); // The following v8::Float64Array has 5 fields. These fields are shared in // this way to allow JS and C++ to read/write each value as quickly as @@ -207,16 +209,17 @@ void AsyncWrap::Initialize(Local target, uid_fields_count * sizeof(*uid_fields_ptr)); Local uid_fields = Float64Array::New(uid_fields_ab, 0, uid_fields_count); - target->Set(context, - FIXED_ONE_BYTE_STRING(isolate, "async_uid_fields"), - uid_fields).FromJust(); + target->ForceSet(context, + FIXED_ONE_BYTE_STRING(isolate, "async_uid_fields"), + uid_fields, + ReadOnlyDontDelete).FromJust(); Local constants = Object::New(isolate); #define SET_HOOKS_CONSTANT(name) \ constants->ForceSet(context, \ FIXED_ONE_BYTE_STRING(isolate, #name), \ Integer::New(isolate, AsyncHooks::name), \ - v8::ReadOnly).FromJust() + ReadOnlyDontDelete).FromJust(); SET_HOOKS_CONSTANT(kInit); SET_HOOKS_CONSTANT(kBefore); SET_HOOKS_CONSTANT(kAfter); @@ -228,16 +231,24 @@ void AsyncWrap::Initialize(Local target, SET_HOOKS_CONSTANT(kInitTriggerId); SET_HOOKS_CONSTANT(kScopedTriggerId); #undef SET_HOOKS_CONSTANT - target->Set(context, FIXED_ONE_BYTE_STRING(isolate, "constants"), constants) - .FromJust(); + target->ForceSet(context, + FIXED_ONE_BYTE_STRING(isolate, "constants"), + constants, + ReadOnlyDontDelete).FromJust(); Local async_providers = Object::New(isolate); #define V(PROVIDER) \ - async_providers->Set(FIXED_ONE_BYTE_STRING(isolate, #PROVIDER), \ - Integer::New(isolate, AsyncWrap::PROVIDER_ ## PROVIDER)); + async_providers->ForceSet( \ + context, \ + FIXED_ONE_BYTE_STRING(isolate, #PROVIDER), \ + Integer::New(isolate, AsyncWrap::PROVIDER_ ## PROVIDER), \ + ReadOnlyDontDelete).FromJust(); NODE_ASYNC_PROVIDER_TYPES(V) #undef V - target->Set(FIXED_ONE_BYTE_STRING(isolate, "Providers"), async_providers); + target->ForceSet(context, + FIXED_ONE_BYTE_STRING(isolate, "Providers"), + async_providers, + ReadOnlyDontDelete).FromJust(); env->set_async_hooks_init_function(Local()); env->set_async_hooks_before_function(Local()); From 2d98170d601146648496b4055bfe0143e523996e Mon Sep 17 00:00:00 2001 From: Trevor Norris Date: Fri, 20 Jan 2017 14:28:36 -0700 Subject: [PATCH 36/40] fix bug in array buffer size in fs --- src/node_file.cc | 6 ++++-- 1 file changed, 4 insertions(+), 2 deletions(-) diff --git a/src/node_file.cc b/src/node_file.cc index 0f8fc7c4b516df..23b029aee84e08 100644 --- a/src/node_file.cc +++ b/src/node_file.cc @@ -1525,9 +1525,11 @@ void InitFs(Local target, Local ab = ArrayBuffer::New(env->isolate(), reinterpret_cast(fd_ids_inst), - 2); + sizeof(*fd_ids_inst)); + static_assert(sizeof(*fd_ids_inst) == 16, "size is incorrect"); Local name = FIXED_ONE_BYTE_STRING(env->isolate(), "fd_async_ids"); - Local value = Float64Array::New(ab, 0, 2); + Local value = + Float64Array::New(ab, 0, sizeof(*fd_ids_inst) / sizeof(double)); target->Set(env->context(), name, value).ToChecked(); StatWatcher::Initialize(env, target); From 7bc7d3c9d84c2c9298e3d340c4e3b216a69cbb2d Mon Sep 17 00:00:00 2001 From: Trevor Norris Date: Thu, 19 Jan 2017 17:35:26 -0700 Subject: [PATCH 37/40] PARTIAL implement new stack tracker In order to get a stack tracker that is performant in JS had to do some ridiculous stuff. This allows the stack to be tracked from both C++ and JS with almost no overhead, and allows it to unwind properly in case there's a fatal exception. --- lib/async_hooks.js | 126 ++++++---- lib/internal/bootstrap_node.js | 28 +-- lib/internal/process/next_tick.js | 123 +++++---- lib/timers.js | 111 +++++--- src/async-wrap.cc | 59 +++-- src/async-wrap.h | 9 + src/env-inl.h | 237 ++++++++++++++++-- src/env.h | 60 +++-- src/node.cc | 19 +- ...test-async-wrap-after-uncaughtexception.js | 28 ++- 10 files changed, 557 insertions(+), 243 deletions(-) diff --git a/lib/async_hooks.js b/lib/async_hooks.js index 40a8f25743ab3c..077c8a56e676fd 100644 --- a/lib/async_hooks.js +++ b/lib/async_hooks.js @@ -9,22 +9,21 @@ const async_hook_fields = async_wrap.async_hook_fields; // that communicates the state of the currentId and triggerId. Fields are as // follows: // kAsyncUidCntr: Maintain state of next unique id. -// kCurrentId: Read/write the id of the current execution context. -// kTriggerId: Read/write the id of the resource responsible for the current -// execution context firing. // kInitTriggerId: Written to just before creating a new resource, so the // constructor knows what other resource is responsible for its init(). // kScopedTriggerId: Hold the init triggerId for all constructors that run // within triggerIdScope(). +// kIdStackIndex: TODO(trevnorris): add explanation +// kIdStackSize: TODO(trevnorris): add explanation const async_uid_fields = async_wrap.async_uid_fields; +// Functions needed to swap the stack if it grows to large. +const { genIdArray, trimIdArray } = async_wrap; +// Stack of scoped trigger id's for triggerIdScope() +const trigger_scope_stack = []; // Array of all AsyncHooks that will be iterated whenever an async event fires. // Using var instead of (preferably const) in order to assign // tmp_active_hooks_array if a hook is enabled/disabled during hook execution. var active_hooks_array = []; -// Stack of scoped trigger id's for triggerIdScope() -const trigger_scope_stack = []; -// Array that holds the current and trigger id's for between before/after. -const current_trigger_id_stack = []; // Track if processing hook callbacks. Used to make sure active_hooks_array // isn't altered in mid execution if another hook is added or removed. var processing_hooks = false; @@ -46,20 +45,15 @@ const destroy_symbol = Symbol('destroy'); // for a given step, that step can bail out early. // The exception is kActiveHooks. Which tracks the total number of AsyncEvents // that exist on "active_hooks_array". -const { kInit, kBefore, kAfter, kDestroy, kActiveHooks, kAsyncUidCntr, - kCurrentId, kTriggerId, kInitTriggerId, kScopedTriggerId } = - async_wrap.constants; - -// Expose the current_trigger_id_stack via the async_wrap binding for internal -// usage. Specifically so that _fatalException() can empty it if the exception -// is caught. -async_wrap.current_trigger_id_stack = current_trigger_id_stack; +const { kInit, kBefore, kAfter, kDestroy, kActiveHooks, kIdStackIndex, + kIdStackSize, kIdStackLimit, kAsyncUidCntr, kInitTriggerId, + kScopedTriggerId } = async_wrap.constants; // Setup the callbacks that node::AsyncWrap will call when there are hooks to // process. They use the same functions as the JS embedder API. async_wrap.setupHooks({ init, - before: emitBeforeS, - after: emitAfterS, + before: emitBeforeN, + after: emitAfterN, destroy: emitDestroyFromNative }); // Used to fatally abort the process if a callback throws. @@ -182,12 +176,12 @@ function createHook(fns) { function currentId() { - return async_uid_fields[kCurrentId]; + return async_wrap.async_id_stack[async_hook_fields[kIdStackIndex]]; } function triggerId() { - return async_uid_fields[kTriggerId]; + return async_wrap.async_id_stack[async_hook_fields[kIdStackIndex] + 1]; } @@ -281,7 +275,8 @@ function initTriggerId() { // Reset value after it's been called so the next constructor doesn't // inherit it by accident. else async_uid_fields[kInitTriggerId] = 0; - if (tId <= 0) tId = async_uid_fields[kCurrentId]; + if (tId <= 0) + tId = async_wrap.async_id_stack[async_hook_fields[kIdStackIndex]]; return tId; } @@ -337,39 +332,58 @@ function emitInitS(id, type, triggerId, handle) { } +function emitBeforeN(id, triggerId) { + for (var i = 0; i < active_hooks_array.length; i++) { + if (typeof active_hooks_array[i][before_symbol] === 'function') { + runCallback(active_hooks_array[i][before_symbol], id); + } + } + + if (tmp_active_hooks_array !== null) { + restoreTmpHooks(); + } +} + + // Usage: emitBeforeS(id[, triggerId]). If triggerId is omitted then id will be // used instead. function emitBeforeS(id, triggerId) { + // CHECK(Number.isSafeInteger(id) && id > 0) + // CHECK(Number.isSafeInteger(triggerId) && triggerId > 0) + // Validate the ids. if (id < 0 || triggerId < 0) fatalError(`before(): id or triggerId < 0 (${id}, ${triggerId})`); - // First setup the currentId and triggerId for the coming callback. - const currentCurrentId = async_uid_fields[kCurrentId]; - const currentTriggerId = async_uid_fields[kTriggerId]; - if (currentCurrentId > 0 || currentTriggerId > 0 || - current_trigger_id_stack.length > 0) { - current_trigger_id_stack.push(currentCurrentId, currentTriggerId); + async_hook_fields[kIdStackIndex] += 2; + async_hook_fields[kIdStackSize] += 2; + + // Doing the assignment first because hitting another stack is irregular. + if (async_hook_fields[kIdStackIndex] >= kIdStackLimit) { + // This call: + // - Creates a new Float64Array and assigns it to async_wrap.async_id_stack + // - Saves the double* to Environment::AsyncHooks + // - Sets kIdStackIndex = 0. + genIdArray(); } - async_uid_fields[kCurrentId] = id; - async_uid_fields[kTriggerId] = triggerId === undefined ? id : triggerId; + + // CHECK(async_hook_fields[kIdStackSize] % kIdStackLimit === + // async_hook_fields[kIdStackIndex]) + + // Even indexes are id, odd indexes are triggerId + async_wrap.async_id_stack[async_hook_fields[kIdStackIndex]] = id; + async_wrap.async_id_stack[async_hook_fields[kIdStackIndex] + 1] = + triggerId === undefined ? id : triggerId; if (async_hook_fields[kBefore] === 0) { return; } - for (var i = 0; i < active_hooks_array.length; i++) { - if (typeof active_hooks_array[i][before_symbol] === 'function') { - runCallback(active_hooks_array[i][before_symbol], id); - } - } - if (tmp_active_hooks_array !== null) { - restoreTmpHooks(); - } + emitBeforeN(id, triggerId); } -function emitAfterS(id) { +function emitAfterN(id) { if (async_hook_fields[kAfter] > 0) { for (var i = 0; i < active_hooks_array.length; i++) { if (typeof active_hooks_array[i][after_symbol] === 'function') { @@ -381,28 +395,38 @@ function emitAfterS(id) { if (tmp_active_hooks_array !== null) { restoreTmpHooks(); } +} - if (id !== async_uid_fields[kCurrentId]) { + +// TODO(trevnorris): Calling emitBefore/emitAfter from native can't adjust the +// kIdStackIndex. But what happens if the user doesn't have both before and +// after callbacks. +function emitAfterS(id) { + // CHECK(Number.isSafeInteger(id) && id > 0) + // CHECK(id === async_wrap.async_id_stack[async_hook_fields[kIdStackIndex]]) + if (id !== async_wrap.async_id_stack[async_hook_fields[kIdStackIndex]]) { fatalError(`async hook stack has become corrupted: ${id}, ` + - async_uid_fields[kCurrentId]); + async_wrap.async_id_stack[async_hook_fields[kIdStackIndex]]); } - // Remove state after the call has completed. - // TODO(trevnorris): Add a check that kCurrentId === id to make sure the - // stack hasn't been corrupted. - if (current_trigger_id_stack.length > 0) { - async_uid_fields[kTriggerId] = current_trigger_id_stack.pop(); - async_uid_fields[kCurrentId] = current_trigger_id_stack.pop(); - } else { - async_uid_fields[kTriggerId] = 0; - async_uid_fields[kCurrentId] = 0; + emitAfterN(id); + + // CHECK(async_hook_fields[kIdStackSize] >= 2) + + async_hook_fields[kIdStackIndex] -= 2; + async_hook_fields[kIdStackSize] -= 2; + + if (async_hook_fields[kIdStackIndex] === 0 && + async_hook_fields[kIdStackSize] > 0) { + // This call: + // - Replaces async_wrap.async_id_stack with the previous one in the stack. + // - Neuters the ArrayBuffer for the previous Float64Array + // - Resets the double* in Environment::AsyncHooks + trimIdArray(); } } -// TODO(trevnorris): If these are delayed until ran then we will have a list -// of id's that may need to run. So instead of forcing native to call into JS -// for every handle, instead have this call back into C++ for the next id. function emitDestroyS(id) { // Return early if there are no destroy callbacks, or on attempt to emit // destroy on the void. diff --git a/lib/internal/bootstrap_node.js b/lib/internal/bootstrap_node.js index 7ed5c77a74a617..1631fdf9d56da3 100644 --- a/lib/internal/bootstrap_node.js +++ b/lib/internal/bootstrap_node.js @@ -282,8 +282,10 @@ function setupProcessFatal() { const async_wrap = process.binding('async_wrap'); - const async_uid_fields = async_wrap.async_uid_fields; - const { kCurrentId, kTriggerId } = async_wrap.constants; + const async_hook_fields = async_wrap.async_hook_fields; + const resetIdArray = async_wrap.resetIdArray; + const { kAfter, kIdStackIndex, kIdStackSize } = + async_wrap.constants; process._fatalException = function(er) { var caught; @@ -306,22 +308,20 @@ // nothing to be done about it at this point. } - // if we handled an error, then make sure any ticks get processed } else { + // If we handled an error, then make sure any ticks get processed NativeModule.require('timers').setImmediate(process._tickCallback); // Emit the after() hooks now that the exception has been delt with. - NativeModule.require('async_hooks').emitAfter( - async_uid_fields[kCurrentId]); - // TODO(trevnorris): This is too blunt a weapon. If there was a stack - // of ids that needed to be processed then after() should be called - // for all of them. - async_uid_fields[kCurrentId] = 0; - async_uid_fields[kTriggerId] = 0; - async_wrap.current_trigger_id_stack.length = 0; - // No need to worry about running restoreTmpHooks() because that's - // only used while other hooks are running and if a hook throws then - // the application is forced to shut down. + if (async_hook_fields[kAfter] > 0) { + while (async_hook_fields[kIdStackSize] > 0) { + NativeModule.require('async_hooks').emitAfter( + async_wrap.async_id_stack[async_hook_fields[kIdStackIndex]]); + } + // Or completely reset the id stack. + } else { + resetIdArray(); + } } return caught; diff --git a/lib/internal/process/next_tick.js b/lib/internal/process/next_tick.js index 8f26819791f3f8..1b9c9190cc4b67 100644 --- a/lib/internal/process/next_tick.js +++ b/lib/internal/process/next_tick.js @@ -10,9 +10,13 @@ function setupNextTick() { const initTriggerId = async_hooks.initTriggerId; // Two arrays that share state between C++ and JS. const { async_uid_fields, async_hook_fields } = async_wrap; + // Two functions to manipulate the id stack. + const { genIdArray, trimIdArray } = async_wrap; + // The needed emit*() functions. + const { emitInit, emitBefore, emitAfter, emitDestroy } = async_hooks; // Grab the constants necessary for working with internal arrays. - const { kInit, kBefore, kAfter, kDestroy, kActiveHooks, kAsyncUidCntr, - kCurrentId, kTriggerId } = async_wrap.constants; + const { kInit, kBefore, kAfter, kDestroy, kAsyncUidCntr, kIdStackIndex, + kIdStackSize, kIdStackLimit } = async_wrap.constants; var nextTickQueue = []; var microtasksScheduled = false; @@ -93,42 +97,66 @@ function setupNextTick() { } } + function nextTickEmitBefore(id, triggerId) { + if (async_hook_fields[kBefore] > 0) + return emitBefore(id, triggerId); + + // Do same basic operations to the id stack as emitBefore/emitAfter. + async_hook_fields[kIdStackIndex] += 2; + async_hook_fields[kIdStackSize] += 2; + + if (async_hook_fields[kIdStackIndex] >= kIdStackLimit) { + genIdArray(); + } + + async_wrap.async_id_stack[async_hook_fields[kIdStackIndex]] = id; + async_wrap.async_id_stack[async_hook_fields[kIdStackIndex] + 1] = + triggerId === undefined ? id : triggerId; + } + + function nextTickEmitAfter(id) { + // CHECK(id > 1) + if (async_hook_fields[kAfter] > 0) + return emitAfter(id); + + async_hook_fields[kIdStackIndex] -= 2; + async_hook_fields[kIdStackSize] -= 2; + + if (async_hook_fields[kIdStackIndex] === 0 && + async_hook_fields[kIdStackSize] > 0) { + trimIdArray(); + } + } + // Run callbacks that have no domain. // Using domains will cause this to be overridden. function _tickCallback() { - var callback, args, tock; - do { while (tickInfo[kIndex] < tickInfo[kLength]) { - const has_hooks = async_hook_fields[kActiveHooks] > 0; - const prev_id = async_uid_fields[kCurrentId]; - const prev_trigger_id = async_uid_fields[kTriggerId]; - - tock = nextTickQueue[tickInfo[kIndex]++]; - callback = tock.callback; - args = tock.args; - - // Setting these fields manually with the expectation that has_hooks - // will almost always be false. Overhead for doing it twice if it is - // true is negligible anyway. - async_uid_fields[kCurrentId] = tock._asyncId; - async_uid_fields[kTriggerId] = tock._triggerId; - if (has_hooks && async_hook_fields[kBefore] > 0) - async_hooks.emitBefore(tock._asyncId, tock._triggerId); + const tock = nextTickQueue[tickInfo[kIndex]++]; + const callback = tock.callback; + const args = tock.args; + + // CHECK(Number.isSafeInteger(tock._asyncId) && tock._asyncId > 0) + // CHECK(Number.isSafeInteger(tock._triggerId) && tock._triggerId > 0) + + nextTickEmitBefore(tock._asyncId, tock._triggerId); + // TODO(trevnorris): Checking kDestroy this way is a cheat because if + // any destroy() hooks are enabled during the callback then they won't + // be notified of this id's destroy. Though that may be alright since + // no other callbacks would have been called for that id. + // Emit destroy() here because if there's an error the destroy() still + // needs to be called for this id. Works because all destroy() are + // called asyncronously. + if (async_hook_fields[kDestroy] > 0) + emitDestroy(tock._asyncId); // Using separate callback execution functions allows direct // callback invocation with small numbers of arguments to avoid the // performance hit associated with using `fn.apply()` _combinedTickCallback(args, callback); - if (has_hooks) { - if (async_hook_fields[kAfter] > 0) - async_hooks.emitAfter(tock._asyncId); - if (async_hook_fields[kDestroy] > 0) - async_hooks.emitDestroy(tock._asyncId); - } - async_uid_fields[kCurrentId] = prev_id; - async_uid_fields[kTriggerId] = prev_trigger_id; + nextTickEmitAfter(tock._asyncId); if (1e4 < tickInfo[kIndex]) tickDone(); @@ -140,42 +168,31 @@ function setupNextTick() { } function _tickDomainCallback() { - var callback, domain, args, tock; - do { while (tickInfo[kIndex] < tickInfo[kLength]) { - const has_hooks = async_hook_fields[kActiveHooks] > 0; - const prev_id = async_uid_fields[kCurrentId]; - const prev_trigger_id = async_uid_fields[kTriggerId]; - - tock = nextTickQueue[tickInfo[kIndex]++]; - callback = tock.callback; - domain = tock.domain; - args = tock.args; + const tock = nextTickQueue[tickInfo[kIndex]++]; + const callback = tock.callback; + const domain = tock.domain; + const args = tock.args; if (domain) domain.enter(); - // Setting these fields manually with the expectation that has_hooks - // will almost always be false. Overhead for doing it twice if it is - // true is negligible anyway. - async_uid_fields[kCurrentId] = tock._asyncId; - async_uid_fields[kTriggerId] = tock._triggerId; - if (has_hooks && async_hook_fields[kBefore]) - async_hooks.emitBefore(tock._asyncId, tock._triggerId); + // CHECK(Number.isSafeInteger(tock._asyncId) && tock._asyncId > 0) + // CHECK(Number.isSafeInteger(tock._triggerId) && tock._triggerId > 0) + + nextTickEmitBefore(tock._asyncId, tock._triggerId); + // Emit destroy() here because if there's an error the destroy() still + // needs to be called for this id. Works because all destroy() are + // called asyncronously. + if (async_hook_fields[kDestroy] > 0) + emitDestroy(tock._asyncId); // Using separate callback execution functions allows direct // callback invocation with small numbers of arguments to avoid the // performance hit associated with using `fn.apply()` _combinedTickCallback(args, callback); - if (has_hooks) { - if (async_hook_fields[kAfter]) - async_hooks.emitAfter(tock._asyncId); - if (async_hook_fields[kDestroy]) - async_hooks.emitDestroy(tock._asyncId); - } - async_uid_fields[kCurrentId] = prev_id; - async_uid_fields[kTriggerId] = prev_trigger_id; + nextTickEmitAfter(tock._asyncId); if (1e4 < tickInfo[kIndex]) tickDone(); @@ -205,7 +222,7 @@ function setupNextTick() { self._asyncId = ++async_uid_fields[kAsyncUidCntr]; self._triggerId = initTriggerId(); if (async_hook_fields[kInit] > 0) - async_hooks.emitInit(self._asyncId, 'TickObject', self._triggerId, self); + emitInit(self._asyncId, 'TickObject', self._triggerId, self); } function nextTick(callback) { diff --git a/lib/timers.js b/lib/timers.js index ebffd2fb46f406..eac81fce49d600 100644 --- a/lib/timers.js +++ b/lib/timers.js @@ -9,11 +9,17 @@ const debug = util.debuglog('timer'); const kOnTimeout = TimerWrap.kOnTimeout | 0; // Two arrays that share state between C++ and JS. const { async_uid_fields, async_hook_fields } = async_wrap; +// Two functions to manipulate the id stack. +const { genIdArray, trimIdArray } = async_wrap; const { emitInit, emitBefore, emitAfter, emitDestroy, initTriggerId } = require('async_hooks'); // Grab the constants necessary for working with internal arrays. -const { kInit, kBefore, kAfter, kDestroy, kCurrentId, kTriggerId, - kAsyncUidCntr } = async_wrap.constants; +const { kInit, kBefore, kAfter, kDestroy, kAsyncUidCntr, kIdStackIndex, + kIdStackSize, kIdStackLimit } = async_wrap.constants; + +const async_id_symbol = Symbol('asyncId'); +const trigger_id_symbol = Symbol('triggerId'); +const not_called_symbol = Symbol('notCalled'); // Timeout values > TIMEOUT_MAX are set to 1. const TIMEOUT_MAX = 2147483647; // 2^31-1 @@ -140,14 +146,14 @@ function insert(item, unrefed) { } // No UID, assign a new one. - if (!item._asyncId) { - item._asyncId = ++async_uid_fields[kAsyncUidCntr]; - item._triggerId = initTriggerId(); + if (!item[async_id_symbol]) { + item[async_id_symbol] = ++async_uid_fields[kAsyncUidCntr]; + item[trigger_id_symbol] = initTriggerId(); // NOTE: While all "item" passed to insert() are emitted with type // 'Timeout', the actual object can be created outside of the Timeout // constructor. Even so, they'll all be emitted as 'Timeout'. if (async_hook_fields[kInit] > 0) - emitInit(item._asyncId, 'Timeout', item._triggerId, item); + emitInit(item[async_id_symbol], 'Timeout', item[trigger_id_symbol], item); } L.append(list, item); @@ -177,6 +183,36 @@ function TimersList(msecs, unrefed) { this.msecs = msecs; } +function timerEmitBefore(id, triggerId) { + if (async_hook_fields[kBefore] > 0) + return emitBefore(id, triggerId); + + // Do same basic operations to the id stack as emitBefore/emitAfter. + async_hook_fields[kIdStackIndex] += 2; + async_hook_fields[kIdStackSize] += 2; + + if (async_hook_fields[kIdStackIndex] >= kIdStackLimit) { + genIdArray(); + } + + async_wrap.async_id_stack[async_hook_fields[kIdStackIndex]] = id; + async_wrap.async_id_stack[async_hook_fields[kIdStackIndex] + 1] = + triggerId === undefined ? id : triggerId; +} + +function timerEmitAfter(id) { + if (async_hook_fields[kAfter] > 0) + return emitAfter(id); + + async_hook_fields[kIdStackIndex] -= 2; + async_hook_fields[kIdStackSize] -= 2; + + if (async_hook_fields[kIdStackIndex] === 0 && + async_hook_fields[kIdStackSize] > 0) { + trimIdArray(); + } +} + function listOnTimeout() { var list = this._list; var msecs = list.msecs; @@ -208,8 +244,8 @@ function listOnTimeout() { assert(timer !== L.peek(list)); if (!timer._onTimeout) { - if (timer._asyncId > 0 && async_hook_fields[kDestroy]) - emitDestroy(timer._asyncId); + if (timer[async_id_symbol] > 0 && async_hook_fields[kDestroy]) + emitDestroy(timer[async_id_symbol]); continue; } @@ -227,25 +263,8 @@ function listOnTimeout() { domain.enter(); } - // Setting these fields manually with the expectation that has_hooks - // will almost always be false. Overhead for doing it twice if it is - // true is negligible anyway. - const prev_id = async_uid_fields[kCurrentId]; - const prev_trigger_id = async_uid_fields[kTriggerId]; - async_uid_fields[kCurrentId] = timer._asyncId; - async_uid_fields[kTriggerId] = timer._triggerId; - if (async_hook_fields[kBefore] > 0) - emitBefore(timer._asyncId, timer._triggerId); - tryOnTimeout(timer, list); - if (async_hook_fields[kAfter] > 0) - emitAfter(timer._asyncId); - if (!timer._repeat && async_hook_fields[kDestroy] > 0) - emitDestroy(timer._asyncId); - async_uid_fields[kCurrentId] = prev_id; - async_uid_fields[kTriggerId] = prev_trigger_id; - if (domain) domain.exit(); } @@ -320,9 +339,9 @@ function reuse(item) { // Remove a timer. Cancels the timeout and resets the relevant timer properties. const unenroll = exports.unenroll = function(item) { - if (item._asyncId > 0) { + if (item[async_id_symbol] > 0 && item[not_called_symbol]) { if (async_hook_fields[kDestroy]) - emitDestroy(item._asyncId); + emitDestroy(item[async_id_symbol]); } var handle = reuse(item); @@ -406,6 +425,14 @@ function createSingleTimeout(callback, after, args) { function ontimeout(timer) { var args = timer._timerArgs; var callback = timer._onTimeout; + timerEmitBefore(timer[async_id_symbol], timer[trigger_id_symbol]); + // TODO(trevnorris): Like with nextTick, this is a bit of a hack to get + // around needing to store the id and wait to check for kDestroy until after + // the callback has run. This is to account for the callback possibly + // throwing an exception. + if (async_hook_fields[kDestroy] > 0) + emitDestroy(timer[async_id_symbol]); + timer[not_called_symbol] = false; if (!args) callback.call(timer); else { @@ -425,6 +452,7 @@ function ontimeout(timer) { } if (timer._repeat) rearm(timer); + timerEmitAfter(timer[async_id_symbol]); } @@ -507,10 +535,11 @@ function Timeout(after, callback, args) { this._onTimeout = callback; this._timerArgs = args; this._repeat = null; - this._asyncId = ++async_uid_fields[kAsyncUidCntr]; - this._triggerId = initTriggerId(); + this[not_called_symbol] = true; + this[async_id_symbol] = ++async_uid_fields[kAsyncUidCntr]; + this[trigger_id_symbol] = initTriggerId(); if (async_hook_fields[kInit] > 0) - emitInit(this._asyncId, 'Timeout', this._triggerId, this); + emitInit(this[async_id_symbol], 'Timeout', this[trigger_id_symbol], this); } @@ -637,8 +666,6 @@ function processImmediate() { if (domain) domain.enter(); - emitBefore(immediate._asyncId, immediate._triggerId); - immediate._callback = immediate._onImmediate; // Save next in case `clearImmediate(immediate)` is called from callback @@ -646,10 +673,6 @@ function processImmediate() { tryOnImmediate(immediate, tail); - emitAfter(immediate._asyncId); - if (async_hook_fields[kDestroy]) - emitDestroy(immediate._asyncId); - if (domain) domain.exit(); @@ -674,6 +697,10 @@ function processImmediate() { // 4.7) what is in this smaller function. function tryOnImmediate(immediate, oldTail) { var threw = true; + timerEmitBefore(immediate[async_id_symbol], immediate[trigger_id_symbol]); + if (async_hook_fields[kDestroy] > 0) + emitDestroy(immediate[async_id_symbol]); + immediate[not_called_symbol] = false; try { // make the actual call outside the try/catch to allow it to be optimized runCallback(immediate); @@ -695,6 +722,7 @@ function tryOnImmediate(immediate, oldTail) { process.nextTick(processImmediate); } } + timerEmitAfter(immediate[async_id_symbol]); } function runCallback(timer) { @@ -726,10 +754,11 @@ function Immediate() { this._argv = null; this._onImmediate = null; this.domain = process.domain; - this._asyncId = ++async_uid_fields[kAsyncUidCntr]; - this._triggerId = initTriggerId(); + this[not_called_symbol] = true; + this[async_id_symbol] = ++async_uid_fields[kAsyncUidCntr]; + this[trigger_id_symbol] = initTriggerId(); if (async_hook_fields[kInit]) - emitInit(this._asyncId, 'Immediate', this._triggerId, this); + emitInit(this[async_id_symbol], 'Immediate', this[trigger_id_symbol], this); } exports.setImmediate = function(callback, arg1, arg2, arg3) { @@ -788,8 +817,8 @@ exports.clearImmediate = function(immediate) { process._needImmediateCallback = false; } - if (immediate._asyncId > 0) { + if (immediate[async_id_symbol] > 0 && immediate[not_called_symbol]) { if (async_hook_fields[kDestroy]) - emitDestroy(immediate._asyncId); + emitDestroy(immediate[async_id_symbol]); } }; diff --git a/src/async-wrap.cc b/src/async-wrap.cc index 3d5912daf5daeb..b26d16df2477cb 100644 --- a/src/async-wrap.cc +++ b/src/async-wrap.cc @@ -9,6 +9,7 @@ #include "v8.h" #include "v8-profiler.h" +using v8::Array; using v8::ArrayBuffer; using v8::Context; using v8::Float64Array; @@ -159,10 +160,22 @@ void AsyncWrap::Initialize(Local target, env->SetMethod(target, "setupHooks", SetupHooks); env->SetMethod(target, "addIdToDestroyList", AddIdToDestroyList); + env->SetMethod(target, "genIdArray", GenIdArray); + env->SetMethod(target, "trimIdArray", TrimIdArray); + env->SetMethod(target, "resetIdArray", ResetIdArray); v8::PropertyAttribute ReadOnlyDontDelete = static_cast(v8::ReadOnly | v8::DontDelete); + // The stack of async and trigger ids will be kept in a Float64Array. With a + // size of 1024 (allowing for 512 recursive calls) it is very unlikely that + // a new array will ever need to be created, but just in case that occation + // comes we can handle it. This array will hold allocations that have filled. + target->ForceSet(context, + FIXED_ONE_BYTE_STRING(isolate, "async_id_stack_list"), + Array::New(isolate), + ReadOnlyDontDelete).FromJust(); + // Attach the uint32_t[] where each slot contains the count of the number of // callbacks waiting to be called on a particular event. It can then be // incremented/decremented from JS quickly to communicate to C++ if there are @@ -184,16 +197,6 @@ void AsyncWrap::Initialize(Local target, // // kAsyncUid: Maintains the state of the next unique id to be assigned. // - // kCurrentId: Is the id of the resource responsible for the current - // execution context. A currentId == 0 means the "void", or that there is - // no JS stack above the init() call (happens when a new handle is created - // for an incoming TCP socket). A currentId == 1 means "root". Or the - // execution context of node::StartNodeInstance. - // - // kTriggerId: Is the id of the resource responsible for init() being called. - // For example, the trigger id of a new connection's TCP handle would be - // the server handle. Whereas the current id at that time would be 0. - // // kInitTriggerId: Write the id of the resource resource responsible for a // handle's creation just before calling the new handle's constructor. // After the new handle is constructed kInitTriggerId is set back to 0. @@ -225,11 +228,12 @@ void AsyncWrap::Initialize(Local target, SET_HOOKS_CONSTANT(kAfter); SET_HOOKS_CONSTANT(kDestroy); SET_HOOKS_CONSTANT(kActiveHooks); + SET_HOOKS_CONSTANT(kIdStackIndex); + SET_HOOKS_CONSTANT(kIdStackSize); SET_HOOKS_CONSTANT(kAsyncUidCntr); - SET_HOOKS_CONSTANT(kCurrentId); - SET_HOOKS_CONSTANT(kTriggerId); SET_HOOKS_CONSTANT(kInitTriggerId); SET_HOOKS_CONSTANT(kScopedTriggerId); + SET_HOOKS_CONSTANT(kIdStackLimit); #undef SET_HOOKS_CONSTANT target->ForceSet(context, FIXED_ONE_BYTE_STRING(isolate, "constants"), @@ -250,6 +254,10 @@ void AsyncWrap::Initialize(Local target, async_providers, ReadOnlyDontDelete).FromJust(); + // Pass the async_wrap object to Environment::AsyncHooks so the + // async_id_stack property can be set automatically. + env->async_hooks()->set_async_wrap_object(target); + env->set_async_hooks_init_function(Local()); env->set_async_hooks_before_function(Local()); env->set_async_hooks_after_function(Local()); @@ -323,6 +331,24 @@ void AsyncWrap::AddIdToDestroyList(const FunctionCallbackInfo& args) { } +void AsyncWrap::GenIdArray(const FunctionCallbackInfo& args) { + Environment* env = Environment::GetCurrent(args); + env->async_hooks()->gen_id_array(); +} + + +void AsyncWrap::TrimIdArray(const FunctionCallbackInfo& args) { + Environment* env = Environment::GetCurrent(args); + env->async_hooks()->trim_id_array(); +} + + +void AsyncWrap::ResetIdArray(const FunctionCallbackInfo& args) { + Environment* env = Environment::GetCurrent(args); + env->async_hooks()->reset_id_array(); +} + + void LoadAsyncWrapperInfo(Environment* env) { HeapProfiler* heap_profiler = env->isolate()->GetHeapProfiler(); #define V(PROVIDER) \ @@ -366,7 +392,7 @@ AsyncWrap::~AsyncWrap() { // the resource is pulled out of the pool and put back into use. void AsyncWrap::Reset() { AsyncHooks* async_hooks = env()->async_hooks(); - id_ = env()->new_async_uid(); + id_ = env()->new_async_id(); trigger_id_ = env()->exchange_init_trigger_id(0); // Nothing to execute, so can continue normally. @@ -379,7 +405,7 @@ void AsyncWrap::Reset() { Local argv[] = { Number::New(env()->isolate(), get_id()), - env()->async_hooks()->provider_string(env()->isolate(), provider_type()), + env()->async_hooks()->provider_string(provider_type()), object(), Number::New(env()->isolate(), get_trigger_id()), }; @@ -491,6 +517,11 @@ Local AsyncWrap::MakeCallback(const Local cb, env()->isolate()->RunMicrotasks(); } + // Make sure the stack unwound properly. If there are nested MakeCallback's + // then it should return early and not reach this code. + CHECK_EQ(env()->current_async_id(), 0); + CHECK_EQ(env()->trigger_id(), 0); + Local process = env()->process_object(); if (tick_info->length() == 0) { diff --git a/src/async-wrap.h b/src/async-wrap.h index d17517719d0243..57ce15c176a136 100644 --- a/src/async-wrap.h +++ b/src/async-wrap.h @@ -72,6 +72,15 @@ class AsyncWrap : public BaseObject { static void AddIdToDestroyList( const v8::FunctionCallbackInfo& args); + // Counterpart to Environment::AsyncHooks::gen_id_array(). + static void GenIdArray(const v8::FunctionCallbackInfo& args); + + // Counterpart to Environment::AsyncHooks::trim_id_array(). + static void TrimIdArray(const v8::FunctionCallbackInfo& args); + + // Counterpart to Environment::AsyncHooks::reset_id_array(). + static void ResetIdArray(const v8::FunctionCallbackInfo& args); + inline ProviderType provider_type() const; inline double get_id() const; diff --git a/src/env-inl.h b/src/env-inl.h index 88b73569537e0d..19782b119268a1 100644 --- a/src/env-inl.h +++ b/src/env-inl.h @@ -60,20 +60,33 @@ inline uint32_t* IsolateData::zero_fill_field() const { } inline Environment::AsyncHooks::AsyncHooks(v8::Isolate* isolate) - : fields_(), - uid_fields_() { + : isolate_(isolate), + fields_(), + uid_fields_(), + id_stack_(new double[AsyncHooks::kIdStackLimit]()) { + v8::HandleScope handle_scope(isolate_); // kAsyncUidCntr should start at 1 because that'll be the id for bootstrap. uid_fields_[AsyncHooks::kAsyncUidCntr] = 1; #define V(Provider) \ providers_[AsyncWrap::PROVIDER_ ## Provider].Set( \ - isolate, \ + isolate_, \ v8::String::NewFromOneByte( \ - isolate, \ + isolate_, \ reinterpret_cast(#Provider), \ v8::NewStringType::kInternalized, \ sizeof(#Provider) - 1).ToLocalChecked()); NODE_ASYNC_PROVIDER_TYPES(V) #undef V + + v8::Local id_stack_ab = v8::ArrayBuffer::New( + isolate_, + id_stack_, + AsyncHooks::kIdStackLimit * sizeof(double)); + v8::Local id_stack_array = + v8::Float64Array::New(id_stack_ab, 0, AsyncHooks::kIdStackLimit); + v8::Persistent* p_float_array = + new v8::Persistent(isolate_, id_stack_array); + stack_of_id_stacks_.push(p_float_array); } inline uint32_t* Environment::AsyncHooks::fields() { @@ -88,16 +101,198 @@ inline double* Environment::AsyncHooks::uid_fields() { return uid_fields_; } -inline v8::Local Environment::AsyncHooks::provider_string( - v8::Isolate* isolate, - int idx) { - return providers_[idx].Get(isolate); -} - inline int Environment::AsyncHooks::uid_fields_count() const { return kUidFieldsCount; } +inline v8::Local Environment::AsyncHooks::provider_string(int idx) { + return providers_[idx].Get(isolate_); +} + +inline double* Environment::AsyncHooks::get_id_stack() { + return id_stack_; +} + +inline void Environment::AsyncHooks::push_to_id_stack(double id, + double trigger_id) { + CHECK_GE(id, 0); + CHECK_GE(trigger_id, 0); + // First make sure that JS hasn't totally screwed up the stack. + CHECK_LE(fields_[AsyncHooks::kIdStackIndex], AsyncHooks::kIdStackLimit - 2); + // Also verify that Size and Index are in sync. + CHECK_EQ(fields_[AsyncHooks::kIdStackSize] % AsyncHooks::kIdStackLimit, + fields_[AsyncHooks::kIdStackIndex]); + + fields_[AsyncHooks::kIdStackIndex] += 2; + fields_[AsyncHooks::kIdStackSize] += 2; + + if (fields_[AsyncHooks::kIdStackIndex] == AsyncHooks::kIdStackLimit) { + gen_id_array(); + + // Final check to make sure we're writing to the right spot in memory. + CHECK_LE(fields_[AsyncHooks::kIdStackIndex], AsyncHooks::kIdStackLimit - 2); + } + + id_stack_[fields_[AsyncHooks::kIdStackIndex]] = id; + id_stack_[fields_[AsyncHooks::kIdStackIndex] + 1] = trigger_id; +} + +inline void Environment::AsyncHooks::pop_from_id_stack(double id) { + // In case of a fatal exception then this may have already been reset, + // if the stack was multiple MakeCallback()'s deep. + if (fields_[AsyncHooks::kIdStackSize] == 0) { + // Sanity check to make sure the id stack hasn't become corrupted. + CHECK_EQ(fields_[AsyncHooks::kIdStackIndex], 0); + return; + } + + // Make sure the stack hasn't become corrupted. + CHECK_EQ(id_stack_[fields_[AsyncHooks::kIdStackIndex]], id); + + // Fast path where there's probably no extra stack. + if (fields_[AsyncHooks::kIdStackSize] < AsyncHooks::kIdStackLimit) { + CHECK_GE(fields_[AsyncHooks::kIdStackIndex], 2); + CHECK_EQ(fields_[AsyncHooks::kIdStackIndex], + fields_[AsyncHooks::kIdStackSize]); + fields_[AsyncHooks::kIdStackIndex] -= 2; + fields_[AsyncHooks::kIdStackSize] -= 2; + return; + } + + if (fields_[AsyncHooks::kIdStackIndex] == 0) { + trim_id_array(); + } + + CHECK_EQ(fields_[AsyncHooks::kIdStackSize] % AsyncHooks::kIdStackLimit, + fields_[AsyncHooks::kIdStackIndex]); + + fields_[AsyncHooks::kIdStackIndex] -= 2; + fields_[AsyncHooks::kIdStackSize] -= 2; +} + +inline void Environment::AsyncHooks::gen_id_array() { + // This should only be called when there are enough ids to merit creating + // another id array. + CHECK_EQ(fields_[AsyncHooks::kIdStackIndex], AsyncHooks::kIdStackLimit); + + v8::HandleScope handle_scope(isolate_); + id_stack_ = new double[AsyncHooks::kIdStackLimit](); + v8::Local id_stack_ab = v8::ArrayBuffer::New( + isolate_, + id_stack_, + AsyncHooks::kIdStackLimit * sizeof(double)); + v8::Local id_stack_array = + v8::Float64Array::New(id_stack_ab, 0, AsyncHooks::kIdStackLimit); + v8::Persistent* p_array = + new v8::Persistent(isolate_, id_stack_array); + stack_of_id_stacks_.push(p_array); + + fields_[AsyncHooks::kIdStackIndex] = 0; + + // If we happen to need a new stack before the async_wrap object has been set + // then return early. + if (async_wrap_object_.IsEmpty()) + return; + + async_wrap_object_.Get(isolate_)->Set( + isolate_->GetCurrentContext(), + FIXED_ONE_BYTE_STRING(isolate_, "async_id_stack"), + StrongPersistentToLocal(*(stack_of_id_stacks_.top()))).FromJust(); +} + +inline void Environment::AsyncHooks::trim_id_array() { + // This shouldn't be called if there's only one id stack left. + CHECK_GT(stack_of_id_stacks_.size(), 1); + CHECK_EQ(fields_[AsyncHooks::kIdStackIndex], 0); + CHECK_GE(fields_[AsyncHooks::kIdStackSize], AsyncHooks::kIdStackLimit - 2); + + // Proper cleanup of the previous Persistent + v8::HandleScope handle_scope(isolate_); + v8::Persistent* p_array = stack_of_id_stacks_.top(); + v8::Local ab = StrongPersistentToLocal(*p_array)->Buffer(); + double* data = reinterpret_cast(ab->GetContents().Data()); + stack_of_id_stacks_.pop(); + p_array->Reset(); + ab->Neuter(); + delete[] data; + delete p_array; + + // Reset id_stack_ to old Float64Array. + p_array = stack_of_id_stacks_.top(); + ab = StrongPersistentToLocal(*p_array)->Buffer(); + id_stack_ = reinterpret_cast(ab->GetContents().Data()); + + fields_[AsyncHooks::kIdStackIndex] = AsyncHooks::kIdStackLimit - 2; + + CHECK_EQ(fields_[AsyncHooks::kIdStackSize] % AsyncHooks::kIdStackLimit, + fields_[AsyncHooks::kIdStackIndex]); + + // If we happen to need a new stack before the async_wrap object has been set + // then return early. + if (async_wrap_object_.IsEmpty()) + return; + + async_wrap_object_.Get(isolate_)->Set( + isolate_->GetCurrentContext(), + FIXED_ONE_BYTE_STRING(isolate_, "async_id_stack"), + StrongPersistentToLocal(*p_array)).FromJust(); +} + +inline void Environment::AsyncHooks::reset_id_array() { + v8::HandleScope handle_scope(isolate_); + + while (stack_of_id_stacks_.size() > 1) { + v8::Persistent* p_array = stack_of_id_stacks_.top(); + v8::Local ab = StrongPersistentToLocal(*p_array)->Buffer(); + double* data = reinterpret_cast(ab->GetContents().Data()); + stack_of_id_stacks_.pop(); + p_array->Reset(); + ab->Neuter(); + delete[] data; + delete p_array; + } + + v8::Persistent* p_array = stack_of_id_stacks_.top(); + v8::Local ab = StrongPersistentToLocal(*p_array)->Buffer(); + id_stack_ = reinterpret_cast(ab->GetContents().Data()); + + fields_[AsyncHooks::kIdStackIndex] = 0; + fields_[AsyncHooks::kIdStackSize] = 0; +} + +inline void Environment::AsyncHooks::set_async_wrap_object( + v8::Local object) { + // This should only be set once. + CHECK(async_wrap_object_.IsEmpty()); + + v8::HandleScope handle_scope(isolate_); + async_wrap_object_.Set(isolate_, object); + object->Set(isolate_->GetCurrentContext(), + FIXED_ONE_BYTE_STRING(isolate_, "async_id_stack"), + StrongPersistentToLocal(*(stack_of_id_stacks_.top()))).FromJust(); +} + +inline Environment::AsyncHooks::ExecScope::ExecScope( + Environment* env, double id, double trigger_id) + : env_(env), + id_(id), + disposed_(false) { + env->async_hooks()->push_to_id_stack(id, trigger_id); + + auto fields = env->async_hooks()->fields(); + CHECK_EQ(fields[AsyncHooks::kIdStackSize] % AsyncHooks::kIdStackLimit, + fields[AsyncHooks::kIdStackIndex]); +} + +inline void Environment::AsyncHooks::ExecScope::Dispose() { + disposed_ = true; + env_->async_hooks()->pop_from_id_stack(id_); + + auto fields = env_->async_hooks()->fields(); + CHECK_EQ(fields[AsyncHooks::kIdStackSize] % AsyncHooks::kIdStackLimit, + fields[AsyncHooks::kIdStackIndex]); +} + inline Environment::AsyncCallbackScope::AsyncCallbackScope(Environment* env) : env_(env) { env_->makecallback_cntr_++; @@ -339,28 +534,18 @@ inline std::vector* Environment::destroy_ids_list() { return &destroy_ids_list_; } -inline double Environment::new_async_uid() { +inline double Environment::new_async_id() { return ++async_hooks()->uid_fields()[AsyncHooks::kAsyncUidCntr]; } inline double Environment::current_async_id() { - return async_hooks()->uid_fields()[AsyncHooks::kCurrentId]; -} - -inline double Environment::exchange_current_async_id(const double id) { - const double oid = async_hooks()->uid_fields()[AsyncHooks::kCurrentId]; - async_hooks()->uid_fields()[AsyncHooks::kCurrentId] = id; - return oid; + return async_hooks()->get_id_stack()[ + async_hooks()->fields()[AsyncHooks::kIdStackIndex]]; } inline double Environment::trigger_id() { - return async_hooks()->uid_fields()[AsyncHooks::kTriggerId]; -} - -inline double Environment::exchange_trigger_id(const double id) { - const double oid = async_hooks()->uid_fields()[AsyncHooks::kTriggerId]; - async_hooks()->uid_fields()[AsyncHooks::kTriggerId] = id; - return oid; + return async_hooks()->get_id_stack()[ + async_hooks()->fields()[AsyncHooks::kIdStackIndex] + 1]; } inline double Environment::exchange_init_trigger_id(const double id) { @@ -368,7 +553,7 @@ inline double Environment::exchange_init_trigger_id(const double id) { double tid = uid_fields[AsyncHooks::kInitTriggerId]; uid_fields[AsyncHooks::kInitTriggerId] = id; if (tid <= 0) tid = uid_fields[AsyncHooks::kScopedTriggerId]; - if (tid <= 0) tid = uid_fields[AsyncHooks::kCurrentId]; + if (tid <= 0) tid = current_async_id(); return tid; } diff --git a/src/env.h b/src/env.h index d12f9ff1d3ec50..045547534bd8b1 100644 --- a/src/env.h +++ b/src/env.h @@ -18,6 +18,7 @@ #include #include #include +#include // Caveat emptor: we're going slightly crazy with macros here but the end // hopefully justifies the means. We have a lot of per-context properties @@ -317,8 +318,6 @@ class Environment { // and the other as a uint32_t*. enum UidFields { kAsyncUidCntr, - kCurrentId, - kTriggerId, kInitTriggerId, kScopedTriggerId, kUidFieldsCount, @@ -330,14 +329,26 @@ class Environment { kAfter, kDestroy, kActiveHooks, + kIdStackIndex, + kIdStackSize, kFieldsCount, }; + static const size_t kIdStackLimit = 1024; + inline uint32_t* fields(); inline int fields_count() const; inline double* uid_fields(); inline int uid_fields_count() const; - inline v8::Local provider_string(v8::Isolate* isolate, int idx); + inline v8::Local provider_string(int idx); + + inline double* get_id_stack(); + inline void push_to_id_stack(double id, double trigger_id); + inline void pop_from_id_stack(double id); + inline void gen_id_array(); + inline void trim_id_array(); + inline void reset_id_array(); // Used in fatal exceptions. + inline void set_async_wrap_object(v8::Local object); class InitScope { public: @@ -356,35 +367,22 @@ class Environment { DISALLOW_COPY_AND_ASSIGN(InitScope); }; - // ExecScope is meant for use in MakeCallback, to maintained stacked - // state. - // TODO(trevnorris): This conflicts with how emitBefore/emitAfter work - // (manually tracking the stacks in a JS array). Technically they should - // play nicely together, but write tests to prove this. + // ExecScope is meant for use in MakeCallback, to manage the id stack. class ExecScope { public: - explicit ExecScope(Environment* env, double id, double trigger_id) - : uid_fields_(env->async_hooks()->uid_fields()), - id_(uid_fields_[AsyncHooks::kCurrentId]), - trigger_id_(uid_fields_[AsyncHooks::kTriggerId]), - disposed_(false) { - uid_fields_[AsyncHooks::kCurrentId] = id; - uid_fields_[AsyncHooks::kTriggerId] = trigger_id; - } + explicit ExecScope(Environment* env, double id, double trigger_id); ~ExecScope() { if (disposed_) return; Dispose(); } - void Dispose() { - disposed_ = true; - uid_fields_[AsyncHooks::kCurrentId] = id_; - uid_fields_[AsyncHooks::kTriggerId] = trigger_id_; - } + void Dispose(); private: - double* uid_fields_; - const double id_; - const double trigger_id_; + ExecScope() { } + Environment* env_; + double id_; + // Manually track if disposed so if the user calls Dispose() and it's + // RAII it won't alter the id stack twice. bool disposed_; DISALLOW_COPY_AND_ASSIGN(ExecScope); @@ -397,11 +395,21 @@ class Environment { // Keep a list of all Persistent strings used for Provider types. v8::Eternal providers_[AsyncWrap::PROVIDERS_LENGTH]; + // Store a reference to the async_wrap object so we can override the + // async_id_stack property if the stack grows too large. + v8::Eternal async_wrap_object_; + + std::stack*> stack_of_id_stacks_; + + v8::Isolate* isolate_; uint32_t fields_[kFieldsCount]; // Gives us 2^53-1 unique ids. Good enough for now and makes the operation // cheaper in JS. double uid_fields_[kUidFieldsCount]; + // Pointer to the data in the Float64Array that holds the current stack of + // ids. + double* id_stack_; DISALLOW_COPY_AND_ASSIGN(AsyncHooks); }; @@ -543,11 +551,9 @@ class Environment { inline void set_trace_sync_io(bool value); // The necessary API for async_hooks. - inline double new_async_uid(); + inline double new_async_id(); inline double current_async_id(); - inline double exchange_current_async_id(const double id); inline double trigger_id(); - inline double exchange_trigger_id(const double id); inline double exchange_init_trigger_id(const double id); inline void set_init_trigger_id(const double id); diff --git a/src/node.cc b/src/node.cc index 31d52c601745cc..9da2cc275d87d6 100644 --- a/src/node.cc +++ b/src/node.cc @@ -132,6 +132,8 @@ using v8::Uint32Array; using v8::V8; using v8::Value; +using AsyncHooks = node::Environment::AsyncHooks; + static bool print_eval = false; static bool force_repl = false; static bool syntax_check_only = false; @@ -1209,6 +1211,10 @@ Local MakeCallback(Environment* env, } } + // TODO(trevnorris): Correct this once node::MakeCallback() support id and + // triggerId. + AsyncHooks::ExecScope exec_scope(env, 0, 0); + Local ret = callback->Call(recv, argc, argv); if (ret.IsEmpty()) { @@ -1218,6 +1224,8 @@ Local MakeCallback(Environment* env, ret : Undefined(env->isolate()).As(); } + exec_scope.Dispose(); + if (has_domain) { Local exit_v = domain->Get(env->exit_string()); if (exit_v->IsFunction()) { @@ -1238,12 +1246,13 @@ Local MakeCallback(Environment* env, env->isolate()->RunMicrotasks(); } - Local process = env->process_object(); - - // Make sure the stack unwound properly. + // Make sure the stack unwound properly. If there are nested MakeCallback's + // then it should return early and not reach this code. CHECK_EQ(env->current_async_id(), 0); CHECK_EQ(env->trigger_id(), 0); + Local process = env->process_object(); + if (tick_info->length() == 0) { tick_info->set_index(0); return ret; @@ -4369,10 +4378,6 @@ inline int Start(Isolate* isolate, IsolateData* isolate_data, } env.set_trace_sync_io(trace_sync_io); - // A current_async_id() of 1 means bootstrap phase. Now that the bootstrap - // phase is complete set the global id to 0 to let the internals know that - // everything points to the void. - env.exchange_current_async_id(0); // Enable debugger if (debug_enabled) diff --git a/test/parallel/test-async-wrap-after-uncaughtexception.js b/test/parallel/test-async-wrap-after-uncaughtexception.js index 5cb47f0615007f..082d52fec2ea80 100644 --- a/test/parallel/test-async-wrap-after-uncaughtexception.js +++ b/test/parallel/test-async-wrap-after-uncaughtexception.js @@ -20,21 +20,29 @@ async_hooks.createHook({ }, }).enable(); -const simId = setImmediate(() => { +const si = setImmediate(() => { throw new Error('setImmediate'); -})._asyncId; -thrown_ids[simId] = 'setImmediate'; - -const stId = setTimeout(() => { +}); +let async_id_symbol = null; +for (const i of Object.getOwnPropertySymbols(si)) { + if (i.toString() === 'Symbol(asyncId)') { + async_id_symbol = i; + break; + } +} +assert.ok(async_id_symbol !== null); +thrown_ids[si[async_id_symbol]] = 'setImmediate'; + +const st = setTimeout(() => { throw new Error('setTimeout'); -}, 10)._asyncId; -thrown_ids[stId] = 'setTimeout'; +}, 10); +thrown_ids[st[async_id_symbol]] = 'setTimeout'; -const sinId = setInterval(function() { +const sin = setInterval(function() { clearInterval(this); throw new Error('setInterval'); -}, 10)._asyncId; -thrown_ids[sinId] = 'setInterval'; +}, 10); +thrown_ids[sin[async_id_symbol]] = 'setInterval'; const rbId = require('crypto').randomBytes(1, () => { throw new Error('RANDOMBYTESREQUEST'); From edf67a79a4162848dea12e2dbb88e8e6562a234e Mon Sep 17 00:00:00 2001 From: Trevor Norris Date: Thu, 26 Jan 2017 09:52:22 -0700 Subject: [PATCH 38/40] timers: don't emit destroy() on same id twice --- lib/timers.js | 13 +++++-------- 1 file changed, 5 insertions(+), 8 deletions(-) diff --git a/lib/timers.js b/lib/timers.js index eac81fce49d600..34e60c06a23cfc 100644 --- a/lib/timers.js +++ b/lib/timers.js @@ -244,8 +244,6 @@ function listOnTimeout() { assert(timer !== L.peek(list)); if (!timer._onTimeout) { - if (timer[async_id_symbol] > 0 && async_hook_fields[kDestroy]) - emitDestroy(timer[async_id_symbol]); continue; } @@ -339,11 +337,6 @@ function reuse(item) { // Remove a timer. Cancels the timeout and resets the relevant timer properties. const unenroll = exports.unenroll = function(item) { - if (item[async_id_symbol] > 0 && item[not_called_symbol]) { - if (async_hook_fields[kDestroy]) - emitDestroy(item[async_id_symbol]); - } - var handle = reuse(item); if (handle) { debug('unenroll: list empty'); @@ -430,7 +423,7 @@ function ontimeout(timer) { // around needing to store the id and wait to check for kDestroy until after // the callback has run. This is to account for the callback possibly // throwing an exception. - if (async_hook_fields[kDestroy] > 0) + if (async_hook_fields[kDestroy] > 0 && timer[not_called_symbol]) emitDestroy(timer[async_id_symbol]); timer[not_called_symbol] = false; if (!args) @@ -600,6 +593,10 @@ Timeout.prototype.close = function() { } else { unenroll(this); } + if (async_hook_fields[kDestroy] && this[not_called_symbol]) { + emitDestroy(this[async_id_symbol]); + } + this[not_called_symbol] = false; return this; }; From 90f0a634524014441e0d0f7087fcfa0a5bc9de2d Mon Sep 17 00:00:00 2001 From: Trevor Norris Date: Thu, 26 Jan 2017 09:53:17 -0700 Subject: [PATCH 39/40] test: make async hooks test better --- test/common.js | 17 +++++++++++------ 1 file changed, 11 insertions(+), 6 deletions(-) diff --git a/test/common.js b/test/common.js index df2c4da06213a2..5503f05cfb26b7 100644 --- a/test/common.js +++ b/test/common.js @@ -14,7 +14,7 @@ const execSync = require('child_process').execSync; const testRoot = process.env.NODE_TEST_DIR ? fs.realpathSync(process.env.NODE_TEST_DIR) : __dirname; -// If env var is set then enable noop async_hook hooks for testing. +// If env var is set then enable async_hook hooks for all tests. if (process.env.NODE_TEST_WITH_ASYNC_HOOKS) { const destroydIdsList = {}; const destroyListList = {}; @@ -31,7 +31,7 @@ if (process.env.NODE_TEST_WITH_ASYNC_HOOKS) { const _addIdToDestroyList = async_wrap.addIdToDestroyList; async_wrap.addIdToDestroyList = function addIdToDestroyList(id) { - if (!process.env.NODE_CHECK_ASYNC_DESTROY) + if (!process.env.NODE_TEST_ASYNC_DESTROY) return _addIdToDestroyList.call(this, id); if (destroyListList[id] !== undefined) { process._rawDebug(destroyListList[id]); @@ -42,15 +42,20 @@ if (process.env.NODE_TEST_WITH_ASYNC_HOOKS) { }; require('async_hooks').createHook({ - init(id, ty, tr, h) { initHandles[id] = h; }, + init(id, ty, tr, h) { + if (initHandles[id]) { + throw new Error(`init called twice for same id (${id})`); + } + initHandles[id] = h; + }, before() { }, - after() { }, + after: process.env.NODE_TEST_ONLY_BEFORE_HOOK ? undefined : () => {}, destroy(id) { - if (!process.env.NODE_CHECK_ASYNC_DESTROY) return; + if (!process.env.NODE_TEST_ASYNC_DESTROY) return; if (destroydIdsList[id] !== undefined) { process._rawDebug(destroydIdsList[id]); process._rawDebug(); - throw new Error(`destroy called for same id(${id})`); + throw new Error(`destroy called for same id (${id})`); } destroydIdsList[id] = new Error().stack; }, From 1ff2dfbfe067e1925a1f6d2f4e42a0811b9663c8 Mon Sep 17 00:00:00 2001 From: Trevor Norris Date: Tue, 31 Jan 2017 23:06:27 -0700 Subject: [PATCH 40/40] async_wrap: print msg on error instead of abort Instead of using a CHECK(), print an error message to the user. Check if --abort-on-uncaught-exception has been passed, and abort if this is the case. Otherwise exit(1). --- lib/async_hooks.js | 11 +++++++---- src/env-inl.h | 25 ++++++++++++++++++++++++- src/env.h | 5 +++++ src/node.cc | 6 ++++++ 4 files changed, 42 insertions(+), 5 deletions(-) diff --git a/lib/async_hooks.js b/lib/async_hooks.js index 077c8a56e676fd..6461c146807f16 100644 --- a/lib/async_hooks.js +++ b/lib/async_hooks.js @@ -352,8 +352,10 @@ function emitBeforeS(id, triggerId) { // CHECK(Number.isSafeInteger(triggerId) && triggerId > 0) // Validate the ids. - if (id < 0 || triggerId < 0) - fatalError(`before(): id or triggerId < 0 (${id}, ${triggerId})`); + if (id < 0 || triggerId < 0) { + fatalError('before(): id or triggerId is less than zero ' + + `(id: ${id}, triggerId: ${triggerId})`); + } async_hook_fields[kIdStackIndex] += 2; async_hook_fields[kIdStackSize] += 2; @@ -405,8 +407,9 @@ function emitAfterS(id) { // CHECK(Number.isSafeInteger(id) && id > 0) // CHECK(id === async_wrap.async_id_stack[async_hook_fields[kIdStackIndex]]) if (id !== async_wrap.async_id_stack[async_hook_fields[kIdStackIndex]]) { - fatalError(`async hook stack has become corrupted: ${id}, ` + - async_wrap.async_id_stack[async_hook_fields[kIdStackIndex]]); + fatalError('async hook stack has become corrupted (actual: ' + + async_wrap.async_id_stack[async_hook_fields[kIdStackIndex]] + + `, expected: ${id}`); } emitAfterN(id); diff --git a/src/env-inl.h b/src/env-inl.h index 19782b119268a1..c71fda6c235385 100644 --- a/src/env-inl.h +++ b/src/env-inl.h @@ -147,7 +147,21 @@ inline void Environment::AsyncHooks::pop_from_id_stack(double id) { } // Make sure the stack hasn't become corrupted. - CHECK_EQ(id_stack_[fields_[AsyncHooks::kIdStackIndex]], id); + if (id_stack_[fields_[AsyncHooks::kIdStackIndex]] != id) { + fprintf(stderr, + "Error: async hook stack has become corrupted (" + "actual: %'.f, expected: %'.f)\n", + id_stack_[fields_[AsyncHooks::kIdStackIndex]], + id); + Environment* env = Environment::GetCurrent(isolate_); + DumpBacktrace(stderr); + fflush(stderr); + if (!env->abort_on_uncaught_exception()) + exit(1); + fprintf(stderr, "\n"); + fflush(stderr); + ABORT_NO_BACKTRACE(); + } // Fast path where there's probably no extra stack. if (fields_[AsyncHooks::kIdStackSize] < AsyncHooks::kIdStackLimit) { @@ -385,6 +399,7 @@ inline Environment::Environment(IsolateData* isolate_data, using_domains_(false), printed_error_(false), trace_sync_io_(false), + abort_on_uncaught_exception_(false), makecallback_cntr_(0), fd_async_ids_inst_(), debugger_agent_(this), @@ -530,6 +545,14 @@ inline void Environment::set_trace_sync_io(bool value) { trace_sync_io_ = value; } +inline bool Environment::abort_on_uncaught_exception() const { + return abort_on_uncaught_exception_; +} + +inline void Environment::set_abort_on_uncaught_exception(bool value) { + abort_on_uncaught_exception_ = value; +} + inline std::vector* Environment::destroy_ids_list() { return &destroy_ids_list_; } diff --git a/src/env.h b/src/env.h index 045547534bd8b1..f59d5da0858857 100644 --- a/src/env.h +++ b/src/env.h @@ -233,6 +233,7 @@ namespace node { V(async_hooks_destroy_function, v8::Function) \ V(async_hooks_init_function, v8::Function) \ V(async_hooks_before_function, v8::Function) \ + V(async_hooks_fatal_error_function, v8::Function) \ V(async_hooks_after_function, v8::Function) \ V(binding_cache_object, v8::Object) \ V(buffer_constructor_function, v8::Function) \ @@ -550,6 +551,9 @@ class Environment { void PrintSyncTrace() const; inline void set_trace_sync_io(bool value); + inline bool abort_on_uncaught_exception() const; + inline void set_abort_on_uncaught_exception(bool value); + // The necessary API for async_hooks. inline double new_async_id(); inline double current_async_id(); @@ -662,6 +666,7 @@ class Environment { bool using_domains_; bool printed_error_; bool trace_sync_io_; + bool abort_on_uncaught_exception_; size_t makecallback_cntr_; double async_wrap_id_; std::vector destroy_ids_list_; diff --git a/src/node.cc b/src/node.cc index 9da2cc275d87d6..393dc1aeb2d56b 100644 --- a/src/node.cc +++ b/src/node.cc @@ -155,6 +155,7 @@ static node_module* modlist_linked; static node_module* modlist_addon; static bool trace_enabled = false; static const char* trace_enabled_categories = nullptr; +static bool abort_on_uncaught_exception = false; #if defined(NODE_HAVE_I18N_SUPPORT) // Path to ICU data (for i18n / Intl) @@ -3698,6 +3699,9 @@ static void ParseArgs(int* argc, } else if (strcmp(arg, "--expose-internals") == 0 || strcmp(arg, "--expose_internals") == 0) { // consumed in js + } else if (strcmp(arg, "--abort-on-uncaught-exception") || + strcmp(arg, "--abort_on_uncaught_exception")) { + abort_on_uncaught_exception = true; } else if (strcmp(arg, "--") == 0) { index += 1; break; @@ -4371,6 +4375,8 @@ inline int Start(Isolate* isolate, IsolateData* isolate_data, return 12; // Signal internal error. } + env.set_abort_on_uncaught_exception(abort_on_uncaught_exception); + { Environment::AsyncCallbackScope callback_scope(&env); Environment::AsyncHooks::ExecScope exec_scope(&env, 1, 0);