rushstack icon indicating copy to clipboard operation
rushstack copied to clipboard

fix(node-core-library): iterator weighting isn't fully respected by `Async#forEachAsync`

Open aramissennyeydd opened this issue 1 year ago • 0 comments

Summary

Followup to #4672. This PR fixes 2 bugs that I found while trying to integrate this with our CI,

  1. operation weight isn't passed correctly to the execution record.
  2. operation weight wasn't being respected if operations were queued at the same time.

Before

Screenshot 2024-05-07 at 12 24 01 PM

After

(a,b,c all have operation weight 10) Screenshot 2024-05-08 at 7 44 31 PM

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 🙏

Impacted documentation

aramissennyeydd avatar May 08 '24 23:05 aramissennyeydd