Merge pull request #100 from taskrabbit/immutable_params

immutable params
actionhero · Oct 16, 2015 · 06e2473 · 06e2473
2 parents 4d5f979 + 001218e
commit 06e2473
Show file tree

Hide file tree

Showing 2 changed files with 34 additions and 4 deletions.
diff --git 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,19 @@ worker.prototype.perform = function(job, callback) {
                   }
                 });
               }
-            }]));
+            }]);
+
+            // 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){ 
+              if(typeof combinedInputs[i] === 'object'){
+                Object.freeze(combinedInputs[i]); 
+              }
+            }
+
+            cb.apply(self, combinedInputs);
           }
         });
       }else{

diff --git 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');
@@ -124,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);
       });
 
@@ -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();