1

I've been trying to figure out how to best implement retrying e.g. a failed download using promises. My best guess is to resolve a promise with a new promise (see the pseudocode below), but all I could find in the documentation is that Promise.resolve() can be called with a thenable (and so, I believe, with a Promise). Does this apply to the resolve callback in the promise constructor as well? Is my approach correct or is there a better way to implement a retry?

function getdata(url) {
   return new Promise(function(resolve, reject) {
      ajaxcall({
         url: url,
         success: function(data) { resolve(data); },
         failure: function(err) { 
            if(retriable(err)) {
               resolve(getdata(url));
            }
            else {
               reject(err);
            }
         }
      });
   });
}
cseprog
  • 557
  • 1
  • 5
  • 9
  • That's pretty easy to find out: `new Promise(resolve => resolve(Promise.resolve(42))).then(v => console.log(v));` logs `42`, not a promise. So yeah, it seems to work with `resolve` as well. – Felix Kling Jan 18 '17 at 23:16
  • generic promise retry helper `var retry = fn => _ => fn().catch(retry(fn));` – Jaromanda X Jan 18 '17 at 23:20
  • 1
    generic limited number of promise retries `var retryP = (fn, retry) => fn().catch(err => retry > 0 ? retryP(fn, retry - 1) : Promise.reject(err));` – Jaromanda X Jan 18 '17 at 23:21
  • See: [Promise retry patterns](http://stackoverflow.com/questions/38213668/promise-retry-design-patterns/38214203#38214203) – jfriend00 Jan 18 '17 at 23:22

2 Answers2

4

Your approach would work. When a promise is passed to the resolve callback of the Promise constructor, the resolution value of that promise is used as the resolution value of the "outer" promise.

It could be cleaner to wrap ajaxcall in a promise-returning function so that you're not mixing and matching promise and callback paradigms.

function fetch(url) {
  return new Promise(function(resolve, reject) {
    ajaxcall({
      url: url,
      success: resolve,
      failure: reject
    });
  });
}

function getdata(url) {
  return fetch(url)
    .catch(function(err) {
      if(retriable(err)) {
        return getdata(url); // or fetch(url) to retry only once
      } else {
        throw err;
      }
    });
}
Ates Goral
  • 137,716
  • 26
  • 137
  • 190
  • note that under modern rules this might get flagged as erroneous code, as that inner `fetch` has no error handling and JS engines are being encouraged to disallow promises that let errors disappear into the void. You could return `getdata(url)` again, instead of `fetch(url)` – Mike 'Pomax' Kamermans Jan 18 '17 at 23:20
  • @Mike'Pomax'Kamermans could you please elaborate or point to a resource where this rule is mentioned? Who would be doing the "flagging"? A linter? – Ates Goral Jan 18 '17 at 23:25
  • The engines themselves. For instance, the version of V8 currently used by Node.js, when telling them to run `new Promise(function(resolve, reject) { throw new Error('test'); })` will warn `UnhandledPromiseRejectionWarning: Unhandled promise rejection (rejection id: 2): Error: test`. In this particular case: by calling `fetch` rather than `getdata`, we lose the ability to "see" (and deal with) errors that are unrelated to the initial failure (for instance, a hardware glitch causing a network error on the second attempt) – Mike 'Pomax' Kamermans Jan 18 '17 at 23:28
  • 1
    But, in the case above, wouldn't it be the responsibility of the caller of `getdata()` to add the `.catch()` handler? How would your suggestion alleviate the problem if the caller of `getdata()` doesn't catch the eventual error? – Ates Goral Jan 18 '17 at 23:32
  • naturally, but getdata already has code in place to deal with retries, tapping into that and making sure `retriable(err)` does the right thing for as many times as a retry is necessary is far more valuable than a "retry once, and once only" construction. Your suggestion isn't bad, but it can be made much better with an almost trivial change. – Mike 'Pomax' Kamermans Jan 18 '17 at 23:34
  • The thing I don't understand is, in the `else` branch of error handling, the error is being rethrown. Wouldn't that still result in an unhandled rejection warning? I mean, even if the trivial change you suggested is made? – Ates Goral Jan 18 '17 at 23:36
  • Let us [continue this discussion in chat](http://chat.stackoverflow.com/rooms/133500/discussion-between-ates-goral-and-mike-pomax-kamermans). – Ates Goral Jan 18 '17 at 23:38
  • leaving this comment, to keep it part of the immediately visible thread: calling `getdata` rather than `fetch` maintains the separation of concerns. With `getdata(url)`, the `else` will return to the caller's error handling only for errors when the url cannot be retried. With just `fetch(url)` the calling function will need to deal with both types of errors, because a still retriable url that fails on the first retry will end the promise. In the first use case, the caller can rely on only ever seeing one type of error, without needing to know about retries, or what code to call for that. – Mike 'Pomax' Kamermans Jan 18 '17 at 23:47
  • Ah, I see. Beyond the unhandled rejection warning, the issue is that I wasn't clear on whether the OP meant to make the retries indefinite or for just once. Yes, indeed, with your suggestion, retries can be made indefinite. I'm still not convinced that the above code would have any problems in terms of an unhandled rejection warning (but I'm still in the process of testing that). – Ates Goral Jan 18 '17 at 23:49
-1

You can change your code to call inner function inside Promise. Something like this:

function getdata(url) {
   return new Promise(function(resolve, reject) {
    function call() {
     return ajaxcall({
          url: url,
          success: function(data) { resolve(data); },
          failure: function(err) { 
             if(retriable(err)) {
                call();
             }
             else {
                reject(err);
             }
          }
       });
     }
     call(); 
   });
}
Leonid Lazaryev
  • 326
  • 1
  • 8