Is wrapping a promise in a promise an anti-pattern?

Question

I was trying to write code for reconnecting to a database with a timeout using a Promise API.

What I ended up doing in the end was wrapping the promise to connect to the DB in a promise, but I'm not sure if that's the best way to do things. I thought there might be a way to use the original promise from trying to connect to the db, but I couldn't figure it out.

function connect(resolve) {
  console.log('Connecting to db...');
  MongoClient.connect(url, { promiseLibrary: Promise })
    .then((db) => resolve(db))
    .catch((err) => {
      console.log('db connection failed!:\n', err);
      if (retry++ < 3) {
        console.log('Trying again...');
        setTimeout(() => connect(resolve), 5000);
      } else {
        console.log('Retry limit reached!');
      }
    });
}

module.exports = new Promise(connect);

I think it would be possible without the setTimeout block, but I couldn't work around it.

Doesn't `MongoClient` handle retries automatically? I think this is an XY problem. — Evan Davis, Jan 08 '16 at 16:19
I think that depends on the implementation IMO. Given your example though, I don't see why it would be an issue, although `retry` appears to be an implied global so would have that declared within the scope of the `connect` function to iron out any potential issues. i.e If retry is reset elsewhere, you could end up in an infinite loop :-/ — An0nC0d3r, Jan 08 '16 at 16:19
`setTimeout` doesn't return a promise, so you have to do this kind of wrapping anyway. — kjaquier, Jan 08 '16 at 16:24
some answers to http://stackoverflow.com/questions/26694467/promises-repeat-operation-until-it-succeeds may be useful for the retry portion — Jaromanda X, Jan 08 '16 at 16:25
@Mathletics I don't think so. The db object it returns has options for retrying queries, but the actual MongoClient doesn't seem to have that capability for the main connection from what I see here http://mongodb.github.io/node-mongodb-native/2.0/api/MongoClient.html#.connect — m0meni, Jan 08 '16 at 16:42
To answer your title question: [Yes, it is the `Promise` constructor antipattern](http://stackoverflow.com/q/23803743/1048572)! — Bergi, Aug 04 '16 at 23:16

Madara's Ghost · Accepted Answer · 2019-10-15T09:26:45.277

8

Here's a slightly more general solution (tests for non-positive):

function withRetry(asyncAction, retries) {
  if (retries <= 0) {
    // Promise.resolve to convert sync throws into rejections.
    return Promise.resolve().then(asyncAction); 
  }
  return Promise.resolve()
    .then(asyncAction)
    .catch(() => withRetry(asyncAction, retries - 1));
}

This function will take a function that returns a promise, and a number of retries, and retry the function as many times as retries, if the Promise rejects.

If it resolves, the retry chains stops.

In your case:

let connectionPromise = withRetry(connect, 3); // connect with 3 retries if fails.

edited Oct 15 '19 at 09:26

answered Jan 10 '16 at 14:51

Madara's Ghost

172,118
50
264
308

Possible improvements to that function include ability to accept parameters on `asyncAction`, aggregate the errors somehow, indicate that the promise resolved only after N retries, etc. – Madara's Ghost Jan 10 '16 at 15:44
How is `Promise.resolve().then(asyncAction)` different from `asyncAction()`? – Jan 10 '16 at 16:44
1

@torazaburo If `asyncAction` throws an error synchronously, it will be caught in the asynchronous `.catch()` on the promise. Without it, you will need a sync `try {} catch() {}` block, which is also an option (but we're doing Promises here). – Madara's Ghost Jan 10 '16 at 16:45
@MadaraUchiha I might have accepted a bit prematurely. How does this work if async action is unable to return a promise? Such as when wrapped inside of a setTimeout? – m0meni Jan 10 '16 at 20:30
1

@AR7 Well, this function was written with promises, but you can fairly easily do the same with a callback-based function (it took me 10 lines to write this on the fly). Then again, any callback based function can (and in my honest opinion, should) be promisified. If your function has an asynchronous side effect that's not returned or is accessible from anywhere, you have a code smell. – Madara's Ghost Jan 10 '16 at 20:32
@MadaraUchiha I think you misinterpreted the question. For example in my own code example here: `setTimeout(() => connect(resolve), 5000)` I might have done it differently if `return` inside of a `setTimeout` actually returned a promise, but it doesn't behave that way. Also if you look here: https://github.com/petkaantonov/bluebird/wiki/Promise-anti-patterns they have an example on `setTimeout` where they make a `Promise.pending()` but that is no longer part of their API. – m0meni Jan 10 '16 at 20:34
Yes well, adding a time delay between retries is something that would be added to the `withRetries` function, it's pretty trivial to introduce (hint, it should come right before the recursive call to `withRetries(asyncAction, retries - 1)` – Madara's Ghost Jan 10 '16 at 20:37
@MadaraUchiha I see now that you could just add a `.delay()` to remedy that. Thanks for the help. – m0meni Jan 10 '16 at 20:39
@MadaraUchiha I have a new question that's related to this one but more broad http://stackoverflow.com/questions/34710730/how-do-you-return-promises-from-functions-that-dont-allow-for-return-values-wit – m0meni Jan 10 '16 at 21:03

Is wrapping a promise in a promise an anti-pattern?

1 Answers1

Linked