From 46459ff1a7d689461f3f6acd6d7fa01bd099640b Mon Sep 17 00:00:00 2001 From: Alejandro Torrado Date: Thu, 11 Dec 2014 23:33:12 -0300 Subject: [PATCH 1/5] Moved cache queue before of the store get function --- lib/caching.js | 45 ++++++++++++++++++++++---------------------- lib/multi_caching.js | 45 +++++++++++++++++++++----------------------- 2 files changed, 43 insertions(+), 47 deletions(-) diff --git a/lib/caching.js b/lib/caching.js index a956e30..38319b5 100644 --- a/lib/caching.js +++ b/lib/caching.js @@ -40,38 +40,37 @@ var caching = function (args) { ttl = undefined; } + if (self.queues[key]) { + self.queues[key].push(cb); + return; + } + + self.queues[key] = [cb]; + + function fillCallbacks(err, data){ + self.queues[key].forEach(function(f){ + f(err, data); + }); + delete self.queues[key]; + } + self.store.get(key, function (err, result) { if (err && (!self.ignoreCacheErrors)) { - cb(err); + fillCallbacks(err); } else if (result) { - cb.call(cb, null, result); - } else if (self.queues[key]) { - self.queues[key].push(cb); + fillCallbacks(null, result); } else { - self.queues[key] = [cb]; - - work(function () { - var work_args = Array.prototype.slice.call(arguments, 0); - if (work_args[0]) { // assume first arg is an error - self.queues[key].forEach(function (done) { - done.call(null, work_args[0]); - }); - delete self.queues[key]; + work(function (err, data) { + if (err) { + fillCallbacks(err); return; } - // Subsequently assume second arg is result. - self.store.set(key, work_args[1], ttl, function (err) { + self.store.set(key, data, ttl, function (err) { if (err && (!self.ignoreCacheErrors)) { - self.queues[key].forEach(function (done) { - done.call(null, err); - }); + fillCallbacks(err); } else { - self.queues[key].forEach(function (done) { - done.apply(null, work_args); - }); + fillCallbacks(err, data); } - - delete self.queues[key]; }); }); } diff --git a/lib/multi_caching.js b/lib/multi_caching.js index 2dc52e7..9e9b328 100644 --- a/lib/multi_caching.js +++ b/lib/multi_caching.js @@ -47,9 +47,23 @@ var multi_caching = function (caches) { ttl = undefined; } + if (self.queues[key]) { + self.queues[key].push(cb); + return; + } + + self.queues[key] = [cb]; + + function fillCallbacks(err, data){ + self.queues[key].forEach(function(f){ + f(err, data); + }); + delete self.queues[key]; + } + get_from_highest_priority_cache(key, function (err, result, index) { if (err) { - return cb(err); + return fillCallbacks(err); } else if (result) { var caches_to_update = caches.slice(0, index); var opts = { @@ -58,38 +72,21 @@ var multi_caching = function (caches) { ttl: ttl }; set_in_multiple_caches(caches_to_update, opts, function (err) { - cb(err, result); + fillCallbacks(err, result); }); - } else if (self.queues[key]) { - self.queues[key].push(cb); } else { - self.queues[key] = [cb]; - work(function () { - var work_args = Array.prototype.slice.call(arguments, 0); - if (work_args[0]) { // assume first arg is an error - self.queues[key].forEach(function (done) { - done.call(null, work_args[0]); - }); - delete self.queues[key]; + work(function (err, data) { + if (err) { + fillCallbacks(err, data); return; } var opts = { key: key, - value: work_args[1], + value: data, ttl: ttl }; set_in_multiple_caches(caches, opts, function (err) { - if (err) { - self.queues[key].forEach(function (done) { - done.call(null, err); - }); - delete self.queues[key]; - return; - } - self.queues[key].forEach(function (done) { - done.apply(null, work_args); - }); - delete self.queues[key]; + fillCallbacks(null, data); }); }); } From 80f23186387433fca333a7321ceb048fce422667 Mon Sep 17 00:00:00 2001 From: Alejandro Torrado Date: Fri, 12 Dec 2014 00:03:17 -0300 Subject: [PATCH 2/5] Lint fixes --- lib/caching.js | 6 +++--- lib/multi_caching.js | 12 ++++++++---- 2 files changed, 11 insertions(+), 7 deletions(-) diff --git a/lib/caching.js b/lib/caching.js index 38319b5..ac8e372 100644 --- a/lib/caching.js +++ b/lib/caching.js @@ -47,8 +47,8 @@ var caching = function (args) { self.queues[key] = [cb]; - function fillCallbacks(err, data){ - self.queues[key].forEach(function(f){ + function fillCallbacks(err, data) { + self.queues[key].forEach(function(f) { f(err, data); }); delete self.queues[key]; @@ -69,7 +69,7 @@ var caching = function (args) { if (err && (!self.ignoreCacheErrors)) { fillCallbacks(err); } else { - fillCallbacks(err, data); + fillCallbacks(null, data); } }); }); diff --git a/lib/multi_caching.js b/lib/multi_caching.js index 9e9b328..fc83ac4 100644 --- a/lib/multi_caching.js +++ b/lib/multi_caching.js @@ -54,8 +54,8 @@ var multi_caching = function (caches) { self.queues[key] = [cb]; - function fillCallbacks(err, data){ - self.queues[key].forEach(function(f){ + function fillCallbacks(err, data) { + self.queues[key].forEach(function(f) { f(err, data); }); delete self.queues[key]; @@ -77,7 +77,7 @@ var multi_caching = function (caches) { } else { work(function (err, data) { if (err) { - fillCallbacks(err, data); + fillCallbacks(err, null); return; } var opts = { @@ -86,7 +86,11 @@ var multi_caching = function (caches) { ttl: ttl }; set_in_multiple_caches(caches, opts, function (err) { - fillCallbacks(null, data); + if (err) { + fillCallbacks(err); + } else { + fillCallbacks(null, data); + } }); }); } From 812c353a42af898618b90838d1a7d88668cdfb2c Mon Sep 17 00:00:00 2001 From: Alejandro Torrado Date: Thu, 18 Dec 2014 18:30:36 -0300 Subject: [PATCH 3/5] Capture work function exceptions and fill callbacks accordingly --- lib/caching.js | 15 +++++++++++---- lib/multi_caching.js | 20 +++++++++++--------- 2 files changed, 22 insertions(+), 13 deletions(-) diff --git a/lib/caching.js b/lib/caching.js index ac8e372..70d53e8 100644 --- a/lib/caching.js +++ b/lib/caching.js @@ -1,4 +1,6 @@ /*jshint maxcomplexity:15*/ +var domain = require('domain'); + var caching = function (args) { args = args || {}; var self = {}; @@ -47,8 +49,8 @@ var caching = function (args) { self.queues[key] = [cb]; - function fillCallbacks(err, data) { - self.queues[key].forEach(function(f) { + function fillCallbacks(err, data){ + self.queues[key].forEach(function(f){ f(err, data); }); delete self.queues[key]; @@ -60,7 +62,12 @@ var caching = function (args) { } else if (result) { fillCallbacks(null, result); } else { - work(function (err, data) { + domain + .create() + .on('error', function(err){ + fillCallbacks(err); + }) + .bind(work)(function (err, data) { if (err) { fillCallbacks(err); return; @@ -69,7 +76,7 @@ var caching = function (args) { if (err && (!self.ignoreCacheErrors)) { fillCallbacks(err); } else { - fillCallbacks(null, data); + fillCallbacks(err, data); } }); }); diff --git a/lib/multi_caching.js b/lib/multi_caching.js index fc83ac4..1e5a5bd 100644 --- a/lib/multi_caching.js +++ b/lib/multi_caching.js @@ -1,4 +1,5 @@ var async = require('async'); +var domain = require('domain'); /** * Module that lets you specify a hierarchy of caches. @@ -54,8 +55,8 @@ var multi_caching = function (caches) { self.queues[key] = [cb]; - function fillCallbacks(err, data) { - self.queues[key].forEach(function(f) { + function fillCallbacks(err, data){ + self.queues[key].forEach(function(f){ f(err, data); }); delete self.queues[key]; @@ -75,9 +76,14 @@ var multi_caching = function (caches) { fillCallbacks(err, result); }); } else { - work(function (err, data) { + domain + .create() + .on('error', function(err){ + fillCallbacks(err); + }) + .bind(work)(function (err, data) { if (err) { - fillCallbacks(err, null); + fillCallbacks(err, data); return; } var opts = { @@ -86,11 +92,7 @@ var multi_caching = function (caches) { ttl: ttl }; set_in_multiple_caches(caches, opts, function (err) { - if (err) { - fillCallbacks(err); - } else { - fillCallbacks(null, data); - } + fillCallbacks(null, data); }); }); } From 7bd7e9e6ed917ed9dc90aa4552b397e752c51ea1 Mon Sep 17 00:00:00 2001 From: Alejandro Torrado Date: Thu, 18 Dec 2014 19:26:38 -0300 Subject: [PATCH 4/5] Run wrap callback function in original domain --- lib/caching.js | 15 ++++++++------- lib/multi_caching.js | 21 +++++++++++++-------- 2 files changed, 21 insertions(+), 15 deletions(-) diff --git a/lib/caching.js b/lib/caching.js index 70d53e8..9a18101 100644 --- a/lib/caching.js +++ b/lib/caching.js @@ -43,15 +43,16 @@ var caching = function (args) { } if (self.queues[key]) { - self.queues[key].push(cb); + self.queues[key].push({cb: cb, domain: process.domain}); return; } - self.queues[key] = [cb]; + self.queues[key] = [{cb: cb, domain: process.domain}]; - function fillCallbacks(err, data){ - self.queues[key].forEach(function(f){ - f(err, data); + function fillCallbacks(err, data) { + self.queues[key].forEach(function(task) { + var taskDomain = task.domain || domain.create(); + taskDomain.bind(task.cb)(err, data); }); delete self.queues[key]; } @@ -64,7 +65,7 @@ var caching = function (args) { } else { domain .create() - .on('error', function(err){ + .on('error', function(err) { fillCallbacks(err); }) .bind(work)(function (err, data) { @@ -76,7 +77,7 @@ var caching = function (args) { if (err && (!self.ignoreCacheErrors)) { fillCallbacks(err); } else { - fillCallbacks(err, data); + fillCallbacks(null, data); } }); }); diff --git a/lib/multi_caching.js b/lib/multi_caching.js index 1e5a5bd..3c638bc 100644 --- a/lib/multi_caching.js +++ b/lib/multi_caching.js @@ -49,15 +49,16 @@ var multi_caching = function (caches) { } if (self.queues[key]) { - self.queues[key].push(cb); + self.queues[key].push({cb: cb, domain: process.domain}); return; } - self.queues[key] = [cb]; + self.queues[key] = [{cb: cb, domain: process.domain}]; - function fillCallbacks(err, data){ - self.queues[key].forEach(function(f){ - f(err, data); + function fillCallbacks(err, data) { + self.queues[key].forEach(function(task) { + var taskDomain = task.domain || domain.create(); + taskDomain.bind(task.cb)(err, data); }); delete self.queues[key]; } @@ -78,12 +79,12 @@ var multi_caching = function (caches) { } else { domain .create() - .on('error', function(err){ + .on('error', function(err) { fillCallbacks(err); }) .bind(work)(function (err, data) { if (err) { - fillCallbacks(err, data); + fillCallbacks(err); return; } var opts = { @@ -92,7 +93,11 @@ var multi_caching = function (caches) { ttl: ttl }; set_in_multiple_caches(caches, opts, function (err) { - fillCallbacks(null, data); + if (err) { + fillCallbacks(err); + } else { + fillCallbacks(null, data); + } }); }); } From a6cb2983b859392533b327180ab030ecc7000b94 Mon Sep 17 00:00:00 2001 From: Alejandro Torrado Date: Thu, 18 Dec 2014 20:53:33 -0300 Subject: [PATCH 5/5] Unit tests for thrown errors in the work function. --- test/caching.unit.js | 17 +++++++++++++++++ test/multi_caching.unit.js | 17 +++++++++++++++++ 2 files changed, 34 insertions(+) diff --git a/test/caching.unit.js b/test/caching.unit.js index 33837fd..de8a2ed 100644 --- a/test/caching.unit.js +++ b/test/caching.unit.js @@ -357,6 +357,23 @@ describe("caching", function () { }); }); + context("when an error is thrown in the work function", function () { + var fake_error; + + beforeEach(function() { + fake_error = new Error(support.random.string()); + }); + + it("bubbles up that error", function (done) { + cache.wrap(key, function () { + throw fake_error; + }, ttl, function (err) { + assert.equal(err, fake_error); + done(); + }); + }); + }); + context("when store.get() calls back with an error", function () { context("and ignoreCacheErrors is not set (default is false)", function () { it("bubbles up that error", function (done) { diff --git a/test/multi_caching.unit.js b/test/multi_caching.unit.js index 746c951..31688e0 100644 --- a/test/multi_caching.unit.js +++ b/test/multi_caching.unit.js @@ -449,6 +449,23 @@ describe("multi_caching", function () { memory_store.create.restore(); }); + context("when an error is thrown in the work function", function () { + var fake_error; + + beforeEach(function() { + fake_error = new Error(support.random.string()); + }); + + it("bubbles up that error", function (done) { + multi_cache.wrap(key, function () { + throw fake_error; + }, ttl, function (err) { + assert.equal(err, fake_error); + done(); + }); + }); + }); + context("when store.get() calls back with an error", function () { it("bubbles up that error", function (done) { var fake_error = new Error(support.random.string());