tim-kos/node-retry

operation.reset() is somewhat unintuitive

timbowhite opened this issue · 1 comments

Hi. Thanks for this module, it's come in handy.

I just started trying out operation.reset() and I find it's usage a little weird. It requires that you wrap the entire operation.attempt() in another closure function, and then recursively call that closure function after operation.reset() in order to start the operation from scratch again, ie:

  var operation = retry.operation();
  var fn = function() {
    operation.attempt(function(currentAttempt) {
      ...
      if (needToStartAgain()){
        operation.reset();
        fn();
        return;
      }
      if (operation.retry(error)) return;
      ...
    });
  }

This just seems a little clunky because you could easily just declare a whole new operation in fn() instead of reusing the old one, at which point reset isn't even necessary.

My idea is that calling reset and then retry (with or without an error) would begin the operation all over again from scratch, ie:


  operation.attempt(function(currentAttempt) {
    ...
    if (needToStartAgain()){
      operation.reset();
      operation.retry();
      return;
    }
    if (operation.retry(error)) return;
    ...
  });

Basically it's a way to force retrying from scratch.

Anyways, just an idea. I'll submit a PR, feel free to reject.

Oh one other thing. As it is currently, more than one reset call probably won't work. The reset function assigns _originalTimeouts to _timeouts as an object reference, which will modify _originalTimeouts in any subsequent retry calls, might be better to clone it instead:

lib/retry_operation.js

RetryOperation.prototype.reset = function() {
  this._attempts = 1;
  this._timeouts = this._originalTimeouts; // should clone this instead, via JSON.parse(JSON.stringify(this._originalTimeouts));
}

PR #66