[node-core-library] Fix weighted oversubscription in forEachAsync #1
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
ethanburrelldd
commented
Sep 11, 2025
| let concurrentUnitsInProgress: number = 0; | ||
|
|
||
| const iterator: Iterator<TEntry> | AsyncIterator<TEntry> = (iterable as AsyncIterable<TEntry>)[ | ||
| const baseIterator: Iterator<TEntry> | AsyncIterator<TEntry> = (iterable as AsyncIterable<TEntry>)[ |
Owner
Author
There was a problem hiding this comment.
Q: does this need to be of type Iterator<TEntry> | AsyncIterator<TEntry>? it looks like iterable is of type AsyncIterable<TEntry>,?
Can we simplify so we only have to contain types for AsyncIterator?
ethanburrelldd
commented
Sep 11, 2025
| expect(fn).toHaveBeenNthCalledWith(3, 3, 2); | ||
| }); | ||
|
|
||
| it('returns the same result as built-in Promise.all', async () => { |
Owner
Author
There was a problem hiding this comment.
duplicated test case
ethanburrelldd
changed the title
ethanburrelldd
force-pushed
the
eb/concurrency-bug-fix
branch
from
36c1370 to
18960cd
Compare
…ID (microsoft#5364) * Initial plan * Replace uuid package with Node.js built-in crypto.randomUUID Co-authored-by: bmiddha <5100938+bmiddha@users.noreply.github.com> * Add rush change file for uuid package replacement Co-authored-by: bmiddha <5100938+bmiddha@users.noreply.github.com> --------- Co-authored-by: copilot-swe-agent[bot] <198982749+Copilot@users.noreply.github.com> Co-authored-by: bmiddha <5100938+bmiddha@users.noreply.github.com>
…soft#5363) * Add edge case for projects with duplicate names * Replace custom interfaces with official PNPM types; temporarily remove the V6 kludge * Add lockfilePath.ts utility that can completely eliminate `@lifaon/path` * Completed rewrite of 5.4 loader logic * Remove "@lifaon/path" dependency * Splite createLockfileEntry() into createProjectLockfileEntry() and createPackageLockfileEntry() * Upgrade to use @pnpm/lockfile.types@1002.0.1 * Implement support for lockfile version 6.0 * Hide the "." package from Rush workspaces, shuffling the jsonId's in all the snapshots * Fix an incorrect path * rush change * Improve formatting of 6.0 suffixes * Move entry to nonbrowser-approved-packages.json * Add a failing test case for "link:" in packages section * PR feedback * PR feedback: don't try to resolve "link:" under packages section * rush update
Co-authored-by: David Michon <dmichon-msft@users.noreply.github.com>
* [rush-serve] Add dependencies to operations, support abort * Adjust "silent" field in socket --------- Co-authored-by: David Michon <dmichon-msft@users.noreply.github.com>
…ighlighter (microsoft#5366) * Enable syntax highlighting for pnpmfile.cjs tab * Highlight package.json as well * Introduce PnpmfileRunner.ts to isolate .pnpmfile.cjs execution * rush change * Disable webpack bundle size limits, since this app is served from localhost * In a Rush repo, the ".pnpmfile.cjs" tab should show Rush's file, not the generated temp file * Upgrade Prettier to support `async using` * prettier -w . * PR feedback: use "await using" * PR feedback * PR feedback * rush change * PR feedback * Fix build break * Revert `[Symbol.asyncDispose]()` because it isn't supported by Node 18
…lse; improve docs
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Summary
Fixes concurrency bug where weighted operations could exceed the specified parallelism limit.
Details
In the current implementation with
maxConcurrency = 8andconcurrentUnitsInProgress = 4, a task withweight = 8could be queued due to no check existing to prevent queueing this new task. This change waits for enough free units before queueing the weight-8 task.This oversubscription causes test flakes and slower execution due to too many tasks running simultaneously.
Changes:
How it was tested
rush-redis-cobuild-plugin-integration-testImpacted documentation