rushstack
rushstack copied to clipboard
fix(node-core-library): iterator weighting isn't fully respected by `Async#forEachAsync`
Summary
Followup to #4672. This PR fixes 2 bugs that I found while trying to integrate this with our CI,
- operation weight isn't passed correctly to the execution record.
- operation weight wasn't being respected if operations were queued at the same time.
Before
After
(a,b,c all have operation weight 10)
Details
The existing AsyncQueueManager requires the forEachAsync implementation to be reentrant and to have multiple waiting iterators. Moving to a weighted operation model requires the forEachAsync to lock when pulling off an item from the iterator as we don't know the operation weight until after we pop the item. To correct this difference, I updated AsyncQueueManager to explicitly reassign operations when marking the operation as complete or after handling a remote executing operation.
This may affect performance, but the critical zone that is now being locked should not affect performance. As seen in the graphs above, this does change the REMOTE_EXECUTING behavior a bit.
How it was tested
I tested this using the cobuilds build-tests sandbox repo and a bunch of console logs. I changed the operation weights to equal the -p parameter which caused sequential timelines.
I would appreciate additional testing/validation to make sure I'm on the right track 🙏