178

I'm using the async.eachLimit function to control the maximum number of operations at a time.

const { eachLimit } = require("async");

function myFunction() {
 return new Promise(async (resolve, reject) => {
   eachLimit((await getAsyncArray), 500, (item, callback) => {
     // do other things that use native promises.
   }, (error) => {
     if (error) return reject(error);
     // resolve here passing the next value.
   });
 });
}

As you can see, I can't declare the myFunction function as async because I don't have access to the value inside the second callback of the eachLimit function.

Alexis Tyler
  • 1,394
  • 6
  • 30
  • 48
  • "As you can see, i can't declare the myFunction as async" --- can you elaborate more? – zerkms Mar 27 '17 at 00:44
  • 1
    Oh, ok... sorry. I need the constructor because i need the async.eachLimit to avoid more than 500 asynchronous operations at a time. I'm downloading and extracting data from text files and i want avoid to much asynchronous operations, after i extract the data, i must return a Promise with the data, and i wont be able to return it from the callback of the async.eachLimit. –  Mar 27 '17 at 00:50
  • 1. Why do you need the await? Async is already a control-flow mechanism. 2. If you want to use async.js with promises inside node.js take a look at async-q – slebetman Mar 27 '17 at 03:44
  • To avoid callback hell, and if something throws, the outer promise would catch. –  Mar 27 '17 at 12:28

5 Answers5

131

You're effectively using promises inside the promise constructor executor function, so this is the Promise constructor anti-pattern.

Your code is a good example of the main risk: not propagating all errors safely. Read why there.

In addition, the use of async/await can make the same traps even more surprising. Compare:

let p = new Promise(resolve => {
  ""(); // TypeError
  resolve();
});

(async () => {
  await p;
})().catch(e => console.log("Caught: " + e)); // Catches it.

with a naive (wrong) async equivalent:

let p = new Promise(async resolve => {
  ""(); // TypeError
  resolve();
});

(async () => {
  await p;
})().catch(e => console.log("Caught: " + e)); // Doesn't catch it!

Look in your browser's web console for the last one.

The first one works because any immediate exception in a Promise constructor executor function conveniently rejects the newly constructed promise (but inside any .then you're on your own).

The second one doesn't work because any immediate exception in an async function rejects the implicit promise returned by the async function itself.

Since the return value of a promise constructor executor function is unused, that's bad news!

Your code

There's no reason you can't define myFunction as async:

async function myFunction() {
  let array = await getAsyncArray();
  return new Promise((resolve, reject) => {
    eachLimit(array, 500, (item, callback) => {
      // do other things that use native promises.
    }, error => {
      if (error) return reject(error);
      // resolve here passing the next value.
    });
  });
}

Though why use outdated concurrency control libraries when you have await?

Michael M.
  • 10,486
  • 9
  • 18
  • 34
jib
  • 40,579
  • 17
  • 100
  • 158
  • 17
    You don't need `return await`: `return new Promise` is sufficient. – lonesomeday Mar 27 '17 at 16:02
  • Because i need control the maximum number of async operations at a time. Do you know any other way to do it without the eachLimit? @jib –  Mar 27 '17 at 16:46
  • I need this because i'm spawning child process, and if i don't control the number, this will cause OS exceptions. Sometimes i need open 10.000 child process's. –  Mar 27 '17 at 16:50
  • 2
    I officially approve this answer, I'd have said exactly the same :-) – Bergi Mar 27 '17 at 17:10
  • 1
    @celoxxx Have a look [here](http://stackoverflow.com/a/39197252/1048572). You indeed should never use async.js with promises – Bergi Mar 27 '17 at 17:11
  • I don't understand TypeScript @Bergi. –  Mar 27 '17 at 20:14
  • I'm a bit curious... why not use async + promises? Promise.all won't resolve the problem of limiting the number of async operations. –  Mar 27 '17 at 20:16
  • That's not the first time i see you saying it's wrong use the Promise + Async module. I really would like to understand what problems would case this. –  Mar 27 '17 at 20:17
  • 1
    @celoxxx Just drop the types and it becomes plain js. You should not use async.js because the different interfaces - node-style callbacks vs promises - cause too much friction and lead to unnecessary complicated and error-prone code. – Bergi Mar 27 '17 at 20:35
  • 1
    I agree with you... But this code is old, and i'm refactoring to use events + async.js (to control the limit of async, yet. If you know a better way, please say). –  Mar 27 '17 at 20:39
  • I agree with user5487299 - if you have100,000 tasks that need to be run, then you don't necessarily want synchronous execution. But you can't execute them all at once either. A library like [p-limit](https://www.npmjs.com/package/p-limit) allows you to specify the appropriate amount of concurrency to your desired resource usage and speed. – Matthew Rideout Sep 13 '18 at 14:21
66

Based on the feedback given by @Bergi, I want to highlight that I agree with the answers given above and would rather choose a different approach.

However, if you need to have async inside your promise, I would consider using something like this:

const operation1 = Promise.resolve(5)
const operation2 = Promise.resolve(15)
const publishResult = () => Promise.reject(`Can't publish`)

let p = new Promise((resolve, reject) => {
  (async () => {
    try {
      const op1 = await operation1;
      const op2 = await operation2;

      if (op2 == null) {
         throw new Error('Validation error');
      }

      const res = op1 + op2;
      const result = await publishResult(res);
      resolve(result)
    } catch (err) {
      reject(err)
    }
  })()
});

(async () => {
  await p;
})().catch(e => console.log("Caught: " + e));
  1. The function passed to Promise constructor is not async, so linters don't show errors.
  2. All of the async functions can be called in sequential order using await.
  3. Custom errors can be added to validate the results of async operations
  4. The error is caught nicely eventually.

A drawback though is that you have to remember putting try/catch and attaching it to reject.

Vladyslav Zavalykhatko
  • 15,202
  • 8
  • 65
  • 100
  • it gives linting error: `Promises must be handled appropriately or explicitly marked as ignored with the void operator` https://github.com/typescript-eslint/typescript-eslint/blob/v4.28.0/packages/eslint-plugin/docs/rules/no-floating-promises.md – GorvGoyl Jul 04 '21 at 11:33
  • 3
    While this works, you might as well get rid of the wrapping promise, try/catch etc, and do the same with the remaining of your IEFE function: https://i.stack.imgur.com/S3pU2.png – noseratio Jul 10 '21 at 01:51
  • 3
    @noseratio can't agree more. the op though asked if it's okay to use `async` inside of a `Promise` body. – Vladyslav Zavalykhatko Jul 10 '21 at 15:21
  • This answer is a perfect complement of jib's answer and exactly what that I need to understand. – Marcelo Scofano Diniz Nov 11 '22 at 13:03
  • @VladyslavZavalykhatko If you agree with noseratio, it should be clear that it's not okay to use `async` inside of a `new Promise`. It's not even "*neater*". – Bergi May 01 '23 at 18:54
  • @Bergi I agree with noseratio in the sense that _my particular example_ does not require the wrapping promise; it is provided only to illustrate the idea – Vladyslav Zavalykhatko May 01 '23 at 19:41
  • @VladyslavZavalykhatko Then please [edit] your answer to show a better example. (Hint: you shouldn't be able to find any where the wrapping promise is required) – Bergi May 01 '23 at 19:45
  • @Bergi I agree that the wrapping promise won't be _required_. a necessity to use callbacks and passing `resolve` or `reject` could be a case when it's "_neater_" – Vladyslav Zavalykhatko May 01 '23 at 20:05
  • @VladyslavZavalykhatko There is no necessity to use callbacks though? – Bergi May 01 '23 at 20:26
  • @Bergi in own code probably not. what would you do for 3rd party providers exposing functions expecting callbacks? – Vladyslav Zavalykhatko May 01 '23 at 20:28
  • Let us [continue this discussion in chat](https://chat.stackoverflow.com/rooms/253416/discussion-between-vladyslav-zavalykhatko-and-bergi). – Vladyslav Zavalykhatko May 01 '23 at 20:29
9

BELIEVING IN ANTI-PATTERNS IS AN ANTI-PATTERN

Throws within an async promise callback can easily be caught.

(async () => {
    try {
        await new Promise (async (FULFILL, BREAK) => {
            try {
                throw null;
            }
            catch (BALL) {
                BREAK (BALL);
            }
        });
    }
    catch (BALL) {
        console.log ("(A) BALL CAUGHT", BALL);
        throw BALL;
    }
}) ().
catch (BALL => {
    console.log ("(B) BALL CAUGHT", BALL);
});

or even more simply,

(async () => {
    await new Promise (async (FULFILL, BREAK) => {
        try {
            throw null;
        }
        catch (BALL) {
            BREAK (BALL);
        }
    });
}) ().
catch (BALL => {
    console.log ("(B) BALL CAUGHT", BALL);
});
Bryan Grace
  • 1,826
  • 2
  • 16
  • 29
  • 2
    It really is a pattern that should never be used. Just remove the line `await new Promise (async (FULFILL, BREAK) => {` and replace `BREAK` with `throw`. – Bergi Feb 18 '23 at 08:48
  • 2
    Actually no, this is anti-pattern and demonstrates you do not fully understand Promises and the supporting async..await syntax. Moreover it's very ugly, especially the eye-crossing line of `}) ().` – Mulan Feb 22 '23 at 16:46
  • 1
    "BELIEVING IN ANTI-PATTERNS IS AN ANTI-PATTERN" sounds like an attempt to be contrarian for the sake of it. Adding pointless wrappers on top of promises like this makes code objectively more confusing and is very much an antipattern. Even if this was good practice, JS isn't COBOL, so naming variables in all caps and calling `(resolve, reject)` `(FULFILL, BREAK)` would never fly in any PR or linter I know of. – ggorlen Mar 02 '23 at 03:27
  • @ggorlen--onLLMstrike has the right of it. – limeandcoconut Jun 07 '23 at 17:45
1

I didn't realized it directly by reading the other answers, but what is important is to evaluate your async function to turn it into a Promise.

So if you define your async function using something like:

let f = async () => {
    // ... You can use await, try/catch, throw syntax here (see answer of Vladyslav Zavalykhatko) .. 
};

your turn it into a promise using:

let myPromise = f()

You can then manipulate is as a Promise, using for instance Promise.all([myPromise])...

Of course, you can turn it into a one liner using:

(async () => { code with await })()
tobiasBora
  • 1,542
  • 14
  • 23
  • This should be the top answer, it is actually what u want to do with the new Promise(async) thing, u jsut want to store the async and just call it, it's the same Thank u for opening my eyes lol – Pipo Aug 30 '23 at 16:05
-6
static getPosts(){
    return new Promise( (resolve, reject) =>{
        try {
            const res =  axios.get(url);
            const data = res.data;
            resolve(
                data.map(post => ({
                    ...post,
                    createdAt: new Date(post.createdAt)
                }))
            )
        } catch (err) {
            reject(err);                
        }
    })
}

remove await and async will solve this issue. because you have applied Promise object, that's enough.

Alain
  • 21
  • 3
  • 1
    So in your example, will `axios.get(url)` function as though it was called as `await axios.get(url)`? – PrestonDocks Oct 24 '20 at 11:40
  • 2
    No it won't, `res` will contain a promise and rest of the code will fail since `res.data` will be undefined. – CherryDT Mar 01 '21 at 09:07
  • This code is broken and misleading. Suggest deletion. Even if you _could_ magically avoid `async`/`await` with a `new Promise()`, it's still uglier than just using `async`/`await`. – ggorlen Mar 02 '23 at 00:56