From 372077172c2b7b39c15486c85a0051e3eb18426e Mon Sep 17 00:00:00 2001 From: Evan Tahler Date: Thu, 15 Oct 2015 18:34:06 -0700 Subject: [PATCH 1/3] immutable params --- lib/worker.js | 12 ++++++++++-- test/core/worker.js | 19 ++++++++++++++++++- 2 files changed, 28 insertions(+), 3 deletions(-) diff --git a/lib/worker.js b/lib/worker.js index bb0db359..94106649 100644 --- a/lib/worker.js +++ b/lib/worker.js @@ -169,7 +169,8 @@ worker.prototype.perform = function(job, callback) { }else{ args = [job.args]; } - cb.apply(self, [].slice.call(args).concat([function(err, result){ + + var combinedInputs = [].slice.call(args).concat([function(err, result){ returnCounter++; if(returnCounter !== 2){ self.emit('failure', self.queue, job, callbackError); @@ -185,7 +186,14 @@ worker.prototype.perform = function(job, callback) { } }); } - }])); + }]); + + // When returning the payload back to redis (on error), it is important to make it immutable + // https://github.com/taskrabbit/node-resque/issues/99 + // Note: if an input is a string or a number, you CANNOT freeze it saddly. + for(var i in combinedInputs){ Object.freeze(combinedInputs[i]); } + + cb.apply(self, combinedInputs); } }); }else{ diff --git a/test/core/worker.js b/test/core/worker.js index 6063b009..b4b3d0c4 100644 --- a/test/core/worker.js +++ b/test/core/worker.js @@ -19,6 +19,12 @@ describe('worker', function(){ callback(new Error("Blue Smoke")); } }, + "messWithData": { + perform: function(a, callback){ + a.data = 'new thing'; + callback(null, a); + } + }, "doubleCaller":{ perform: function(callback){ callback(null, 'a'); @@ -160,6 +166,17 @@ describe('worker', function(){ worker.start(); }); + it('job arguments are immutable', function(done){ + var listener = worker.on('success', function(q, job, result){ + result.a.should.equal('starting value'); + worker.removeAllListeners('success'); + done(); + }); + + queue.enqueue(specHelper.queue, "messWithData", {a: 'starting value'}); + worker.start(); + }); + it('can accept jobs that are simple functions', function(done){ var listener = worker.on('success', function(q, job, result){ result.should.equal("ok"); @@ -175,7 +192,7 @@ describe('worker', function(){ it('will not work jobs that are not defined', function(done){ var listener = worker.on('failure', function(q, job, failure){ q.should.equal(specHelper.queue); - String(failure).should.equal == "Error: No job defined for class 'somethingFake'"; + String(failure).should.equal("Error: No job defined for class 'somethingFake'"); worker.removeAllListeners('failure'); done(); From f3c516adfb82a3aa5322ddab10bd1b63536fafca Mon Sep 17 00:00:00 2001 From: Evan Tahler Date: Thu, 15 Oct 2015 18:37:29 -0700 Subject: [PATCH 2/3] more notes --- lib/worker.js | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/lib/worker.js b/lib/worker.js index 94106649..4fb2b89f 100644 --- a/lib/worker.js +++ b/lib/worker.js @@ -188,7 +188,8 @@ worker.prototype.perform = function(job, callback) { } }]); - // When returning the payload back to redis (on error), it is important to make it immutable + // When returning the payload back to redis (on error), it is important that the orignal payload is preserved + // To help with this, we can stry to make the inputs to the job immutible // https://github.com/taskrabbit/node-resque/issues/99 // Note: if an input is a string or a number, you CANNOT freeze it saddly. for(var i in combinedInputs){ Object.freeze(combinedInputs[i]); } From 001218edb101c213a96add0bf3cd18ee40056ee1 Mon Sep 17 00:00:00 2001 From: Evan Tahler Date: Thu, 15 Oct 2015 18:48:41 -0700 Subject: [PATCH 3/3] be more safer when trying to freeze objects --- lib/worker.js | 6 +++++- test/core/worker.js | 2 +- 2 files changed, 6 insertions(+), 2 deletions(-) diff --git a/lib/worker.js b/lib/worker.js index 4fb2b89f..4a58db8a 100644 --- a/lib/worker.js +++ b/lib/worker.js @@ -192,7 +192,11 @@ worker.prototype.perform = function(job, callback) { // To help with this, we can stry to make the inputs to the job immutible // https://github.com/taskrabbit/node-resque/issues/99 // Note: if an input is a string or a number, you CANNOT freeze it saddly. - for(var i in combinedInputs){ Object.freeze(combinedInputs[i]); } + for(var i in combinedInputs){ + if(typeof combinedInputs[i] === 'object'){ + Object.freeze(combinedInputs[i]); + } + } cb.apply(self, combinedInputs); } diff --git a/test/core/worker.js b/test/core/worker.js index b4b3d0c4..844b00e5 100644 --- a/test/core/worker.js +++ b/test/core/worker.js @@ -130,7 +130,7 @@ describe('worker', function(){ describe('integration', function(){ beforeEach(function(done){ - worker = new specHelper.NR.worker({connection: specHelper.connectionDetails, timeout: specHelper.timeout, queues: specHelper.queue}, jobs) + worker = new specHelper.NR.worker({connection: specHelper.connectionDetails, timeout: specHelper.timeout, queues: specHelper.queue}, jobs); worker.connect(done); });