From cbc5b4f864f621fe6068a6455faca2c06418c90f Mon Sep 17 00:00:00 2001
From: Bojan Djurkovic <dbojan@gmail.com>
Date: Tue, 2 Aug 2016 12:55:26 -0300
Subject: [PATCH 1/4] add filter option to retry() and retryable() to allow for
 error filtering and control of retry flow. Resolves #1256.

---
 lib/retry.js            |  31 +++++++++++-
 mocha_test/retry.js     | 102 +++++++++++++++++++++++++++++++++++++++-
 mocha_test/retryable.js |  24 ++++++++++
 3 files changed, 154 insertions(+), 3 deletions(-)

diff --git a/lib/retry.js b/lib/retry.js
index 8163ad94b..a70051641 100644
--- a/lib/retry.js
+++ b/lib/retry.js
@@ -19,6 +19,10 @@ import constant from 'lodash/constant';
  * * `interval` - The time to wait between retries, in milliseconds.  The
  *   default is `0`. The interval may also be specified as a function of the
  *   retry count (see example).
+ * * `filter` - Synchronous function that is invoked on erroneous result with the
+ *   the error. If it returns `true` the retry attempts will continue, if the
+ *   function returns `false` the retry flow is aborting with the current
+ *   attempt's error and result being returned to the final callback.
  * * If `opts` is a number, the number specifies the number of times to retry,
  *   with the default interval of `0`.
  * @param {Function} task - A function which receives two arguments: (1) a
@@ -62,6 +66,16 @@ import constant from 'lodash/constant';
  *     // do something with the result
  * });
  *
+ * // try calling apiMethod only when error condition satisfies, all other
+ * // errors will abort the retry control flow and return to final callback
+ * async.retry({
+ *   filter: function(err) {
+ *     return err.message === 'Temporary error'; // only retry on a specific error
+ *   }
+ * }, apiMethod, function(err, result) {
+ *     // do something with the result
+ * });
+ *
  * // It can also be embedded within other control flow functions to retry
  * // individual methods that are not as reliable, like this:
  * async.auto({
@@ -70,6 +84,7 @@ import constant from 'lodash/constant';
  * }, function(err, results) {
  *     // do something with the results
  * });
+ *
  */
 export default function retry(opts, task, callback) {
     var DEFAULT_TIMES = 5;
@@ -87,6 +102,10 @@ export default function retry(opts, task, callback) {
             acc.intervalFunc = typeof t.interval === 'function' ?
                 t.interval :
                 constant(+t.interval || DEFAULT_INTERVAL);
+
+            if(typeof t.filter === 'function') {
+                acc.filter = t.filter;
+            }
         } else if (typeof t === 'number' || typeof t === 'string') {
             acc.times = +t || DEFAULT_TIMES;
         } else {
@@ -94,7 +113,6 @@ export default function retry(opts, task, callback) {
         }
     }
 
-
     if (arguments.length < 3 && typeof opts === 'function') {
         callback = task || noop;
         task = opts;
@@ -111,7 +129,16 @@ export default function retry(opts, task, callback) {
     function retryAttempt() {
         task(function(err) {
             if (err && attempt++ < options.times) {
-                setTimeout(retryAttempt, options.intervalFunc(attempt));
+                var proceed = true;
+                if(options.filter) {
+                    proceed = options.filter(err);
+                }
+
+                if(proceed) {
+                    setTimeout(retryAttempt, options.intervalFunc(attempt));
+                } else {
+                    callback.apply(null, arguments);
+                }
             } else {
                 callback.apply(null, arguments);
             }
diff --git a/mocha_test/retry.js b/mocha_test/retry.js
index 4d7b241cb..1cfbcab78 100644
--- a/mocha_test/retry.js
+++ b/mocha_test/retry.js
@@ -156,5 +156,105 @@ describe("retry", function () {
         async.retry(5, fn, function(err, result) {
             expect(result).to.be.eql({a: 1});
         });
-    })
+    });
+
+    it('retry when all attempts fail and error filter returns true',function(done) {
+        var times = 3;
+        var callCount = 0;
+        var error = 'ERROR';
+        var special = 'SPECIAL_ERROR';
+        var erroredResult = 'RESULT';
+        function fn(callback) {
+            callCount++;
+            callback(error + callCount, erroredResult + callCount);
+        }
+        function filter(err) {
+            return err && err !== special;
+        }
+        var options = {
+            times: times,
+            filter: filter
+        };
+        async.retry(options, fn, function(err, result){
+            assert.equal(callCount, 3, "did not retry the correct number of times");
+            assert.equal(err, error + times, "Incorrect error was returned");
+            assert.equal(result, erroredResult + times, "Incorrect result was returned");
+            done();
+        });
+    });
+
+    it('retry when some attempts fail and error filter returns false at some invokation',function(done) {
+        var callCount = 0;
+        var error = 'ERROR';
+        var special = 'SPECIAL_ERROR';
+        var erroredResult = 'RESULT';
+        function fn(callback) {
+            callCount++;
+            var err = callCount === 2 ? special : error + callCount;
+            callback(err, erroredResult + callCount);
+        }
+        function filter(err) {
+            return err && err === error + callCount; // just a different pattern
+        }
+        var options = {
+            filter: filter
+        };
+        async.retry(options, fn, function(err, result){
+            assert.equal(callCount, 2, "did not retry the correct number of times");
+            assert.equal(err, special, "Incorrect error was returned");
+            assert.equal(result, erroredResult + 2, "Incorrect result was returned");
+            done();
+        });
+    });
+
+    it('retry with interval when some attempts fail and error filter returns false at some invokation',function(done) {
+        var interval = 50;
+        var callCount = 0;
+        var error = 'ERROR';
+        var erroredResult = 'RESULT';
+        var special = 'SPECIAL_ERROR';
+        var specialCount = 3;
+        function fn(callback) {
+            callCount++;
+            var err = callCount === specialCount ? special : error + callCount;
+            callback(err, erroredResult + callCount);
+        }
+        function filter(err) {
+            return err && err !== special;
+        }
+        var start = new Date().getTime();
+        async.retry({ interval: interval, filter: filter }, fn, function(err, result){
+            var now = new Date().getTime();
+            var duration = now - start;
+            assert(duration >= (interval * (specialCount - 1)),  'did not include interval');
+            assert.equal(callCount, specialCount, "did not retry the correct number of times");
+            assert.equal(err, special, "Incorrect error was returned");
+            assert.equal(result, erroredResult + specialCount, "Incorrect result was returned");
+            done();
+        });
+    });
+
+    it('retry when first attempt succeeds and error filter should not be called',function(done) {
+        var callCount = 0;
+        var error = 'ERROR';
+        var erroredResult = 'RESULT';
+        var filterCalled = false;
+        function fn(callback) {
+            callCount++;
+            callback(null, erroredResult + callCount);
+        }
+        function filter(err) {
+            filterCalled = true;
+            return err && err === error;
+        }
+        var options = {
+            filter: filter
+        };
+        async.retry(options, fn, _.rest(function(args) {
+            assert.equal(callCount, 1, "did not retry the correct number of times");
+            expect(args).to.be.eql([null, erroredResult + callCount]);
+            assert.equal(filterCalled, false, "filter function was called");
+            done();
+        }));
+    });
 });
diff --git a/mocha_test/retryable.js b/mocha_test/retryable.js
index 7147269da..0a229f50d 100644
--- a/mocha_test/retryable.js
+++ b/mocha_test/retryable.js
@@ -21,6 +21,30 @@ describe('retryable', function () {
         }, 15);
     });
 
+    it('basics with filter function', function (done) {
+        var calls = 0;
+        var special = 'special';
+        var opts = {
+            filter: function(err) {
+                return err == special;
+            }
+        };
+        var retryableTask = async.retryable(opts, function (arg, cb) {
+            calls++;
+            expect(arg).to.equal(42);
+            cb(calls === 3 ? 'fail' : special);
+        });
+
+        retryableTask(42, function (err) {
+            expect(err).to.equal('fail');
+            expect(calls).to.equal(3);
+            done();
+        });
+
+        setTimeout(function () {
+        }, 15);
+    });
+
     it('should work as an embedded task', function(done) {
         var retryResult = 'RETRY';
         var fooResults;

From 0337ee0b2e582917cc15e6efc186e3c89e0a2cdb Mon Sep 17 00:00:00 2001
From: Bojan Djurkovic <dbojan@gmail.com>
Date: Wed, 3 Aug 2016 19:46:51 -0300
Subject: [PATCH 2/4] changed the error test function to continueOperation.
 improved comment documentation and fixed code based on PR feedback.

---
 lib/retry.js            | 22 +++++++++-------------
 mocha_test/retry.js     | 30 +++++++++++++++---------------
 mocha_test/retryable.js |  4 ++--
 3 files changed, 26 insertions(+), 30 deletions(-)

diff --git a/lib/retry.js b/lib/retry.js
index a70051641..d8b711b19 100644
--- a/lib/retry.js
+++ b/lib/retry.js
@@ -19,10 +19,11 @@ import constant from 'lodash/constant';
  * * `interval` - The time to wait between retries, in milliseconds.  The
  *   default is `0`. The interval may also be specified as a function of the
  *   retry count (see example).
- * * `filter` - Synchronous function that is invoked on erroneous result with the
- *   the error. If it returns `true` the retry attempts will continue, if the
- *   function returns `false` the retry flow is aborting with the current
- *   attempt's error and result being returned to the final callback.
+ * * `continueOperation` - An optional synchronous function that is invoked on
+ *   erroneous result with the the error. If it returns `true` the retry attempts
+ *   will continue, if the function returns `false` the retry flow is aborted
+ *   with the current attempt's error and result being returned to the final
+ *   callback. Invoked with (err).
  * * If `opts` is a number, the number specifies the number of times to retry,
  *   with the default interval of `0`.
  * @param {Function} task - A function which receives two arguments: (1) a
@@ -69,7 +70,7 @@ import constant from 'lodash/constant';
  * // try calling apiMethod only when error condition satisfies, all other
  * // errors will abort the retry control flow and return to final callback
  * async.retry({
- *   filter: function(err) {
+ *   continueOperation: function(err) {
  *     return err.message === 'Temporary error'; // only retry on a specific error
  *   }
  * }, apiMethod, function(err, result) {
@@ -103,9 +104,7 @@ export default function retry(opts, task, callback) {
                 t.interval :
                 constant(+t.interval || DEFAULT_INTERVAL);
 
-            if(typeof t.filter === 'function') {
-                acc.filter = t.filter;
-            }
+            acc.continueOperation = t.continueOperation;
         } else if (typeof t === 'number' || typeof t === 'string') {
             acc.times = +t || DEFAULT_TIMES;
         } else {
@@ -129,11 +128,8 @@ export default function retry(opts, task, callback) {
     function retryAttempt() {
         task(function(err) {
             if (err && attempt++ < options.times) {
-                var proceed = true;
-                if(options.filter) {
-                    proceed = options.filter(err);
-                }
-
+                var proceed = typeof options.continueOperation != 'function' ||
+                    options.continueOperation(err);
                 if(proceed) {
                     setTimeout(retryAttempt, options.intervalFunc(attempt));
                 } else {
diff --git a/mocha_test/retry.js b/mocha_test/retry.js
index 1cfbcab78..612b14457 100644
--- a/mocha_test/retry.js
+++ b/mocha_test/retry.js
@@ -158,7 +158,7 @@ describe("retry", function () {
         });
     });
 
-    it('retry when all attempts fail and error filter returns true',function(done) {
+    it('retry when all attempts fail and error continue test returns true',function(done) {
         var times = 3;
         var callCount = 0;
         var error = 'ERROR';
@@ -168,12 +168,12 @@ describe("retry", function () {
             callCount++;
             callback(error + callCount, erroredResult + callCount);
         }
-        function filter(err) {
+        function errorTest(err) {
             return err && err !== special;
         }
         var options = {
             times: times,
-            filter: filter
+            continueOperation: errorTest
         };
         async.retry(options, fn, function(err, result){
             assert.equal(callCount, 3, "did not retry the correct number of times");
@@ -183,7 +183,7 @@ describe("retry", function () {
         });
     });
 
-    it('retry when some attempts fail and error filter returns false at some invokation',function(done) {
+    it('retry when some attempts fail and error test returns false at some invokation',function(done) {
         var callCount = 0;
         var error = 'ERROR';
         var special = 'SPECIAL_ERROR';
@@ -193,11 +193,11 @@ describe("retry", function () {
             var err = callCount === 2 ? special : error + callCount;
             callback(err, erroredResult + callCount);
         }
-        function filter(err) {
+        function errorTest(err) {
             return err && err === error + callCount; // just a different pattern
         }
         var options = {
-            filter: filter
+            continueOperation: errorTest
         };
         async.retry(options, fn, function(err, result){
             assert.equal(callCount, 2, "did not retry the correct number of times");
@@ -207,7 +207,7 @@ describe("retry", function () {
         });
     });
 
-    it('retry with interval when some attempts fail and error filter returns false at some invokation',function(done) {
+    it('retry with interval when some attempts fail and error test returns false at some invokation',function(done) {
         var interval = 50;
         var callCount = 0;
         var error = 'ERROR';
@@ -219,11 +219,11 @@ describe("retry", function () {
             var err = callCount === specialCount ? special : error + callCount;
             callback(err, erroredResult + callCount);
         }
-        function filter(err) {
+        function errorTest(err) {
             return err && err !== special;
         }
         var start = new Date().getTime();
-        async.retry({ interval: interval, filter: filter }, fn, function(err, result){
+        async.retry({ interval: interval, continueOperation: errorTest }, fn, function(err, result){
             var now = new Date().getTime();
             var duration = now - start;
             assert(duration >= (interval * (specialCount - 1)),  'did not include interval');
@@ -234,26 +234,26 @@ describe("retry", function () {
         });
     });
 
-    it('retry when first attempt succeeds and error filter should not be called',function(done) {
+    it('retry when first attempt succeeds and error test should not be called',function(done) {
         var callCount = 0;
         var error = 'ERROR';
         var erroredResult = 'RESULT';
-        var filterCalled = false;
+        var continueTestCalled = false;
         function fn(callback) {
             callCount++;
             callback(null, erroredResult + callCount);
         }
-        function filter(err) {
-            filterCalled = true;
+        function errorTest(err) {
+            continueTestCalled = true;
             return err && err === error;
         }
         var options = {
-            filter: filter
+            continueOperation: errorTest
         };
         async.retry(options, fn, _.rest(function(args) {
             assert.equal(callCount, 1, "did not retry the correct number of times");
             expect(args).to.be.eql([null, erroredResult + callCount]);
-            assert.equal(filterCalled, false, "filter function was called");
+            assert.equal(continueTestCalled, false, "error test function was called");
             done();
         }));
     });
diff --git a/mocha_test/retryable.js b/mocha_test/retryable.js
index 0a229f50d..9053dd1c7 100644
--- a/mocha_test/retryable.js
+++ b/mocha_test/retryable.js
@@ -21,11 +21,11 @@ describe('retryable', function () {
         }, 15);
     });
 
-    it('basics with filter function', function (done) {
+    it('basics with error test function', function (done) {
         var calls = 0;
         var special = 'special';
         var opts = {
-            filter: function(err) {
+            continueOperation: function(err) {
                 return err == special;
             }
         };

From 0af976cc89096ee64db3bd08fe65e2b3445f56d5 Mon Sep 17 00:00:00 2001
From: Bojan Djurkovic <dbojan@gmail.com>
Date: Thu, 4 Aug 2016 21:00:56 -0300
Subject: [PATCH 3/4] rename the new retry option to errorFilter and
 consolidate retry attempt condition into one statement

---
 lib/retry.js            | 26 +++++++++++---------------
 mocha_test/retry.js     |  8 ++++----
 mocha_test/retryable.js |  2 +-
 3 files changed, 16 insertions(+), 20 deletions(-)

diff --git a/lib/retry.js b/lib/retry.js
index d8b711b19..45b09a83d 100644
--- a/lib/retry.js
+++ b/lib/retry.js
@@ -19,11 +19,11 @@ import constant from 'lodash/constant';
  * * `interval` - The time to wait between retries, in milliseconds.  The
  *   default is `0`. The interval may also be specified as a function of the
  *   retry count (see example).
- * * `continueOperation` - An optional synchronous function that is invoked on
- *   erroneous result with the the error. If it returns `true` the retry attempts
- *   will continue, if the function returns `false` the retry flow is aborted
- *   with the current attempt's error and result being returned to the final
- *   callback. Invoked with (err).
+ * * `errorFilter` - An optional synchronous function that is invoked on
+ *   erroneous result. If it returns `true` the retry attempts will continue;
+ *   if the function returns `false` the retry flow is aborted with the current
+ *   attempt's error and result being returned to the final callback.
+ *   Invoked with (err, result).
  * * If `opts` is a number, the number specifies the number of times to retry,
  *   with the default interval of `0`.
  * @param {Function} task - A function which receives two arguments: (1) a
@@ -70,7 +70,7 @@ import constant from 'lodash/constant';
  * // try calling apiMethod only when error condition satisfies, all other
  * // errors will abort the retry control flow and return to final callback
  * async.retry({
- *   continueOperation: function(err) {
+ *   errorFilter: function(err) {
  *     return err.message === 'Temporary error'; // only retry on a specific error
  *   }
  * }, apiMethod, function(err, result) {
@@ -104,7 +104,7 @@ export default function retry(opts, task, callback) {
                 t.interval :
                 constant(+t.interval || DEFAULT_INTERVAL);
 
-            acc.continueOperation = t.continueOperation;
+            acc.errorFilter = t.errorFilter;
         } else if (typeof t === 'number' || typeof t === 'string') {
             acc.times = +t || DEFAULT_TIMES;
         } else {
@@ -127,14 +127,10 @@ export default function retry(opts, task, callback) {
     var attempt = 1;
     function retryAttempt() {
         task(function(err) {
-            if (err && attempt++ < options.times) {
-                var proceed = typeof options.continueOperation != 'function' ||
-                    options.continueOperation(err);
-                if(proceed) {
-                    setTimeout(retryAttempt, options.intervalFunc(attempt));
-                } else {
-                    callback.apply(null, arguments);
-                }
+            if (err && attempt++ < options.times &&
+                (typeof options.errorFilter != 'function' ||
+                    options.errorFilter(err))) {
+                setTimeout(retryAttempt, options.intervalFunc(attempt));
             } else {
                 callback.apply(null, arguments);
             }
diff --git a/mocha_test/retry.js b/mocha_test/retry.js
index 612b14457..757b7ab95 100644
--- a/mocha_test/retry.js
+++ b/mocha_test/retry.js
@@ -173,7 +173,7 @@ describe("retry", function () {
         }
         var options = {
             times: times,
-            continueOperation: errorTest
+            errorFilter: errorTest
         };
         async.retry(options, fn, function(err, result){
             assert.equal(callCount, 3, "did not retry the correct number of times");
@@ -197,7 +197,7 @@ describe("retry", function () {
             return err && err === error + callCount; // just a different pattern
         }
         var options = {
-            continueOperation: errorTest
+            errorFilter: errorTest
         };
         async.retry(options, fn, function(err, result){
             assert.equal(callCount, 2, "did not retry the correct number of times");
@@ -223,7 +223,7 @@ describe("retry", function () {
             return err && err !== special;
         }
         var start = new Date().getTime();
-        async.retry({ interval: interval, continueOperation: errorTest }, fn, function(err, result){
+        async.retry({ interval: interval, errorFilter: errorTest }, fn, function(err, result){
             var now = new Date().getTime();
             var duration = now - start;
             assert(duration >= (interval * (specialCount - 1)),  'did not include interval');
@@ -248,7 +248,7 @@ describe("retry", function () {
             return err && err === error;
         }
         var options = {
-            continueOperation: errorTest
+            errorFilter: errorTest
         };
         async.retry(options, fn, _.rest(function(args) {
             assert.equal(callCount, 1, "did not retry the correct number of times");
diff --git a/mocha_test/retryable.js b/mocha_test/retryable.js
index 9053dd1c7..897f83eee 100644
--- a/mocha_test/retryable.js
+++ b/mocha_test/retryable.js
@@ -25,7 +25,7 @@ describe('retryable', function () {
         var calls = 0;
         var special = 'special';
         var opts = {
-            continueOperation: function(err) {
+            errorFilter: function(err) {
                 return err == special;
             }
         };

From 4367751bc420f4f279d83e8eae291708bf7990f6 Mon Sep 17 00:00:00 2001
From: Bojan Djurkovic <dbojan@gmail.com>
Date: Mon, 8 Aug 2016 08:58:49 -0300
Subject: [PATCH 4/4] fix errorFilter comment to reflect that we only pass err
 argument. Removed setTimeout from retryable tests

---
 lib/retry.js            | 2 +-
 mocha_test/retryable.js | 6 ------
 2 files changed, 1 insertion(+), 7 deletions(-)

diff --git a/lib/retry.js b/lib/retry.js
index 45b09a83d..a5ad866bf 100644
--- a/lib/retry.js
+++ b/lib/retry.js
@@ -23,7 +23,7 @@ import constant from 'lodash/constant';
  *   erroneous result. If it returns `true` the retry attempts will continue;
  *   if the function returns `false` the retry flow is aborted with the current
  *   attempt's error and result being returned to the final callback.
- *   Invoked with (err, result).
+ *   Invoked with (err).
  * * If `opts` is a number, the number specifies the number of times to retry,
  *   with the default interval of `0`.
  * @param {Function} task - A function which receives two arguments: (1) a
diff --git a/mocha_test/retryable.js b/mocha_test/retryable.js
index 897f83eee..48d9ee902 100644
--- a/mocha_test/retryable.js
+++ b/mocha_test/retryable.js
@@ -16,9 +16,6 @@ describe('retryable', function () {
             expect(calls).to.equal(3);
             done();
         });
-
-        setTimeout(function () {
-        }, 15);
     });
 
     it('basics with error test function', function (done) {
@@ -40,9 +37,6 @@ describe('retryable', function () {
             expect(calls).to.equal(3);
             done();
         });
-
-        setTimeout(function () {
-        }, 15);
     });
 
     it('should work as an embedded task', function(done) {