guard attempts against passing max_attempts, should fix #797

Automattic · Jan 14, 2016 · 0d5ef62 · 0d5ef62
1 parent c6d6a5c
commit 0d5ef62
Show file tree

Hide file tree

Showing 2 changed files with 28 additions and 27 deletions.
diff --git a/lib/queue/job.js b/lib/queue/job.js
@@ -188,8 +188,8 @@ exports.get = function( id, jobType, fn ) {
     job._delay            = hash.delay;
     job.priority(Number(hash.priority));
     job._progress         = hash.progress;
-    job._attempts         = hash.attempts;
-    job._max_attempts     = hash.max_attempts;
+    job._attempts         = Number(hash.attempts);
+    job._max_attempts     = Number(hash.max_attempts);
     job._state            = hash.state;
     job._error            = hash.error;
     job.created_at        = hash.created_at;
@@ -523,20 +523,19 @@ Job.prototype.priority = function( level ) {
  */
 
 Job.prototype.attempt = function( fn ) {
-  var self   = this
-    , client = this.client
+  var client = this.client
     , id     = this.id
     , key    = client.getKey('job:' + id);
 
-  //TODO this now can be removed, since max_attempts is set in the constructor
-  client.hsetnx(key, 'max_attempts', 1, function() {
-    client.hget(key, 'max_attempts', function( err, max ) {
-      client.hincrby(key, 'attempts', 1, function( err, attempts ) {
-        fn(err, Math.max(0, max - attempts), attempts, max);
-      });
-    });
-  });
-
+  this._attempts = this._attempts || 0;
+  if( this._attempts < this._max_attempts ) {
+    client.hincrby(key, 'attempts', 1, function( err, attempts ) {
+      this._attempts = attempts;
+      fn(err, Math.max(0, this._max_attempts - attempts), attempts, this._max_attempts);
+    }.bind(this));
+  } else {
+    fn(null, 0, this._attempts, this._max_attempts);
+  }
   return this;
 };
 
@@ -592,16 +591,18 @@ Job.prototype.failedAttempt = function( theErr, fn ) {
         this.emit( 'error', error );
         return fn && fn( error );
       }
-      if( remaining ) {
+      if( remaining > 0 ) {
         this.reattempt(attempts, function( err ) {
           if( err ) {
             this.emit( 'error', err );
             return fn && fn( err );
           }
           fn && fn( err, true, attempts );
         }.bind(this));
-      } else {
+      } else if( remaining === 0 )  {
         fn && fn( null, false, attempts );
+      } else {
+        fn && fn( new Error('Attempts Exceeded') );
       }
     }.bind(this));
   }.bind(this));

diff --git a/test/test.js b/test/test.js
@@ -14,8 +14,8 @@ describe('CONNECTION', function(){
 		  redis: 'redis://localhost:6379/15?foo=bar'
 	  } );
 
-	  jobs.client.connection_options.port.should.be.eql( 6379 );
-	  jobs.client.connection_options.host.should.be.eql( 'localhost' );
+	  jobs.client.options.port.should.be.eql( 6379 );
+	  jobs.client.options.host.should.be.eql( 'localhost' );
 	  jobs.client.options.foo.should.be.eql( 'bar' );
 
 	  var jobData = {
@@ -46,8 +46,8 @@ describe('CONNECTION', function(){
 			}
 		} );
 
-		jobs.client.connection_options.port.should.be.eql( 6379 );
-		jobs.client.connection_options.host.should.be.eql( 'localhost' );
+		jobs.client.options.port.should.be.eql( 6379 );
+		jobs.client.options.host.should.be.eql( 'localhost' );
 		jobs.client.options.foo.should.be.eql( 'bar' );
 
 		var jobData = {
@@ -71,8 +71,8 @@ describe('CONNECTION', function(){
 			redis: 'redis://localhost:6379/?foo=bar'
 		} );
 
-		jobs.client.connection_options.port.should.be.eql( 6379 );
-		jobs.client.connection_options.host.should.be.eql( 'localhost' );
+		jobs.client.options.port.should.be.eql( 6379 );
+		jobs.client.options.host.should.be.eql( 'localhost' );
 		jobs.client.options.foo.should.be.eql( 'bar' );
 
 		var jobData = {
@@ -96,8 +96,8 @@ describe('CONNECTION', function(){
 			redis: 'redis://localhost:6379?foo=bar'
 		} );
 
-		jobs.client.connection_options.port.should.be.eql( 6379 );
-		jobs.client.connection_options.host.should.be.eql( 'localhost' );
+		jobs.client.options.port.should.be.eql( 6379 );
+		jobs.client.options.host.should.be.eql( 'localhost' );
 		jobs.client.options.foo.should.be.eql( 'bar' );
 
 		var jobData = {
@@ -127,8 +127,8 @@ describe('CONNECTION', function(){
 			}
 		} );
 
-		jobs.client.connection_options.port.should.be.eql( 6379 );
-		jobs.client.connection_options.host.should.be.eql( 'localhost' );
+		jobs.client.options.port.should.be.eql( 6379 );
+		jobs.client.options.host.should.be.eql( 'localhost' );
 		jobs.client.options.foo.should.be.eql( 'bar' );
 
 		var jobData = {
@@ -206,8 +206,8 @@ describe( 'JOBS', function () {
       redis: 'redis://localhost:6379/?foo=bar'
     } );
 
-    jobs.client.connection_options.port.should.be.eql( 6379 );
-    jobs.client.connection_options.host.should.be.eql( 'localhost' );
+    jobs.client.options.port.should.be.eql( 6379 );
+    jobs.client.options.host.should.be.eql( 'localhost' );
     jobs.client.options.foo.should.be.eql( 'bar' );
 
     var jobData = {