Skip to content

stream: switch to internal sleep binding#63611

Open
aduh95 wants to merge 2 commits into
nodejs:mainfrom
aduh95:sleep
Open

stream: switch to internal sleep binding#63611
aduh95 wants to merge 2 commits into
nodejs:mainfrom
aduh95:sleep

Conversation

@aduh95
Copy link
Copy Markdown
Contributor

@aduh95 aduh95 commented May 28, 2026

I don't think there was any reason to maintain two duplicate implementations, my guess is that we simply forgot about the existing util.

node/lib/internal/util.js

Lines 610 to 617 in 8d0a3b8

function sleep(msec) {
// Lazy-load to avoid a circular dependency.
if (validateUint32 === undefined)
({ validateUint32 } = require('internal/validators'));
validateUint32(msec, 'msec');
_sleep(msec);
}

The only limitation seems to be that the util only supports sleeping for 49 days

@aduh95
stream: switch to internal sleep binding
28e0eba 
Signed-off-by: Antoine du Hamel <duhamelantoine1995@gmail.com>
@nodejs-github-bot
Copy link
Copy Markdown
Collaborator

nodejs-github-bot commented May 28, 2026

Review requested:

  • @nodejs/streams

@nodejs-github-bot nodejs-github-bot added needs-ci PRs that need a full CI run. stream Issues and PRs related to the stream subsystem. labels May 28, 2026
@MattiasBuelens
Copy link
Copy Markdown
Contributor

MattiasBuelens commented May 28, 2026

The Utf8Stream code from was originally ported from SonicBoom, which depended on atomic-sleep. The atomic-sleep implementation appears to have been copied verbatim in the original PR. So indeed, this looks like a small oversight while porting.

The only limitation seems to be that the util only supports sleeping for 49 days

That shouldn't be a problem. The only usages of this helper are sleep(BUSY_WRITE_TIMEOUT) calls, and that's set to just 100ms. 😉

@MattiasBuelens MattiasBuelens added the request-ci Add this label to start a Jenkins CI on a PR. label May 28, 2026
@aduh95 aduh95 added the author ready PRs that have at least one approval, no pending requests for changes, and a CI started. label May 28, 2026
@codecov
Copy link
Copy Markdown

codecov Bot commented May 28, 2026 •
edited
Loading

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 90.29%. Comparing base (8d0a3b8) to head (c5f9088).
⚠️ Report is 2 commits behind head on main.

Additional details and impacted files 
@@           Coverage Diff           @@
##             main   #63611   +/-   ##
=======================================
  Coverage   90.29%   90.29%           
=======================================
  Files         730      730           
  Lines      234782   234763   -19     
  Branches    43953    43955    +2     
=======================================
- Hits       211993   211976   -17     
- Misses      14501    14504    +3     
+ Partials     8288     8283    -5
Files with missing lines Coverage Δ
lib/internal/streams/fast-utf8-stream.js 74.80% <100.00%> (+0.23%) ⬆️

... and 47 files with indirect coverage changes

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

@github-actions github-actions Bot removed the request-ci Add this label to start a Jenkins CI on a PR. label May 28, 2026
@nodejs-github-bot
Copy link
Copy Markdown
Collaborator

nodejs-github-bot commented May 28, 2026

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@panva panva panva approved these changes

@MattiasBuelens MattiasBuelens MattiasBuelens approved these changes

@Ethan-Arrowood Ethan-Arrowood Ethan-Arrowood approved these changes

Assignees

No one assigned

Labels

author ready PRs that have at least one approval, no pending requests for changes, and a CI started. needs-ci PRs that need a full CI run. stream Issues and PRs related to the stream subsystem.

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

5 participants

@aduh95 @nodejs-github-bot @MattiasBuelens @panva @Ethan-Arrowood