What is wrong with this recursive Promise function with jQuery Deferred

Question

I am working on my scripts for ReST API processes. Now I need a function which keeps retrying to request to the API as many as possible in few seconds.

So I wrote some Promise abstractions and I made something like below:

$(function () {
  var $el = $('#test');
  function api (message) {
    return $.ajax('/echo/json/', {
      method: 'POST',
      data: { json: JSON.stringify({ message: message }), delay: 2000 },
      timeout: 1000
    });
  }
  // Process to keep retrying an API request as many as possible in 3 seconds
  retried(_.bind(api, undefined, 'Hello, world.'), 3000)
  .then(function (dat) {
    $el.text(dat.message);
  }, function (err) {
    if (err instanceof Error) $el.css('color', 'red');
    $el.text(err.message);
  });
});

Some functions I made for the above are below:

// Promise wrapper
var promisify = function (func) {
  var funcPartial = function () {
    var funcArgs = _.toArray(arguments);
    var dfr = new $.Deferred(), promiseArgs = [ dfr.resolve, dfr.reject ];
    var timeoutId = setTimeout(function () {
      clearTimeout(timeoutId);
      func.apply(undefined, _.union(promiseArgs, funcArgs));
    }, 1);
    return dfr.promise();
  };
  return funcPartial;
};

// Promise abstraction for recursive call
retried = promisify(function (resolve, reject, done, duration, start) {
  if (!_.isNumber(start)) start = +(new Date());
  return done()
  .then(resolve, function (err) {
    var stop = +(new Date());
    if (duration <= stop - start) {
      reject(err);
    } else {
      return retried(done, duration, start);
    }
  });
});

The process finishes in a correct condition, but the retried function won't return Promise chain.

I am really stacked. Can someone point me out the wrong points to correct the implementation above?

Here's the entire demo script.

DEMO

Thank you.

SOLVED

Thanks to @BenjaminGruenbaum below, I have just noticed that I did not need promisify to make retried function at all. This was completely shame question, but thanks again to all the people replied on this question.

This is revised retried function which does not need promisify at all...

var retried = function (done, duration, start) {
  if (!_.isNumber(start)) start = +(new Date());
  return done()
  .then(function (dat) {
    return dat;
  }, function (err) {
    var stop = +(new Date());
    if (duration > stop - start) return retried(done, duration, start);
    return err;
  });
};

I updated the demo and it works fine now XD

DEMO (revised)

`retried` returns the funcPartial function ... should it `return funcPartial()` instead? — Jaromanda X, Sep 18 '15 at 04:30
Well, the first thing that's wrong with it is that it's ridiculously complicated to just retry - so much more complicated than needed that it's quite hard to follow your code. — jfriend00, Sep 18 '15 at 04:54
@JaromandaX I think `promisify` should return the function `funcPartial` instead of the result of `funcPartial` because `promisify` is supposed to be an higher-order function to create a Promise function. — Japboy, Sep 18 '15 at 05:05
@jfriend00 You're right. But I already have many Promise based API wrapper functions which are supposed to be retri-able. So just wondering if I could make an abstraction to wrap them with Promise interface... — Japboy, Sep 18 '15 at 05:08

score 2 · Accepted Answer · edited May 23 '17 at 11:58

2

Your logic for retrying is very complicated. You're "promisifying" a function with resolve and reject and performing explicit construction. How would you implement retry in synchronous code?

do {
   var failed = false;
   try { 
       var val = fn();
   } catch(e){
       failed = true;
   }
} while(failed);

But we can't really do that, because we can't use loops, we can however use recursion instead:

function retry(fn){
    try {
       return fn(); // call the function
    } catch(e){
       return retry(fn); // retry it again
    }
}

Now, adding promises in just means that failure isn't through try/catch but through the promise's resolution/lack of:

function retry(fn) {
    return fn().then(null, function(e){ 
        return retry(fn); // on failure handler, retry again.
    });
}

This implementation has the benefit of not depending on the promise library used (it'll use the same type fn returns).

As a side note, please don't ignore errors. Even network errors - at least log them somewhere :)

edited May 23 '17 at 11:58

Community

1
1

answered Sep 18 '15 at 14:56

Benjamin Gruenbaum

270,886
87
504
504

If they're going to ignore your good advice and not handler the error altogether then you could simplify: `return fn().then(null, retry.bind(null, fn));` – sdgluck Sep 18 '15 at 16:34
1

I mostly wanted to emphasize what's being done, I'd probably just do `fn().catch(e => retry(fn))` but that has even more constraints. I guess it's about finding the balance. – Benjamin Gruenbaum Sep 18 '15 at 16:44
Absolutely. I didn't see any ES6 code in the question so didn't want to jump the gun on that one. – sdgluck Sep 18 '15 at 16:46
@BenjaminGruenbaum After I read your advice, I've just noticed that I did unnecessary Promise wrapping like your said "explicit construction." Oops. So I didn't need the `promisify` function to make `retried` function because I passed Promise as the first argument for `retried`... Shame. Thank you for your kind and careful advice XD – Japboy Sep 19 '15 at 09:22

What is wrong with this recursive Promise function with jQuery Deferred

1 Answers1