loading...

Avoid This async/await Antipattern With This ONE WEIRD TRICK

ronnewcomb profile image Ron Newcomb ・2 min read

I had a coworker who kept making this common mistake with his async/await code.

var states = await GetAllStates();
var addressTypes = await FetchAddressTypes();
var phoneTypes = await FindPhoneTypes();

This code doesn't get the three data types in parallel like he wanted. It gets them in sequence, each waiting on the first to finish before starting. Moreover, the correct code seemed messy for no reason.

var statesTask = GetAllStates();
var addressTypesTask = FetchAddressTypes();
var phoneTypesTask = FindPhoneTypes();

var states = await statesTask;
var addressTypes = await addressTypesTask;
var phoneTypes = await phoneTypesTask;

Indeed, this is just begging for a neatness-obsessed dev like myself to decide the three extra variables are pointless, and to optimize them away by turning this back into the first example.

Now, there's a lot of good tutorials on async/await out there for C#, Typescript, and Javascript, but it's understandably a deep subject, and a lot of us just don't have the time to be an expert on everything. But there's a simple trick you can do to avoid committing this mistake, either de novo or when refactoring.

And the trick is this: use the -ing form of the verb.

var gettingAllStates = GetAllStates();
var fetchingAddressTypes = FetchAddressTypes();
var findingPhoneTypes = FindPhoneTypes();

var states = await gettingAllStates;
var addressTypes = await fetchingAddressTypes;
var phoneTypes = await findingPhoneTypes;

The -ing form of verbs, when combined with "is" or "has" as a helping verb, produces the continuous tense. "I am getting the states." "He is fetching the address types." "She is finding the phones." It denotes an action that is in progress. And that's kind of the whole point of async/await, to start several tasks/promises in parallel and only start waiting on their completion when you must.

Since adopting this naming convention, my coworker has never committed this antipattern ever again.

Shocking, I know.

Posted on by:

ronnewcomb profile

Ron Newcomb

@ronnewcomb

misses his Commodore

Discussion

markdown guide
 

This isn't an anti-pattern, and you don't need write it like this. Just use:

const [states, addressTypes, phoneTypes] = await Promise.all([GetAllStates(), FetchAddressTypes(), FindPhoneTypes()]);

 

That looks really ugly to me. You have to read it left, right, left right, left, right.

You'll probably also lose all typing information if you do it like this (although TypeScript already can type a lot of stupid Javascript idioms).

 

It's not a stupid idiom, it's just passing an array to an async function and receiving an array as a return value. If it's easier for you, you can type

const getAllStatesPromise = GetAllStates();
const fetchAddressTypesPromise = FetchAddressTypes();
const findPhoneTypesPromise = FetchPhoneTypes();

const promisesToAwait = [
  getAllStatesPromise, 
  fetchAddressTypesPromise, 
  findPhoneTypesPromise 
];

const resolvedPromises = await Promise.all(promisesToAwait);

const states = resolvedPromises[0];
const addressTypes = resolvedPromises[1];
const phoneTypes = resolvedPromises[2];

but why would you want to do that???

 

For this particular case, Promise.allSettled() would be probably be preferable.

Though I guess it depends on how you want to do error handling.

 

Yes, I would agree. I posted a similar comment down below. If the async functions we're calling do their own error handling, then it may not be necessary, but if they don't, or you're not sure, it's good to have the result data provided by allSettled()

 
 

In a video I watch from the NodeJS conventions, there is a trap.
Actually, promise.all works well in case of everything being successful.
If one promise fails, the other promises continue running and if they fail too, boom!

 

The promises continue running in any case if you have multiple of them pending, regardless of the mechanism you use. (You have to pass around an abort token or the like to cancel execution in that case, and check it through the execution of the async functions being called, to deal with that case.)

Promise.all() does hook rejection on all of them, though admittedly it does swallow the other errors if any of the others fail as well. Sequential await will leave you with unhandled rejections, though, if an earlier promise in the sequence rejects.

 

This is a good point. In this case, you would want to make sure that the async functions you're calling (e.g. GetAllStates()) are catching and handling their own errors. Another option is to use Promise.allSettled() which will return objects that have a status of 'fulfilled' or 'rejected' and either a value (the return value of the function) or a reason (the error message) if rejected.

 

This is not an antipattern. Async await was not created for parallel processing but for reducing hanging worker thread when waiting for io completion. Use parallel libs for this.

 

I think if it's not an antipattern then it's at least something that developers should avoid, waiting for multiple unrelated Promises, only starting each one after a previous one is finished is clearly not good.

 

If you were doing database calls in those async methods, then your "improved" solution would break, because most database drivers aren't thread safe (I'm not aware of any). So you should really understand what you're doing, when trying to improve something.

Actually no, almost all dbs are threadsafe, as long as each thread opens a separate connection.

Right, except that I write that no DB driver (I am aware of) is thread safe. If you have a separate connection object, your connection doesn't need to be thread safe anymore. Thread safety is always defined on a single object or static methods. Also if you have separate connections, you can share the same transaction unless you'd start using distributed transactions, which probably isn't what you want.

Are you referencing C# here? Because your comments do not apply to JavaScript.

I was talking about C# (that's the first tag on the post), but nodejs should behave the same - although it seems, that there is at least one db driver for nodejs, which is thread safe - github.com/brianc/node-postgres/is... - which I find very interesting, although it might completely screw up the result that you get, if you are running any DML statements. Anyhow, having separate await statements for each async statement isn't an anti pattern, as others have pointed out, it simply solves a different problem - reducing the number of idle threads, and threads are expensive, or in JS - you simply only have a single thread and awaiting an async operation frees up that thread to do other tasks, it's not wasted.

Thread safety is not really a concern here. I think you might be misunderstanding how Node.js works.

Ok, enlighten me. This is my understanding of JavaScript engines, and it might be completely wrong or just off for node js:

JavaScript is single threaded, with an event loop to support async operations. That means, i++ is always stable,

Console.log(i);
SyncFuncWhichDoesNotChangeI();
Console.log(i);

That code above also prints the same value twice, always. But...

Console.log(i);
await AsyncFuncWhichDoesNotChangeI();
Console.log(i);

Can print separate values for i, because it can be changed by another piece of code which jumps in while we're waiting.

If my understanding as described above is correct, than it's straight forward to design a system, which starts breaking when you remove the immediate await. It might not be as easy as in c# where even i++ isn't stable unless you lock it (or do some other stuff), but it's definitely possible. Maybe that's what you meant, that it's not an issue? Otherwise I'm intrigued to know, what node js has up it's sleeve.

Cheers

There's a fair bit to unpack so apologies if I miss something.

Regarding database access using async/await in node: using Promise.all does not result in multiple threads (you can see this by sticking breakpoints in the methods; only one will run at a time). All you're doing with await is waiting for the asynchronous action that returns data to return its value before continuing. These two functions are functionally identical:

()=>{
  const foo = await bar();
  console.log(foo);
}

()=>{
  bar()
  .then(foo => {
    console.log(foo);
  });
}

Worth underlining that I explicitly said unrelated promises. If promises have interdependencies then yeah, they need to wait for each other.

Regarding your examples, the only way i could be changed during an await is if another callback has i in its scope and changes it. In which case, if you want i to change, you should be awaiting the promise that you expect to make that change. If you don't want i to change, don't await anything, the code is synchronous and you can safely run the rest of your code knowing that, even though you triggered some async functions, they won't resolve until the next event loop at the earliest.

If that misses your point I apologise. Do you have a more fleshed out example that we can see to try and clarify things?

 

Since those are most likely all io bound calls (say rest or DB calls) there's no processing going on and using async/await is exactly the right solution here. Parallel libraries are only useful if you want to parallelize actual CPU bound work.

 

The synchronous behavior, the execution pausing is purposeful with await. If you want parallel behavior use Promise.all([first, second, third]) or even Promise.allSettled([first, second, third]), pass in the awaited variables.

 

In a video I watch from the NodeJS conventions, there is a trap.
Actually, promise.all works well in case of everything being successful.
If one promise fails, the other promises continue running and if they fail too, boom!

This is why all settled is coming I guess.

 

Yeah, it's why it added it to my comment. Thanks for elaborating further though 🙂

 

Would definitely just use await Promise.all() for this

 

In C# using await like in the "bad" example is the correct pattern, as it's not designed at all for parallelism. Also, your weird example will produce the exact same effect in C#.

If this is just a JavaScript example you should make that clear, although I thought browser JavaScript code is still all single threaded, but I don't do much in the way of JavaScript so maybe I'm wrong though. Either way I think you at least need to clarify a few things.

 

C# has Task.WhenAll(taskArray) for this...

 

The purpose of async/await is parallelism (in the form of cooperative multitasking), so if you don't use it, just use a plain old fashioned function instead of an async function.

If you are always calling your async function with await before it, you're getting none of the benefits of async.

 

Saw 'var' in code, stopped reading 😂

 

I write it that way so the code snippets are valid C# and Typescript both, but, yeah, understood. :)

 

Not so much an antipattern as a subtle bug. Another subtle bug is something like:

async function checkSomeCondition() : boolean;

if (checkSomeCondition()) ...;

Because it returned a promise and promises are truthy, the condition is always true! Watch out for this one! It can happen especially when you change a non-async function that is already used somewhere into an async one.

It's worth noting that you can await any value, if it's not a promise, the result is the value itself. For instance:

let x = 123;
console.log(await x);
// prints 123

While awaiting literally every variable is way overkill, you could use this in some cases if you are planning to change to async later in your changeset, because it will continue to work even before you change the function.

 
 

For God sakes, use Promise.all([]), and don't write dribble when you don't know what you're talking about.

 

Mmm I would have been nicer but this is roughly what I was thinking.

 

There's no such thing as parallel in JavaScript

 
 

Much like the half-joke that "all computer wait at the same speed", it is also true that "all computers are parallel when waiting on 2+ other computers". :D

 

I don't think it is "anti-pattern".
It's just a way how business logic works..
In some frameworks you can't use parallel execution with.all() or similar statements, for example Entity Framework.
So let's be careful with using strong terms like "anti-pattern".

 

The verb change just makes more clear what's happening. You're still not fetching them in parallel like you probably want to be. I have to agree that promise.all is probably what you want here

 

If only there was a parallel keyword... Wait that would be stupid.

 

Funny you mention that, in C# they do have a parallel keyword.

Parallel.ForEach(files, (currentFile) =>
 {
     // The more computational work you do here, the greater
     // the speedup compared to a sequential foreach loop.
     string filename = Path.GetFileName(currentFile);
     var bitmap = new Bitmap(currentFile);

     bitmap.RotateFlip(RotateFlipType.Rotate180FlipNone);
     bitmap.Save(Path.Combine(newDir, filename));

     // Peek behind the scenes to see how work is parallelized.
     // But be aware: Thread contention for the Console slows down parallel loops!!!

     Console.WriteLine($"Processing {filename} on thread {Thread.CurrentThread.ManagedThreadId}");
     //close lambda expression and method invocation
 });
 

WTF is this? Now what's anti pattern here (Promise All can be better) and by putting F -ing it becomes neat? Rather heading of this post is deceptive .. Rest I agree naming convention helps

 

This is a good trick which also makes the code more readable. Thanks.