From 06453a94a7b06df30be0148e8b1d89932350f677 Mon Sep 17 00:00:00 2001 From: Timothy J Fontaine Date: Mon, 3 Mar 2014 16:27:58 -0800 Subject: [PATCH 1/8] src: domain should not replace nextTick function Previously if you cached process.nextTick and then require('domain') subsequent nextTick() calls would not be caught because enqueued functions were taking the wrong path. This keeps nextTick to a single function reference and changes the implementation details after domain has been required. --- src/node.cc | 2 +- src/node.js | 8 ++++++-- test/message/max_tick_depth_trace.out | 2 ++ test/simple/test-next-tick-domain.js | 29 +++++++++++++++++++++++++++ 4 files changed, 38 insertions(+), 3 deletions(-) create mode 100644 test/simple/test-next-tick-domain.js diff --git a/src/node.cc b/src/node.cc index a18e0d89d5c..ac906f0cd5c 100644 --- a/src/node.cc +++ b/src/node.cc @@ -918,7 +918,7 @@ Handle UsingDomains(const Arguments& args) { Local tdc = tdc_v.As(); Local ndt = ndt_v.As(); process->Set(String::New("_tickCallback"), tdc); - process->Set(String::New("nextTick"), ndt); + process->Set(String::New("_currentTickHandler"), ndt); process_tickCallback.Dispose(); // Possibly already set by MakeCallback(). process_tickCallback = Persistent::New(tdc); return Undefined(); diff --git a/src/node.js b/src/node.js index bdbe1c5e16d..b54ff51752a 100644 --- a/src/node.js +++ b/src/node.js @@ -331,8 +331,12 @@ var index = 1; var depth = 2; - process.nextTick = nextTick; + process.nextTick = function nextTick(cb) { + process._currentTickHandler(cb); + }; + // needs to be accessible from cc land + process._currentTickHandler = _nextTick; process._nextDomainTick = _nextDomainTick; process._tickCallback = _tickCallback; process._tickDomainCallback = _tickDomainCallback; @@ -472,7 +476,7 @@ tickDone(0); } - function nextTick(callback) { + function _nextTick(callback) { // on the way out, don't bother. it won't get fired anyway. if (process._exiting) return; diff --git a/test/message/max_tick_depth_trace.out b/test/message/max_tick_depth_trace.out index 22184a69119..c9db2c5580a 100644 --- a/test/message/max_tick_depth_trace.out +++ b/test/message/max_tick_depth_trace.out @@ -10,6 +10,7 @@ tick 12 tick 11 Trace: (node) warning: Recursive process.nextTick detected. This will break in the next version of node. Please use setImmediate for recursive deferral. at maxTickWarn (node.js:*:*) + at process._nextTick [as _currentTickHandler] (node.js:*:*) at process.nextTick (node.js:*:*) at f (*test*message*max_tick_depth_trace.js:*:*) at process._tickCallback (node.js:*:*) @@ -28,6 +29,7 @@ tick 2 tick 1 Trace: (node) warning: Recursive process.nextTick detected. This will break in the next version of node. Please use setImmediate for recursive deferral. at maxTickWarn (node.js:*:*) + at process._nextTick [as _currentTickHandler] (node.js:*:*) at process.nextTick (node.js:*:*) at f (*test*message*max_tick_depth_trace.js:*:*) at process._tickCallback (node.js:*:*) diff --git a/test/simple/test-next-tick-domain.js b/test/simple/test-next-tick-domain.js new file mode 100644 index 00000000000..16f77ed94d5 --- /dev/null +++ b/test/simple/test-next-tick-domain.js @@ -0,0 +1,29 @@ +// Copyright Joyent, Inc. and other Node contributors. +// +// Permission is hereby granted, free of charge, to any person obtaining a +// copy of this software and associated documentation files (the +// "Software"), to deal in the Software without restriction, including +// without limitation the rights to use, copy, modify, merge, publish, +// distribute, sublicense, and/or sell copies of the Software, and to permit +// persons to whom the Software is furnished to do so, subject to the +// following conditions: +// +// The above copyright notice and this permission notice shall be included +// in all copies or substantial portions of the Software. +// +// THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS +// OR IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF +// MERCHANTABILITY, FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT. IN +// NO EVENT SHALL THE AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, +// DAMAGES OR OTHER LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR +// OTHERWISE, ARISING FROM, OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE +// USE OR OTHER DEALINGS IN THE SOFTWARE. + +var common = require('../common'); +var assert = require('assert'); + +var origNextTick = process.nextTick; + +require('domain'); + +assert.strictEqual(origNextTick, process.nextTick, 'Requiring domain should not change nextTick'); From 6eb4d1d15ca8d2b82cf9db99d56633357ccc3320 Mon Sep 17 00:00:00 2001 From: Greg Brail Date: Tue, 28 Jan 2014 17:36:22 -0800 Subject: [PATCH 2/8] timer: don't reschedule timer bucket in a domain If two timers run on the same tick, and the first timer uses a domain, and then catches an exception and disposes of the domain, then the second timer never runs. (And even if the first timer does not dispose of the domain, the second timer could run under the wrong domain.) This happens because timer.js uses "process.nextTick()" to schedule continued processing of the timers for that tick. However, there was an exception inside a domain, then "process.nextTick()" runs under the domain of the first timer function, and will do nothing if the domain has been disposed. To avoid this, we temporarily save the value of "process.domain" before calling nextTick so that it does not run inside any domain. --- lib/timers.js | 6 ++ test/simple/test-domain-exit-dispose-again.js | 76 +++++++++++++++++++ 2 files changed, 82 insertions(+) create mode 100644 test/simple/test-domain-exit-dispose-again.js diff --git a/lib/timers.js b/lib/timers.js index 076503e6017..db4f5bd972f 100644 --- a/lib/timers.js +++ b/lib/timers.js @@ -113,9 +113,15 @@ function listOnTimeout() { threw = false; } finally { if (threw) { + // We need to continue processing after domain error handling + // is complete, but not by using whatever domain was left over + // when the timeout threw its exception. + var oldDomain = process.domain; + process.domain = null; process.nextTick(function() { list.ontimeout(); }); + process.domain = oldDomain; } } } diff --git a/test/simple/test-domain-exit-dispose-again.js b/test/simple/test-domain-exit-dispose-again.js new file mode 100644 index 00000000000..22928f26045 --- /dev/null +++ b/test/simple/test-domain-exit-dispose-again.js @@ -0,0 +1,76 @@ +// Copyright Joyent, Inc. and other Node contributors. +// +// Permission is hereby granted, free of charge, to any person obtaining a +// copy of this software and associated documentation files (the +// "Software"), to deal in the Software without restriction, including +// without limitation the rights to use, copy, modify, merge, publish, +// distribute, sublicense, and/or sell copies of the Software, and to permit +// persons to whom the Software is furnished to do so, subject to the +// following conditions: +// +// The above copyright notice and this permission notice shall be included +// in all copies or substantial portions of the Software. +// +// THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS +// OR IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF +// MERCHANTABILITY, FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT. IN +// NO EVENT SHALL THE AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, +// DAMAGES OR OTHER LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR +// OTHERWISE, ARISING FROM, OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE +// USE OR OTHER DEALINGS IN THE SOFTWARE. + +var common = require('../common.js'); +var assert = require('assert'); +var domain = require('domain'); +var disposalFailed = false; + +// no matter what happens, we should increment a 10 times. +var a = 0; +log(); +function log(){ + console.log(a++, process.domain); + if (a < 10) setTimeout(log, 20); +} + +var secondTimerRan = false; + +// in 50ms we'll throw an error. +setTimeout(err, 50); +setTimeout(secondTimer, 50); +function err(){ + var d = domain.create(); + d.on('error', handle); + d.run(err2); + + function err2() { + // this timeout should never be called, since the domain gets + // disposed when the error happens. + setTimeout(function() { + console.error('This should not happen.'); + disposalFailed = true; + process.exit(1); + }); + + // this function doesn't exist, and throws an error as a result. + err3(); + } + + function handle(e) { + // this should clean up everything properly. + d.dispose(); + console.error(e); + console.error('in handler', process.domain, process.domain === d); + } +} + +function secondTimer() { + console.log('In second timer'); + secondTimerRan = true; +} + +process.on('exit', function() { + assert.equal(a, 10); + assert.equal(disposalFailed, false); + assert(secondTimerRan); + console.log('ok'); +}); From bd8a5755dceda415eee147bc06574f5a30abb0d0 Mon Sep 17 00:00:00 2001 From: Ben Noordhuis Date: Tue, 4 Mar 2014 14:10:05 +0100 Subject: [PATCH 3/8] src: add default visibility to NODE_MODULE It's currently not really possible to compile native add-ons with -fvisibility=hidden because that also hides the struct containing the module definition. The NODE_MODULE() and NODE_MODULE_DECL() macros are structured in a way that makes it impossible to add a visibility attribute manually so there is no escape hatch there. That's why this commit adds an explicit visibility attribute to the module definition. It doesn't help with node.js releases that are already out there but at least it improves the situation going forward. --- src/node.h | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/node.h b/src/node.h index b2c8fc4c20f..199aadfacaf 100644 --- a/src/node.h +++ b/src/node.h @@ -217,7 +217,7 @@ node_module_struct* get_builtin_module(const char *name); #ifdef _WIN32 # define NODE_MODULE_EXPORT __declspec(dllexport) #else -# define NODE_MODULE_EXPORT /* empty */ +# define NODE_MODULE_EXPORT __attribute__((visibility("default"))) #endif #define NODE_MODULE(modname, regfunc) \ From a9d24fa40d2e02860efa7653164c0c4e0d515457 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Benoit=20Vall=C3=A9e?= Date: Tue, 14 May 2013 11:10:07 +0800 Subject: [PATCH 4/8] test: test sending a handle twice Added test-cluster-send-handle-twice.js testing to send a handle twice to the parent process. --- test/simple/test-cluster-send-handle-twice.js | 56 +++++++++++++++++++ 1 file changed, 56 insertions(+) create mode 100644 test/simple/test-cluster-send-handle-twice.js diff --git a/test/simple/test-cluster-send-handle-twice.js b/test/simple/test-cluster-send-handle-twice.js new file mode 100644 index 00000000000..3215eae79ed --- /dev/null +++ b/test/simple/test-cluster-send-handle-twice.js @@ -0,0 +1,56 @@ +// Copyright Joyent, Inc. and other Node contributors. +// +// Permission is hereby granted, free of charge, to any person obtaining a +// copy of this software and associated documentation files (the +// "Software"), to deal in the Software without restriction, including +// without limitation the rights to use, copy, modify, merge, publish, +// distribute, sublicense, and/or sell copies of the Software, and to permit +// persons to whom the Software is furnished to do so, subject to the +// following conditions: +// +// The above copyright notice and this permission notice shall be included +// in all copies or substantial portions of the Software. +// +// THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS +// OR IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF +// MERCHANTABILITY, FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT. IN +// NO EVENT SHALL THE AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, +// DAMAGES OR OTHER LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR +// OTHERWISE, ARISING FROM, OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE +// USE OR OTHER DEALINGS IN THE SOFTWARE. + +// Testing to send an handle twice to the parent process. + +var common = require('../common'); +var assert = require('assert'); +var cluster = require('cluster'); +var net = require('net'); + +var workers = { + toStart: 1 +}; + +if (cluster.isMaster) { + for (var i = 0; i < workers.toStart; ++i) { + var worker = cluster.fork(); + worker.on('exit', function(code, signal) { + assert.equal(code, 0, 'Worker exited with an error code'); + assert(!signal, 'Worker exited by a signal'); + }); + } +} else { + var server = net.createServer(function(socket) { + process.send('send-handle-1', socket); + process.send('send-handle-2', socket); + }); + + server.listen(common.PORT, function() { + var client = net.connect({ host: 'localhost', port: common.PORT }); + client.on('close', function() { cluster.worker.disconnect(); }); + setTimeout(function() { client.end(); }, 50); + }).on('error', function(e) { + console.error(e); + assert(false, 'server.listen failed'); + cluster.worker.disconnect(); + }); +} From 5e06ce4fb949ab9bd5a7105dc0e8d924fd911d0c Mon Sep 17 00:00:00 2001 From: Fedor Indutny Date: Wed, 26 Feb 2014 14:37:13 +0400 Subject: [PATCH 5/8] child_process: fix sending handle twice When sending a socket to a child process via IPC pipe, `child_process.js` picks a raw UV handle from `_handle` property, sends it, and assigns `null` to the property. Sending the same socket twice was resulting in a runtime error, since we weren't handling the empty `_handle` case. In case of `null` `_handle` we should send just a plain text message as passed it was passed to `.send()` and ignore the handle, letting users handle such cases themselves instead of throwing the error at runtime. fix #5469 --- lib/child_process.js | 11 ++++++++++- 1 file changed, 10 insertions(+), 1 deletion(-) diff --git a/lib/child_process.js b/lib/child_process.js index eec0313337c..e93e5d47c8f 100644 --- a/lib/child_process.js +++ b/lib/child_process.js @@ -111,6 +111,9 @@ var handleConversion = { 'net.Socket': { send: function(message, socket) { + if (!socket._handle) + return; + // if the socket was created by net.Server if (socket.server) { // the slave should keep track of the socket @@ -139,7 +142,8 @@ var handleConversion = { postSend: function(handle) { // Close the Socket handle after sending it - handle.close(); + if (handle) + handle.close(); }, got: function(message, handle, emit) { @@ -438,6 +442,11 @@ function setupChannel(target, channel) { // convert TCP object to native handle object handle = handleConversion[message.type].send.apply(target, arguments); + // If handle was sent twice, or it is impossible to get native handle + // out of it - just send a text without the handle. + if (!handle) + message = message.msg; + // Update simultaneous accepts on Windows if (obj.simultaneousAccepts) { net._setSimultaneousAccepts(handle); From 6bd78fd7704a8a695fc76430b573bc482f42c320 Mon Sep 17 00:00:00 2001 From: Ben Noordhuis Date: Thu, 6 Mar 2014 22:59:56 +0100 Subject: [PATCH 6/8] deps: fix v8 valgrind warning Fix the following valgrind warning: Conditional jump or move depends on uninitialised value(s) at 0x7D64E7: v8::internal::GlobalHandles::IterateAllRootsWithClassIds(v8::internal::ObjectVisitor*) (global-handles.cc:613) by 0x94DCDC: v8::internal::NativeObjectsExplorer::FillRetainedObjects() (profile-generator.cc:2849) # etc. This was fixed upstream in r12903 and released in 3.15.2 but that commit was never back-ported to the 3.14 branch that node.js v0.10 uses. The code itself works okay; this commit simply shuffles the clauses in an `if` statement to check that the node is in use before checking its class id (which is uninitialized if the node is not in use.) --- deps/v8/src/global-handles.cc | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/deps/v8/src/global-handles.cc b/deps/v8/src/global-handles.cc index c09ba4b476b..faff357f9cb 100644 --- a/deps/v8/src/global-handles.cc +++ b/deps/v8/src/global-handles.cc @@ -610,7 +610,7 @@ void GlobalHandles::IterateAllRoots(ObjectVisitor* v) { void GlobalHandles::IterateAllRootsWithClassIds(ObjectVisitor* v) { for (NodeIterator it(this); !it.done(); it.Advance()) { - if (it.node()->has_wrapper_class_id() && it.node()->IsRetainer()) { + if (it.node()->IsRetainer() && it.node()->has_wrapper_class_id()) { v->VisitEmbedderReference(it.node()->location(), it.node()->wrapper_class_id()); } From f0d870501ed240f550b2be6b0a138312fb6f4a30 Mon Sep 17 00:00:00 2001 From: Fedor Indutny Date: Mon, 10 Mar 2014 14:59:18 +0400 Subject: [PATCH 7/8] crypto: do not lowercase cipher/hash names `crypto.getCiphers()` and `crypto.getHashes()` should prefer lower-case variants of names, but should not introduce them. fix #7282 --- lib/crypto.js | 11 ++++++++--- test/simple/test-crypto.js | 2 ++ 2 files changed, 10 insertions(+), 3 deletions(-) diff --git a/lib/crypto.js b/lib/crypto.js index 22141ff8ae9..d1c9eb5d2ec 100644 --- a/lib/crypto.js +++ b/lib/crypto.js @@ -608,8 +608,13 @@ function filterDuplicates(names) { // for example, 'sha1' instead of 'SHA1'. var ctx = {}; names.forEach(function(name) { - if (/^[0-9A-Z\-]+$/.test(name)) name = name.toLowerCase(); - ctx[name] = true; + var key = name; + if (/^[0-9A-Z\-]+$/.test(key)) key = key.toLowerCase(); + if (!ctx.hasOwnProperty(key) || ctx[key] < name) + ctx[key] = name; }); - return Object.getOwnPropertyNames(ctx).sort(); + + return Object.getOwnPropertyNames(ctx).map(function(key) { + return ctx[key]; + }).sort(); } diff --git a/test/simple/test-crypto.js b/test/simple/test-crypto.js index 96a269f4d04..25bb359190e 100644 --- a/test/simple/test-crypto.js +++ b/test/simple/test-crypto.js @@ -867,6 +867,8 @@ assert.notEqual(-1, crypto.getHashes().indexOf('sha1')); assert.notEqual(-1, crypto.getHashes().indexOf('sha')); assert.equal(-1, crypto.getHashes().indexOf('SHA1')); assert.equal(-1, crypto.getHashes().indexOf('SHA')); +assert.notEqual(-1, crypto.getHashes().indexOf('RSA-SHA1')); +assert.equal(-1, crypto.getHashes().indexOf('rsa-sha1')); assertSorted(crypto.getHashes()); // Base64 padding regression test, see #4837. From 43a29f53ca11ab48e1d1ef6b2a0e673ae43a42a0 Mon Sep 17 00:00:00 2001 From: Shuhei Kagawa Date: Sun, 9 Mar 2014 20:16:39 +0900 Subject: [PATCH 8/8] doc: remove an unused arg in process.stdin. The argument of process.stdin's readable event handler is not used. --- doc/api/process.markdown | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/doc/api/process.markdown b/doc/api/process.markdown index dafcd23bddc..68f15c2fb18 100644 --- a/doc/api/process.markdown +++ b/doc/api/process.markdown @@ -170,7 +170,7 @@ Example of opening standard input and listening for both events: process.stdin.setEncoding('utf8'); - process.stdin.on('readable', function(chunk) { + process.stdin.on('readable', function() { var chunk = process.stdin.read(); if (chunk !== null) { process.stdout.write('data: ' + chunk);