tim-kos / node-retry

Abstraction for exponential and custom retry strategies for failed operations.
Other
1.22k stars 80 forks source link

fixed retryOperation.reset() #75

Closed wizo06 closed 4 years ago

wizo06 commented 4 years ago

The problem:

retryOperation.reset() is not resetting properly.

Context

According to the documentation, retryOperation.reset() is supposed to:

Resets the internal state of the operation object, 
so that you can call attempt() again as if this was a new operation object.

The way node-retry works is by creating an array with a length of n, where n is the 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. The n-th attempt is initialized in this._attempts = 1;.

For each retry/attempt, this._attempts is increased by 1, and this._timeouts is .shift()'ed.

When retryOperation.reset() is called, this._attempts is reverted back to 1, and 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.

The solution

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

codecov[bot] commented 4 years ago

Codecov Report

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

Impacted file tree graph

@@            Coverage Diff             @@
##           master      #75      +/-   ##
==========================================
- 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%) :arrow_down:

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...85f814b. Read the comment docs.