Avoid AsyncResource.bind #83
Conversation
How did you confirm that the context is not switched in this case? |
I think it is confirmed by the fact that the test case "propagates async execution context properly" passed. https://github.com/sugoroku-y/p-limit-avoid-async-resource/blob/main/test.js#L44-L59 |
|
// @hyperair |
|
Let me explain what I understand: import { AsyncLocalStorage } from 'async_hooks';
import pLimit from 'p-limit';
const limit = pLimit(2);
const store = new AsyncLocalStorage();
const result = await Promise.all(
Array.from({ length: 100 }, async (_, id) =>
store.run({ id }, () =>
limit(() => Promise.resolve(store.getStore().id)),
),
),
);
console.log(result);When this script is executed with the original However, if you edit This happens because the function passed to To avoid this, With my changed So it is no longer necessary to wrap it in If you execute the previous script with my modified You can see that it is executed in the assumed asynchronous context without wrapping with |
|
Looks good to me, but the |
Is it the following code that seems weird to you? starter = (async () => {
await starter;
if (activeCount < concurrency && queue.size > 0) {
queue.dequeue()();
}
})();This part of the Promise chain is used to delay the comparison of activeCount and concurrency. If it were written more like a Promise chain, it would look like this: starter = starter.then(() => {
if (activeCount < concurrency && queue.size > 0) {
queue.dequeue()();
}
});As mentioned in the original comment, it is necessary to insert an await because the Furthermore, since the |
|
Although this would be a major change, it may be easier to understand if you do the following: const resumeNext = () => {
const resolve = queue.dequeue();
if (!resolve) {
return;
}
resolve();
// Since `pendingCount` has been decreased by one, increase `activeCount` by one.
++activeCount;
};
const pending = () => {
(async () => {
// This function needs to wait until the next microtask before comparing
// `activeCount` to `concurrency`, because `activeCount` is updated asynchronously
// after the resolve function is dequeued and called. The comparison in the if-statement
// needs to happen asynchronously as well to get an up-to-date value for `activeCount`.
await Promise.resolve();
if (activeCount < concurrency) {
resumeNext();
}
})();
// Queue `resolve` function instead of `function_` function
// to preserve asynchronous context.
return new Promise(resolve => {
queue.enqueue(resolve);
});
};
const generator = async (function_, ...arguments_) => {
await pending();
try {
return await function_(...arguments_);
} finally {
activeCount--;
resumeNext();
}
};Yet the weirdness that @hyperair feels may not have lessened still. 2024-06-23 Postscript: Changed to apply 0ca399d. 2024-06-24 Postscript: |
…tiveCount`. And eliminate `starter`.
|
Oh yeah that looks great now, thanks! |
|
I merged
|
|
@sugoroku-y This is not a breaking change, correct? |
There's the fact that I don't think it's a major issue, but I have my suspicions about that could be considered a breaking change. import pLimit from 'p-limit';
const limit = pLimit(3);
Promise.all(Array.from({length: 10}, (_, i) => limit(async (id) => {
console.log(id, 'start', limit.activeCount, limit.pendingCount);
await Promise.resolve()
console.log(id, 'end', limit.activeCount, limit.pendingCount);
}, i))).then(() => console.log('end'))The above script, when run with the original version of After the change, it would be as follows: The values of activeCount and pendingCount are changing for two tasks that are immediately executed. |
|
Thank you 👍 |
Change to
new Promise(r => queue.enqueue(r)).then(...)so that the asynchronous context is not switched, makingAsyncResource.bindunnecessary.Since
AsyncResource.bindis no longer needed, the time required to call the limit function will be reduced.However, because of the change in the way Promise is used, the timing of changes such as
limit.activeCountwill be slower than before the change, which may be fatal if the usage situation is severe.Performance
Before:
After:
Source Code: