lib: ensure no holey array in fixed_queue

[jazelly]

trafficstars

Fixes: https://github.com/nodejs/node/issues/54472 Refs: https://github.com/nodejs/node/issues/54186#issuecomment-2276785038

[codecov[bot]]

Codecov Report

All modified and coverable lines are covered by tests :white_check_mark:

Project coverage is 87.33%. Comparing base (e70bd47) to head (348536f). Report is 93 commits behind head on main.

Additional details and impacted files

@@            Coverage Diff             @@
##             main   #54537      +/-   ##
==========================================
- Coverage   87.34%   87.33%   -0.01%     
==========================================
  Files         649      649              
  Lines      182524   182621      +97     
  Branches    35026    35044      +18     
==========================================
+ Hits       159420   159501      +81     
- Misses      16373    16395      +22     
+ Partials     6731     6725       -6     

Files with missing lines	Coverage Δ
lib/internal/fixed_queue.js	100.00% <100.00%> (ø)

... and 48 files with indirect coverage changes

[avivkeller]

Hi! Could you amend your commit message to begin with an active verb (after the subsystem)?

[jakecastelli]

I have run the benchmark for async_hooks and events, here are the results:

events: https://ci.nodejs.org/view/Node.js%20benchmark/job/benchmark-node-micro-benchmarks/1616/ async_hooks: https://ci.nodejs.org/view/Node.js%20benchmark/job/benchmark-node-micro-benchmarks/1615/

Does not look like this change would cause any performance regression. But would be happy to know if any benchmark that is missed.

[jazelly]

PR comments addressed. I have also updated the data structure comments in fixed_queue as it's no longer with [empty]

[nodejs-github-bot]

CI: https://ci.nodejs.org/job/node-test-pull-request/61462/

[nodejs-github-bot]

CI: https://ci.nodejs.org/job/node-test-pull-request/61463/

[nodejs-github-bot]

CI: https://ci.nodejs.org/job/node-test-pull-request/61465/

[nodejs-github-bot]

CI: https://ci.nodejs.org/job/node-test-pull-request/61469/ 💚

[jakecastelli]

@joyeecheung @targos @benjamingr PTAL 🙏

[benjamingr]

I'm approving this even without a test but a test would be nice

[jazelly]

@benjamingr see this thread. I think (and I agree) the idea is that we don't want to show it as a wanted behaviour. We fix it to avoid hard crashes.

[jakecastelli]

a test would be nice

Thinking about this again, maybe we should add a test to the test/known_issues and put refs on the top. In this way, folks who read the test would not think this is a supported behaviour. Wdyt @jazelly

[jazelly]

Oh, I didn't know there are test/known_issues. I think it makes sense to place it there with a comment.

[jazelly]

Actually, this may not be a known issue anymore - especially with this patch. I added a test case in test-fixed-queue.js to assert the queue no longer being holey. PTAL

[nodejs-github-bot]

CI: https://ci.nodejs.org/job/node-test-pull-request/61522/

[nodejs-github-bot]

CI: https://ci.nodejs.org/job/node-test-pull-request/61528/ 💚

[jakecastelli]

Do you have any objections on this one? @joyeecheung @legendecas 👀

[nodejs-github-bot]

Landed in 981c7014009eee456bdc70adf4d9d5c89211cafe