Docs: no-return-await deprecation notice leads to a wrong conclusion about eslint

[mdjermanovic]

My understanding of the new version of the deprecation notice proposed in #18166 (comment) is that the rule is deprecated because return await in try {} is useful. But this rule doesn't actually report return await in try {}, so that's not the reason why the rule is deprecated.

As for the stack traces, the trade-off was already documented and isn't the reason why the rule is deprecated either.

The reason this rule is deprecated is that, due to the changes in the ECMAScript specification, return await no longer has a negative impact on performance.

[fasttime]

Hi @3axap4eHko, thanks for the issue. Could you point out exactly what you consider wrong in the deprecation notice of the no-return-await rule, and explain how you think it should be changed?

For context, the deprecation paragraph was added following the discussion in #17345, the rest of the documentation about that rule has not changed.

[3axap4eHko]

@fasttime I would suggest to change the deprecation notice from:

This rule was deprecated in ESLint v8.46.0 with no replacement. The original intent of this rule no longer applies due to the fact JavaScript now handles native Promises differently. It can now be slower to remove await rather than keeping it. More technical information can be found in this V8 blog entry.

to
This rule was deprecated in ESLint v8.46.0 without a direct replacement. The rationale behind the rule has evolved as understanding of JavaScript's native Promise handling has deepened. Specifically, concerns about the performance impact of await in try/catch blocks are often outweighed by the benefits of clearer error handling and code readability. While await may introduce a minor overhead, this is generally considered negligible compared to the assurance of properly caught and managed errors. For more in-depth analysis, refer to this this V8 blog entry.

As you can see it clarifies the importance of await in try/catch and at the same time warns about the little performance overhead it introduces.

[fasttime]

Thanks for the clarification. I'm not sure if this sentence hits the nail in the head:

The rationale behind the rule has evolved as understanding of JavaScript's native Promise handling has deepened.

The main reason for the deprecation of that rule was a change in the spec, and the consequent optimizations in V8, both in performance and in the handling of async stack traces, as mentioned in the blog post. Those changes were implemented while the rule already existed. So there were actually technical motivations, not just a shift in the understanding of how things work, that made us reconsider that rule.

The rest looks good to me. Would you like to submit a pull request to update the documentation of no-return-await, so we can discuss any tweaks right there?

[mdjermanovic]

Note that this rule does not report return await in try blocks, as explained in the examples of correct code:

// In this example the `await` is necessary to be able to catch errors thrown from `bar()`
async function foo4() {
    try {
        return await bar();
    } catch (error) {}
}

I also think the current notice is correct, since there was no change in understanding but in the language specification.

[mdjermanovic]

Since fetchSomething() presumably returns a promise, a, b, and c essentially pass this promise up the chain. The JavaScript engine schedules promise resolutions as microtasks, but since there's a direct pass-through of the promise without additional await operations, the overhead is minimized to the handling of the original promise from fetchSomething().

return <Promise> does not just pass through the Promise:

const promise = Promise.resolve(42);

async function foo() {
    return promise;
}

console.log(foo() === promise); // false

[fasttime]

Note that this rule does not report return await in try blocks, as explained in the examples of correct code:

// In this example the `await` is necessary to be able to catch errors thrown from `bar()`
async function foo4() {
    try {
        return await bar();
    } catch (error) {}
}

I also think the current notice is correct, since there was no change in understanding but in the language specification.

This is correct. But then it seems that the only missing bit of information in the current text is the part about "the assurance of properly caught and managed errors". I understand this as an effect of the propagation of the stack trace and the asynchronous context when using await.

async function foo() {
    await null;
    await undef;
}

async function bar(useAwait) {
    return useAwait ? await foo() : foo();
}

bar(false).catch(e => console.log(e.stack));
// Will print:
//
// ReferenceError: undef is not defined
//     at foo (file:///.../index.js:3:5)

bar(true).catch(e => console.log(e.stack));
// Will print:
//
// ReferenceError: undef is not defined
//     at foo (file:///.../index.js:3:5)
//     at async bar (file:///.../index.js:7:23)

This is also explained in the V8 blog post, although not very prominently, as they were focusing mainly on performance.

[fasttime]

@3axap4eHko what do you think? Does @mdjermanovic's analysis make sense to you or are we missing something?

[3axap4eHko]

If await has no more negative impact on performance, then maybe I misread something, but the blog post entry https://v8.dev/blog/fast-async, makes opposite feelings. And definitely I can't tell after the reading that It can now be slower to remove await rather than keeping it. If it is true, so maybe it does make sense to add some more sources to prove that and leave message as is.

[fasttime]

And definitely I can't tell after the reading that It can now be slower to remove await rather than keeping it. If it is true, so maybe it does make sense to add some more sources to prove that and leave message as is.

The argument about the better performance came in with this discussion. I believe the sentence you are pointing out was introduced to refute the assumption in the following paragraph that using await would be slower ("at the cost of an extra microtask").

[3axap4eHko]

And definitely I can't tell after the reading that It can now be slower to remove await rather than keeping it. If it is true, so maybe it does make sense to add some more sources to prove that and leave message as is.

The argument about the better performance came in with this discussion. I believe the sentence you are pointing out was introduced to refute the assumption in the following paragraph that using await would be slower ("at the cost of an extra microtask").

Exactly! And reading the article from the documentation does not make it clear why.

[3axap4eHko]

And definitely I can't tell after the reading that It can now be slower to remove await rather than keeping it. If it is true, so maybe it does make sense to add some more sources to prove that and leave message as is.

The argument about the better performance came in with this discussion. I believe the sentence you are pointing out was introduced to refute the assumption in the following paragraph that using await would be slower ("at the cost of an extra microtask").

I've looked at the example of code provided in the discussion, and found it a little bit inaccurate. The problem of that example that it states on execution performance by counting ticks, but ticks are not fixed to something, it not time or processor operations. The example bellow shows that async () => Promise.resolve() and async () => await Promise.resolve() creates 2 more promises than () => Promise.resolve()

import { createHook } from "node:async_hooks";
 
const logs = [];
let logEnabled = false;
const log = (...data) => {
  if (logEnabled) {
    logs.push(data);
  }
};
 
const asyncHook = createHook({
  init() {
    log("init");
  },
});
 
async function test(name, fn) {
  let tick = 0;
  const tock = () => {
    tick++;
    log("tick", name, tick);
  };
  const rest = Promise.resolve().then(tock).then(tock).then(tock).then(tock).then(tock);
  logEnabled = true;
  log("start", name);
  await fn();
  logEnabled = false;
  await rest;
}
 
setTimeout(() => {}, 100);
 
asyncHook.enable();
await test("promise-sync", () => Promise.resolve());
await test("promise-async", async () => Promise.resolve());
await test("promise-async-await", async () => await Promise.resolve());
asyncHook.disable();
console.log(logs);

The output shows the same amount of created promises for both async functions.

[
  [ 'start', 'promise-sync' ],
  [ 'init' ],
  [ 'init' ],
  [ 'init' ],
  [ 'init' ],
  [ 'tick', 'promise-sync', 1 ],
  [ 'start', 'promise-async' ],
  [ 'init' ],
  [ 'init' ],
  [ 'init' ],
  [ 'init' ],
  [ 'tick', 'promise-async', 1 ],
  [ 'init' ],
  [ 'tick', 'promise-async', 2 ],
  [ 'tick', 'promise-async', 3 ],
  [ 'start', 'promise-async-await' ],
  [ 'init' ],
  [ 'init' ],
  [ 'init' ],
  [ 'init' ],
  [ 'init' ],
  [ 'tick', 'promise-async-await', 1 ],
  [ 'tick', 'promise-async-await', 2 ]
]

So I've decided to measure execution time of 1_000_000 functions and results are consistently showing a faster execution of async () => Promise.resolve() over async () => await Promise.resolve()

⭐ Script  /home/zource/projects/overtake/__benchmarks__/await-vs-pass.js
  ⇶ Suite  Async await performance
    ➤ Perform  1000000 calls
      ✓ Measure 1000000 calls of function that returns Promise.resolve()
        ┌──────────┬──────────┬──────────┬──────────┬────────────┬─────────┐
        │ (index)  │ med      │ p95      │ p99      │ total      │ count   │
        ├──────────┼──────────┼──────────┼──────────┼────────────┼─────────┤
        │ 0.000084 │ 0.000094 │ 0.000158 │ 0.000285 │ 116.838682 │ 1000000 │
        └──────────┴──────────┴──────────┴──────────┴────────────┴─────────┘
      ✓ Measure 1000000 calls of async function that returns await Promise.resolve()
        ┌─────────┬──────────┬──────────┬──────────┬────────────┬─────────┐
        │ (index) │ med      │ p95      │ p99      │ total      │ count   │
        ├─────────┼──────────┼──────────┼──────────┼────────────┼─────────┤
        │ 0.00011 │ 0.000124 │ 0.000268 │ 0.000425 │ 166.154862 │ 1000000 │
        └─────────┴──────────┴──────────┴──────────┴────────────┴─────────┘
      ✓ Measure 1000000 calls of async function that returns Promise.resolve()
        ┌──────────┬─────────┬──────────┬──────────┬────────────┬─────────┐
        │ (index)  │ med     │ p95      │ p99      │ total      │ count   │
        ├──────────┼─────────┼──────────┼──────────┼────────────┼─────────┤
        │ 0.000107 │ 0.00012 │ 0.000246 │ 0.000391 │ 154.897689 │ 1000000 │
        └──────────┴─────────┴──────────┴──────────┴────────────┴─────────┘

[3axap4eHko]

@fasttime @mdjermanovic please notice that even if majority of calls are almost the same with a little fluctuations, the average performance shows that there is a difference with these two approaches, and approach with await shows the worst results, I didn't have a chance to participate that discussion, but definitely the observation of created promises and performance benchmark show the opposite of the discussion result.

[fasttime]

I think that the current notice makes sense in stating that it can be be slower to remove await. This is in line with the observed fluctuations, and with the fact that different measurements are producing different results.

Personally, I don't feel strongly about the current phrasing, and if there is an objective way to improve it, I'm sure the team will be open to make changes. But I don't think we should bother benchmarking the test cases of a deprecated rule. There is no longer a recommendation to use or avoid await because of performance in that rule, and performance tests are not needed at this time.

[mdjermanovic]

The difference in performance between return and return await in an async function seems small and could be an implementation detail. Perhaps the results would be different in another engine or another version of v8. But the new fact that return now has more ticks than return await stands, as it's dictated by the spec? So, maybe we could update the text in the deprecation notice - instead of "slower" we could say something like that removing await can now make an extra microtask, to directly state the opposite of "at the cost of an extra microtask" from the document (which wasn't using terms "faster"/"slower" anyway)?

[3axap4eHko]

@fasttime @mdjermanovic The amount of ticks does not reflect execution time or performance quality, it is just the matter on how the engine prioritizes promises resolution over promises creation.

  [ 'start', 'promise-async' ], // start
  [ 'init' ], // 1
  [ 'init' ], // 2
  [ 'init' ], // 3
  [ 'init' ], // 4
  [ 'tick', 'promise-async', 1 ], // squeezed a microtask tick
  [ 'init' ], // 5
  [ 'tick', 'promise-async', 2 ],
  [ 'tick', 'promise-async', 3 ],
  [ 'start', 'promise-async-await' ], //start
  [ 'init' ], // 1
  [ 'init' ], // 2
  [ 'init' ], // 3
  [ 'init' ], // 4
  [ 'init' ], // 5
  [ 'tick', 'promise-async-await', 1 ],
  [ 'tick', 'promise-async-await', 2 ]

As I said before both async calls have created the same amount of promises (5 promises both) internally. Also, saying at the cost is not really correct, because there is nothing to trade off. Keep in mind the fact that squeezed microtask does something meaningful in real life application, that eventually shall be done anyway, unless you just want to count these ticks. My goal by creating this issue is to make it clear for other people that there is nothing wrong by having await or avoiding await, because it does not affect performance at all and decision should be made purely on understanding what the code should accomplish. Unfortunately a lot of people seeing word slower have impression of the low performance.

[fasttime]

My goal by creating this issue is to make it clear for other people that there is nothing wrong by having await or avoiding await, because it does not affect performance at all and decision should be made purely on understanding what the code should accomplish. Unfortunately a lot of people seeing word slower have impression of the low performance.

So what change do you suggest?

[3axap4eHko]

@fasttime I would suggest to change the deprecation notice from:

This rule was deprecated in ESLint v8.46.0 with no replacement. The original intent of this rule no longer applies due to the fact JavaScript now handles native Promises differently. It can now be slower to remove await rather than keeping it. More technical information can be found in this V8 blog entry.

to This rule was deprecated in ESLint v8.46.0 without a direct replacement. The rationale behind the rule has evolved as understanding of JavaScript's native Promise handling has deepened. Specifically, concerns about the performance impact of await in try/catch blocks are often outweighed by the benefits of clearer error handling and code readability. While await may introduce a minor overhead, this is generally considered negligible compared to the assurance of properly caught and managed errors. For more in-depth analysis, refer to this this V8 blog entry.

As you can see it clarifies the importance of await in try/catch and at the same time warns about the little performance overhead it introduces.

Still this one

[mdjermanovic]

This rule does not report return await in try blocks, so mentioning try/catch in the deprecation notice would be misleading as that definitely isn't the reason why the rule is deprecated.

https://eslint.org/play/#eyJ0ZXh0IjoiLyplc2xpbnQgbm8tcmV0dXJuLWF3YWl0OiBcImVycm9yXCIqL1xuXG5hc3luYyBmdW5jdGlvbiBmb28oKSB7XG4gICAgdHJ5IHtcbiAgICAgICAgcmV0dXJuIGF3YWl0IGJhcigpOyAvLyBubyBlcnJvcnMgaGVyZVxuICAgIH0gY2F0Y2ggKGVycm9yKSB7fVxufSIsIm9wdGlvbnMiOnsiZW52Ijp7fSwicnVsZXMiOnt9LCJwYXJzZXJPcHRpb25zIjp7ImVjbWFGZWF0dXJlcyI6e30sImVjbWFWZXJzaW9uIjoibGF0ZXN0Iiwic291cmNlVHlwZSI6Im1vZHVsZSJ9fX0=

[3axap4eHko]

This is the corrected original sentence according to benchmarks, would like to keep it?

This rule was deprecated in ESLint v8.46.0 with no replacement. The original intent of this rule no longer applies due to the fact JavaScript now handles native Promises differently. It still can be slower to keep await rather than removing it. More technical information can be found in this V8 blog entry.

[fasttime]

This is the corrected original sentence according to benchmarks, would like to keep it?

This rule was deprecated in ESLint v8.46.0 with no replacement. The original intent of this rule no longer applies due to the fact JavaScript now handles native Promises differently. It still can be slower to keep await rather than removing it. More technical information can be found in this V8 blog entry.

Thanks for the update!

The performance difference in using await has been the topic of some controversial discussions in the past (see #17613). I'm afraid that changing the statement about the speed like you suggest would refuel those discussions without substantially adding to the correctness the notice: if it can be slower, it can be not. Note that the current wording aims to mitigate the impression that speed is a concern, not to reinforce it.

For this reason, I'm not in favor this change.

[3axap4eHko]

So let's just remove the sentence about remove await is slower. It's pretty clear from what I've provided already, and from the specification this is not true. Even if all my benchmarks shows better performance without await, I'm still in favor of removing misleading statement, that is cannot be proved nor by benchmarking or by spec reading. What do you think?

[JoshuaKGoldberg]

I think that the current notice makes sense in stating that it can be be slower to remove await. This is in line with the observed fluctuations, and with the fact that different measurements are producing different results.

I agree with this point, and think it has remained salient through the rest of the discussion. Saying it can be slower isn't the same as saying it is slower, or even is likely to be slower.

I'm afraid that changing the statement about the speed like you suggest would refuel those discussions without substantially adding to the correctness the notice

Agreed. Promises tend to bring up a lot of often-unnecessary bikeshedding. If there is a way to have the docs be slightly better phrased, I don't think that phrasing would be very much better, nor the long process to get there worth the time expenditure.

[3axap4eHko]

@JoshuaKGoldberg it is saying it can be slower without actual proof.