New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Docs: no-return-await deprecation notice leads to a wrong conclusion #18166
Comments
Hi @3axap4eHko, thanks for the issue. Could you point out exactly what you consider wrong in the deprecation notice of the For context, the deprecation paragraph was added following the discussion in #17345, the rest of the documentation about that rule has not changed. |
@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 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. |
Thanks for the clarification. I'm not sure if this sentence hits the nail in the head:
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 |
Note that this rule does not report // 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. |
const promise = Promise.resolve(42);
async function foo() {
return promise;
}
console.log(foo() === promise); // false |
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 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. |
My understanding of the new version of the deprecation notice proposed in #18166 (comment) is that the rule is deprecated because 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, |
@3axap4eHko what do you think? Does @mdjermanovic's analysis make sense to you or are we missing something? |
If |
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 |
Exactly! And reading the article from the documentation does not make it clear why. |
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 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
|
@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 |
I think that the current notice makes sense in stating that it can be be slower to remove 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 |
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)? |
@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 |
So what change do you suggest? |
Still this one |
This rule does not report |
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 For this reason, I'm not in favor this change. |
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? |
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.
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. |
@JoshuaKGoldberg it is saying it can be slower without actual proof. |
I also don't like the part where it says that removing |
Agreed, we should update the deprecation text to say that the noted cost of the extra microtask no longer exists, and remove the word "slower".
My understanding is that the ECMAScript spec required the extra microtask at the time the rule was created (and that V8 didn't actually have that extra microtask, which was considered a bug but eventually became correct behavior when the spec was changed). |
@3axap4eHko This issue has been accepted. Since you are the initiator, would you be willing to create a pull request with the suggested changes? |
Docs page(s)
https://eslint.org/docs/latest/rules/no-return-await
What documentation issue do you want to solve?
The documentation implies that removing await can lower performance and refers to an article that says for every await is created a microtask. According to the specification:
Let's take a look at examples
In this example, each function
(a, b, c)
is marked as async, which means their return values are automatically wrapped in aPromise
if not already a promise. SincefetchSomething()
presumably returns a promise,a
,b
, andc
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 additionalawait
operations, the overhead is minimized to the handling of the original promise fromfetchSomething()
. The key point here is that the use of async alone does not introduce unnecessary microtasks; it's the handling of promises within these functions that matters.Here, by introducing
await
within each function, the JavaScript engine is forced to wait for the promise returned byfetchSomething()
to resolve before proceeding to the next operation, even though directly returning the promise would achieve the same effect. Eachawait
introduces a pause, requiring the engine to handle each resolution as a separate microtask. This means that for eachawait
, there's an associated microtask for the promise resolution. The introduction ofexplicit await
statements in this chain creates additional points at which the engine must manage promise resolutions, potentially increasing the number of microtasks and, as a result, adding overhead. This is because eachawait
unnecessarily adds a layer of promise resolution that must be managed, even though the direct chaining of promises (as in the first example) would suffice.Each
async
function declaration implies that the function's return value will be wrapped in a promise, introducing microtasks for their resolutions. However, the second example with explicitawait
use in each function introduces additional microtasks due to the explicit pause and wait for each promise to resolve before proceeding.What do you think is the correct solution?
That said, using return await inside a try/catch block is a deliberate decision that prioritizes correct error handling over potential performance concerns. In this context, the performance impact is often considered negligible compared to the benefit of ensuring errors are caught and handled properly. The choice to use return await in such scenarios should indeed be meaningful and based on the specific requirements of the code rather than following a rule blindly.
Participation
Additional comments
No response
The text was updated successfully, but these errors were encountered: