From 0cd6f41fb50299c15f4a2eae4cacd01a38407015 Mon Sep 17 00:00:00 2001 From: "yoav@wix.com" Date: Tue, 5 May 2015 13:20:12 +0300 Subject: [PATCH 1/4] fixed a bug in async listener - in case of nested callbacks and an error, it pops the listenerStack only once, casing a leak of listeners into scopes they should not take effect on --- glue.js | 13 +++- ...porting-domain-after-async-listener.tap.js | 74 +++++++++++++++++++ ...orting-domain-before-async-listener.tap.js | 74 +++++++++++++++++++ 3 files changed, 160 insertions(+), 1 deletion(-) create mode 100644 test/double-calls-to-create-if-importing-domain-after-async-listener.tap.js create mode 100644 test/double-calls-to-create-not-happening-if-importing-domain-before-async-listener.tap.js diff --git a/glue.js b/glue.js index d37db89..ba5ba71 100644 --- a/glue.js +++ b/glue.js @@ -53,6 +53,12 @@ var asyncCatcher; */ var asyncWrap; +/** + * Counter for the number of nested wrappings in the same tick, used when poping from listenerStack to pop the right + * number of frames + */ +var nestedAsyncWraps = 0; + /** * Simple helper function that's probably faster than using Array * filter methods and can be inlined. @@ -128,7 +134,10 @@ if (process._fatalException) { * synchronous throws when the listener is active, there may have been * none pushed yet. */ - if (listenerStack.length > 0) listeners = listenerStack.pop(); + while (nestedAsyncWraps > 0) { + if (listenerStack.length > 0) listeners = listenerStack.pop(); + nestedAsyncWraps--; + } errorValues = undefined; return handled && !inAsyncTick; @@ -167,6 +176,7 @@ if (process._fatalException) { * current listeners on a stack. */ listenerStack.push(listeners); + nestedAsyncWraps++; /* Activate both the listeners that were active when the closure was * created and the listeners that were previously active. @@ -200,6 +210,7 @@ if (process._fatalException) { // back to the previous listener list on the stack listeners = listenerStack.pop(); + nestedAsyncWraps--; errorValues = undefined; return returned; diff --git a/test/double-calls-to-create-if-importing-domain-after-async-listener.tap.js b/test/double-calls-to-create-if-importing-domain-after-async-listener.tap.js new file mode 100644 index 0000000..744fc59 --- /dev/null +++ b/test/double-calls-to-create-if-importing-domain-after-async-listener.tap.js @@ -0,0 +1,74 @@ +var test = require('tap').test; +if (!process.addAsyncListener) require('../index.js'); +var domain = require('domain'); + +/** + * if we import domains after async-listener, the domain import causes process.nextTick to call async listener twice. + * The reason is because domain replaces the underlying _currentTickHandler. If domain is loaded after async listener, + * the _currentTickHandler method is replaced with a wrapped method (wrapped by async listener). + * + * The net result is that + * if we load async-listener, then domain, we will have both process.nextTick and _currentTickHandler wrapped. + * + * if we load domain, then async-listener, we will have only process.nextTick wrapped. + * + * This double wrapping causes another issue - if we have an exception in the async method, asycn-listener does not expect a method + * to be wrapped twice and will only pop the listenerStack once, making calls after that that should not respond to the listener do be effected. + * + * **/ +test("asyncListeners with domains", function (t) { + t.plan(2); + + t.test("illustration of the problem - create is called twice", function(t) { + t.plan(1); + var callsToCreate = 0; + var listener = process.createAsyncListener( + { + create : function () { callsToCreate++; }, + before : function () {}, + after : function () {}, + error : function () {} + } + ); + + process.addAsyncListener(listener); + process.nextTick(function Func() { + t.equal(callsToCreate, 2, "number of calls to create"); + }); + process.removeAsyncListener(listener); + + + }); + + t.test("validate that async listener create is not called for a tick executed after a listened on tick with an error", function(t) { + t.plan(2); + var asyncListenedMethodDone = false; + var callsToCreate = 0; + var listener = process.createAsyncListener( + { + create : function () { callsToCreate++; }, + before : function () {}, + after : function () {}, + error : function () {return true;} + } + ); + + process.addAsyncListener(listener); + process.nextTick(function Func() { + asyncListenedMethodDone = true; + throw new Error("Bla"); + }); + process.removeAsyncListener(listener); + + // wait until the error was raised and then perform async operation + var timer = setInterval(function() { + if (asyncListenedMethodDone) { + t.equal(callsToCreate, 2, "number of calls to create - before the async method"); + clearInterval(timer); + process.nextTick(function() { + t.equal(callsToCreate, 2, "number of calls to create - in the async method (who should not be listened on by async the registered listener"); + }) + } + }, 1); + }) +}); diff --git a/test/double-calls-to-create-not-happening-if-importing-domain-before-async-listener.tap.js b/test/double-calls-to-create-not-happening-if-importing-domain-before-async-listener.tap.js new file mode 100644 index 0000000..c66fcee --- /dev/null +++ b/test/double-calls-to-create-not-happening-if-importing-domain-before-async-listener.tap.js @@ -0,0 +1,74 @@ +var domain = require('domain'); +var test = require('tap').test; +if (!process.addAsyncListener) require('../index.js'); + +/** + * if we import domains after async-listener, the domain import causes process.nextTick to call async listener twice. + * The reason is because domain replaces the underlying _currentTickHandler. If domain is loaded after async listener, + * the _currentTickHandler method is replaced with a wrapped method (wrapped by async listener). + * + * The net result is that + * if we load async-listener, then domain, we will have both process.nextTick and _currentTickHandler wrapped. + * + * if we load domain, then async-listener, we will have only process.nextTick wrapped. + * + * This double wrapping causes another issue - if we have an exception in the async method, asycn-listener does not expect a method + * to be wrapped twice and will only pop the listenerStack once, making calls after that that should not respond to the listener do be effected. + * + * **/ +test("asyncListeners with domains", function (t) { + t.plan(2); + + t.test("illustration of the problem - create is called twice", function(t) { + t.plan(1); + var callsToCreate = 0; + var listener = process.createAsyncListener( + { + create : function () { callsToCreate++; }, + before : function () {}, + after : function () {}, + error : function () {} + } + ); + + process.addAsyncListener(listener); + process.nextTick(function Func() { + t.equal(callsToCreate, 1, "number of calls to create"); + }); + process.removeAsyncListener(listener); + + + }); + + t.test("validate that async listener create is not called for a tick executed after a listened on tick with an error", function(t) { + t.plan(2); + var asyncListenedMethodDone = false; + var callsToCreate = 0; + var listener = process.createAsyncListener( + { + create : function () { callsToCreate++; }, + before : function () {}, + after : function () {}, + error : function () {return true;} + } + ); + + process.addAsyncListener(listener); + process.nextTick(function Func() { + asyncListenedMethodDone = true; + throw new Error("Bla"); + }); + process.removeAsyncListener(listener); + + // wait until the error was raised and then perform async operation + var timer = setInterval(function() { + if (asyncListenedMethodDone) { + t.equal(callsToCreate, 1, "number of calls to create - before the async method"); + clearInterval(timer); + process.nextTick(function() { + t.equal(callsToCreate, 1, "number of calls to create - in the async method (who should not be listened on by async the registered listener"); + }) + } + }, 1); + }) +}); From 4d6c0eb5c166e3d2f0f76e44538663aa5dc5d2d2 Mon Sep 17 00:00:00 2001 From: "yoav@wix.com" Date: Tue, 5 May 2015 13:20:12 +0300 Subject: [PATCH 2/4] fixed a bug in async listener - in case of nested callbacks and an error, it pops the listenerStack only once, casing a leak of listeners into scopes they should not take effect on --- glue.js | 13 +++- ...porting-domain-after-async-listener.tap.js | 74 +++++++++++++++++++ ...orting-domain-before-async-listener.tap.js | 74 +++++++++++++++++++ 3 files changed, 160 insertions(+), 1 deletion(-) create mode 100644 test/double-calls-to-create-if-importing-domain-after-async-listener.tap.js create mode 100644 test/double-calls-to-create-not-happening-if-importing-domain-before-async-listener.tap.js diff --git a/glue.js b/glue.js index d37db89..ba5ba71 100644 --- a/glue.js +++ b/glue.js @@ -53,6 +53,12 @@ var asyncCatcher; */ var asyncWrap; +/** + * Counter for the number of nested wrappings in the same tick, used when poping from listenerStack to pop the right + * number of frames + */ +var nestedAsyncWraps = 0; + /** * Simple helper function that's probably faster than using Array * filter methods and can be inlined. @@ -128,7 +134,10 @@ if (process._fatalException) { * synchronous throws when the listener is active, there may have been * none pushed yet. */ - if (listenerStack.length > 0) listeners = listenerStack.pop(); + while (nestedAsyncWraps > 0) { + if (listenerStack.length > 0) listeners = listenerStack.pop(); + nestedAsyncWraps--; + } errorValues = undefined; return handled && !inAsyncTick; @@ -167,6 +176,7 @@ if (process._fatalException) { * current listeners on a stack. */ listenerStack.push(listeners); + nestedAsyncWraps++; /* Activate both the listeners that were active when the closure was * created and the listeners that were previously active. @@ -200,6 +210,7 @@ if (process._fatalException) { // back to the previous listener list on the stack listeners = listenerStack.pop(); + nestedAsyncWraps--; errorValues = undefined; return returned; diff --git a/test/double-calls-to-create-if-importing-domain-after-async-listener.tap.js b/test/double-calls-to-create-if-importing-domain-after-async-listener.tap.js new file mode 100644 index 0000000..744fc59 --- /dev/null +++ b/test/double-calls-to-create-if-importing-domain-after-async-listener.tap.js @@ -0,0 +1,74 @@ +var test = require('tap').test; +if (!process.addAsyncListener) require('../index.js'); +var domain = require('domain'); + +/** + * if we import domains after async-listener, the domain import causes process.nextTick to call async listener twice. + * The reason is because domain replaces the underlying _currentTickHandler. If domain is loaded after async listener, + * the _currentTickHandler method is replaced with a wrapped method (wrapped by async listener). + * + * The net result is that + * if we load async-listener, then domain, we will have both process.nextTick and _currentTickHandler wrapped. + * + * if we load domain, then async-listener, we will have only process.nextTick wrapped. + * + * This double wrapping causes another issue - if we have an exception in the async method, asycn-listener does not expect a method + * to be wrapped twice and will only pop the listenerStack once, making calls after that that should not respond to the listener do be effected. + * + * **/ +test("asyncListeners with domains", function (t) { + t.plan(2); + + t.test("illustration of the problem - create is called twice", function(t) { + t.plan(1); + var callsToCreate = 0; + var listener = process.createAsyncListener( + { + create : function () { callsToCreate++; }, + before : function () {}, + after : function () {}, + error : function () {} + } + ); + + process.addAsyncListener(listener); + process.nextTick(function Func() { + t.equal(callsToCreate, 2, "number of calls to create"); + }); + process.removeAsyncListener(listener); + + + }); + + t.test("validate that async listener create is not called for a tick executed after a listened on tick with an error", function(t) { + t.plan(2); + var asyncListenedMethodDone = false; + var callsToCreate = 0; + var listener = process.createAsyncListener( + { + create : function () { callsToCreate++; }, + before : function () {}, + after : function () {}, + error : function () {return true;} + } + ); + + process.addAsyncListener(listener); + process.nextTick(function Func() { + asyncListenedMethodDone = true; + throw new Error("Bla"); + }); + process.removeAsyncListener(listener); + + // wait until the error was raised and then perform async operation + var timer = setInterval(function() { + if (asyncListenedMethodDone) { + t.equal(callsToCreate, 2, "number of calls to create - before the async method"); + clearInterval(timer); + process.nextTick(function() { + t.equal(callsToCreate, 2, "number of calls to create - in the async method (who should not be listened on by async the registered listener"); + }) + } + }, 1); + }) +}); diff --git a/test/double-calls-to-create-not-happening-if-importing-domain-before-async-listener.tap.js b/test/double-calls-to-create-not-happening-if-importing-domain-before-async-listener.tap.js new file mode 100644 index 0000000..c66fcee --- /dev/null +++ b/test/double-calls-to-create-not-happening-if-importing-domain-before-async-listener.tap.js @@ -0,0 +1,74 @@ +var domain = require('domain'); +var test = require('tap').test; +if (!process.addAsyncListener) require('../index.js'); + +/** + * if we import domains after async-listener, the domain import causes process.nextTick to call async listener twice. + * The reason is because domain replaces the underlying _currentTickHandler. If domain is loaded after async listener, + * the _currentTickHandler method is replaced with a wrapped method (wrapped by async listener). + * + * The net result is that + * if we load async-listener, then domain, we will have both process.nextTick and _currentTickHandler wrapped. + * + * if we load domain, then async-listener, we will have only process.nextTick wrapped. + * + * This double wrapping causes another issue - if we have an exception in the async method, asycn-listener does not expect a method + * to be wrapped twice and will only pop the listenerStack once, making calls after that that should not respond to the listener do be effected. + * + * **/ +test("asyncListeners with domains", function (t) { + t.plan(2); + + t.test("illustration of the problem - create is called twice", function(t) { + t.plan(1); + var callsToCreate = 0; + var listener = process.createAsyncListener( + { + create : function () { callsToCreate++; }, + before : function () {}, + after : function () {}, + error : function () {} + } + ); + + process.addAsyncListener(listener); + process.nextTick(function Func() { + t.equal(callsToCreate, 1, "number of calls to create"); + }); + process.removeAsyncListener(listener); + + + }); + + t.test("validate that async listener create is not called for a tick executed after a listened on tick with an error", function(t) { + t.plan(2); + var asyncListenedMethodDone = false; + var callsToCreate = 0; + var listener = process.createAsyncListener( + { + create : function () { callsToCreate++; }, + before : function () {}, + after : function () {}, + error : function () {return true;} + } + ); + + process.addAsyncListener(listener); + process.nextTick(function Func() { + asyncListenedMethodDone = true; + throw new Error("Bla"); + }); + process.removeAsyncListener(listener); + + // wait until the error was raised and then perform async operation + var timer = setInterval(function() { + if (asyncListenedMethodDone) { + t.equal(callsToCreate, 1, "number of calls to create - before the async method"); + clearInterval(timer); + process.nextTick(function() { + t.equal(callsToCreate, 1, "number of calls to create - in the async method (who should not be listened on by async the registered listener"); + }) + } + }, 1); + }) +}); From 2e063078063a95a8c0e87721ce7e49cd282a1792 Mon Sep 17 00:00:00 2001 From: "yoav@wix.com" Date: Wed, 12 Aug 2015 13:03:07 +0300 Subject: [PATCH 3/4] removed unneeded tests --- ...porting-domain-after-async-listener.tap.js | 74 ------------------- 1 file changed, 74 deletions(-) delete mode 100644 test/double-calls-to-create-if-importing-domain-after-async-listener.tap.js diff --git a/test/double-calls-to-create-if-importing-domain-after-async-listener.tap.js b/test/double-calls-to-create-if-importing-domain-after-async-listener.tap.js deleted file mode 100644 index 744fc59..0000000 --- a/test/double-calls-to-create-if-importing-domain-after-async-listener.tap.js +++ /dev/null @@ -1,74 +0,0 @@ -var test = require('tap').test; -if (!process.addAsyncListener) require('../index.js'); -var domain = require('domain'); - -/** - * if we import domains after async-listener, the domain import causes process.nextTick to call async listener twice. - * The reason is because domain replaces the underlying _currentTickHandler. If domain is loaded after async listener, - * the _currentTickHandler method is replaced with a wrapped method (wrapped by async listener). - * - * The net result is that - * if we load async-listener, then domain, we will have both process.nextTick and _currentTickHandler wrapped. - * - * if we load domain, then async-listener, we will have only process.nextTick wrapped. - * - * This double wrapping causes another issue - if we have an exception in the async method, asycn-listener does not expect a method - * to be wrapped twice and will only pop the listenerStack once, making calls after that that should not respond to the listener do be effected. - * - * **/ -test("asyncListeners with domains", function (t) { - t.plan(2); - - t.test("illustration of the problem - create is called twice", function(t) { - t.plan(1); - var callsToCreate = 0; - var listener = process.createAsyncListener( - { - create : function () { callsToCreate++; }, - before : function () {}, - after : function () {}, - error : function () {} - } - ); - - process.addAsyncListener(listener); - process.nextTick(function Func() { - t.equal(callsToCreate, 2, "number of calls to create"); - }); - process.removeAsyncListener(listener); - - - }); - - t.test("validate that async listener create is not called for a tick executed after a listened on tick with an error", function(t) { - t.plan(2); - var asyncListenedMethodDone = false; - var callsToCreate = 0; - var listener = process.createAsyncListener( - { - create : function () { callsToCreate++; }, - before : function () {}, - after : function () {}, - error : function () {return true;} - } - ); - - process.addAsyncListener(listener); - process.nextTick(function Func() { - asyncListenedMethodDone = true; - throw new Error("Bla"); - }); - process.removeAsyncListener(listener); - - // wait until the error was raised and then perform async operation - var timer = setInterval(function() { - if (asyncListenedMethodDone) { - t.equal(callsToCreate, 2, "number of calls to create - before the async method"); - clearInterval(timer); - process.nextTick(function() { - t.equal(callsToCreate, 2, "number of calls to create - in the async method (who should not be listened on by async the registered listener"); - }) - } - }, 1); - }) -}); From 6e4d72c0c45f16d8dfe118883dedf01b0e548738 Mon Sep 17 00:00:00 2001 From: "yoav@wix.com" Date: Wed, 12 Aug 2015 13:04:06 +0300 Subject: [PATCH 4/4] removed unneeded tests --- ...porting-domain-after-async-listener.tap.js | 74 ------------------- 1 file changed, 74 deletions(-) delete mode 100644 test/double-calls-to-create-if-importing-domain-after-async-listener.tap.js diff --git a/test/double-calls-to-create-if-importing-domain-after-async-listener.tap.js b/test/double-calls-to-create-if-importing-domain-after-async-listener.tap.js deleted file mode 100644 index 744fc59..0000000 --- a/test/double-calls-to-create-if-importing-domain-after-async-listener.tap.js +++ /dev/null @@ -1,74 +0,0 @@ -var test = require('tap').test; -if (!process.addAsyncListener) require('../index.js'); -var domain = require('domain'); - -/** - * if we import domains after async-listener, the domain import causes process.nextTick to call async listener twice. - * The reason is because domain replaces the underlying _currentTickHandler. If domain is loaded after async listener, - * the _currentTickHandler method is replaced with a wrapped method (wrapped by async listener). - * - * The net result is that - * if we load async-listener, then domain, we will have both process.nextTick and _currentTickHandler wrapped. - * - * if we load domain, then async-listener, we will have only process.nextTick wrapped. - * - * This double wrapping causes another issue - if we have an exception in the async method, asycn-listener does not expect a method - * to be wrapped twice and will only pop the listenerStack once, making calls after that that should not respond to the listener do be effected. - * - * **/ -test("asyncListeners with domains", function (t) { - t.plan(2); - - t.test("illustration of the problem - create is called twice", function(t) { - t.plan(1); - var callsToCreate = 0; - var listener = process.createAsyncListener( - { - create : function () { callsToCreate++; }, - before : function () {}, - after : function () {}, - error : function () {} - } - ); - - process.addAsyncListener(listener); - process.nextTick(function Func() { - t.equal(callsToCreate, 2, "number of calls to create"); - }); - process.removeAsyncListener(listener); - - - }); - - t.test("validate that async listener create is not called for a tick executed after a listened on tick with an error", function(t) { - t.plan(2); - var asyncListenedMethodDone = false; - var callsToCreate = 0; - var listener = process.createAsyncListener( - { - create : function () { callsToCreate++; }, - before : function () {}, - after : function () {}, - error : function () {return true;} - } - ); - - process.addAsyncListener(listener); - process.nextTick(function Func() { - asyncListenedMethodDone = true; - throw new Error("Bla"); - }); - process.removeAsyncListener(listener); - - // wait until the error was raised and then perform async operation - var timer = setInterval(function() { - if (asyncListenedMethodDone) { - t.equal(callsToCreate, 2, "number of calls to create - before the async method"); - clearInterval(timer); - process.nextTick(function() { - t.equal(callsToCreate, 2, "number of calls to create - in the async method (who should not be listened on by async the registered listener"); - }) - } - }, 1); - }) -});