fixed retry() and forever

[wizo06]

TLDR (short explanation)

Problem 1

retry() was not properly resetting.
this._originalTimeouts was being modified when calling this._timeouts.shift().

Solution 1

Before

this._timeouts = this._originalTimeouts;

After

this._timeouts = this._originalTimeouts.slice(0);

Problem 2

When forever: true and retries has been reached, it would keep retrying (expected behaviour because forever: true) however it would start from the beginning of this._cachedTimeouts again.
The expected behaviour is to keep retrying with the last interval in this._cachedTimeouts.

Solution 2

Before

this._timeouts = this._cachedTimeouts.slice(0);
timeout = this._timeouts.shift();

After

timeout = this._cachedTimeouts.slice(-1);

Long explanation

Problem 1

The way node-retry works is by creating an array with a length of retries, where retries is the max number of retries.
This array is stored in two variables, this._originalTimeouts and this._timeouts.
Each element in this array, is the interval (in milliseconds) in-between each retry/attempt.

For each retry/attempt, this._timeouts is .shift()'ed.

When retryOperation.reset() is called, this._timeouts is restored by referencing this._originalTimeouts.

RetryOperation.prototype.reset = function() {
  this._attempts = 1;
  this._timeouts = this._originalTimeouts;
}

Arrays in Javascript are mutable, which means this._timeouts = this._originalTimeouts; does not make a new copy of this._originalTimeouts into this._timeouts. Therefore, when this._timeouts is .shift()'ed, this._originalTimeouts also gets .shift()'ed, resulting in the loss of the intervals in this._originalTimeouts.

Solution 1

Make a new copy of this._originalTimeouts and then assign it to this._timeouts.

Before

RetryOperation.prototype.reset = function() {
  this._attempts = 1;
  this._timeouts = this._originalTimeouts;
}

After

RetryOperation.prototype.reset = function() {
  this._attempts = 1;
  this._timeouts = this._originalTimeouts.slice(); // <- .slice() returns a new copy
}

Reference

Read more about .shift() here
Read more about .slice() here
Read more about mutable here

Problem 2

The way node-retry works is by creating an array with a length of retries, where retries is the max number of retries.
This array is stored in this._timeouts.
Each element in this array, is the interval (in milliseconds) in-between each retry/attempt.

If forever: true, a copy of this._timeouts is made and assigned to this._cachedTimeouts.

When forever: true and retries has been reached, it would keep retrying (expected behaviour because forever: true) however it would start from the beginning of this._cachedTimeouts again.
The expected behaviour is to keep retrying with the last interval in this._cachedTimeouts.

An example of a current behaviour: suppose an array with the following intervals:

[ 1000, 2000, 4000, 8000 ]

Retries would behave like so:

Retry attempt 1: `1000`
Retry attempt 2: `2000`
Retry attempt 3: `4000`
Retry attempt 4: `8000`
Retry attempt 5: `1000`
Retry attempt 6: `2000`
...

An example of an expected behaviour: suppose the same array with the same intervals:

[ 1000, 2000, 4000, 8000 ]

Retries should behave like so:

Retry attempt 1: `1000`
Retry attempt 2: `2000`
Retry attempt 3: `4000`
Retry attempt 4: `8000`
Retry attempt 5: `8000`
Retry attempt 6: `8000`
...

Solution 2

Assign the last interval of this._cachedTimeouts to timeout.
timeout is used as delay argument for setTImeout.

Before

this._timeouts = this._cachedTimeouts.slice(0);
timeout = this._timeouts.shift();

After

timeout = this._cachedTimeouts.slice(-1);

[codecov]

Codecov Report

Merging #76 into master will decrease coverage by 0.06%.
The diff coverage is 100.00%.

@@            Coverage Diff             @@
##           master      #76      +/-   ##
==========================================
- Coverage   90.41%   90.34%   -0.07%     
==========================================
  Files           2        2              
  Lines         146      145       -1     
==========================================
- Hits          132      131       -1     
  Misses         14       14              

Impacted Files	Coverage Δ
lib/retry_operation.js	86.31% <100.00%> (-0.15%)	⬇️

---

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update b316bfc...a9a5515. Read the comment docs.

[tim-kos]

Nice work, thank you very much!

[bohdanbirdie]

@tim-kos
Hi,
is this planned to be merged?

[tim-kos]

Sounds good! Thanks for the detailed explanation!

[billyjs]

@tim-kos is there plans to publish a new version with this fix?

[tim-kos]

0.13.1 pushed with the patch!