Skip to content

fix: set kBodyUsed only once#4592

Closed
Uzlopak wants to merge 1 commit into
mainfrom
only-once
Closed

fix: set kBodyUsed only once#4592
Uzlopak wants to merge 1 commit into
mainfrom
only-once

Conversation

@Uzlopak

@Uzlopak Uzlopak commented Sep 27, 2025

Copy link
Copy Markdown
Contributor

@codecov-commenter

codecov-commenter commented Sep 27, 2025

Copy link
Copy Markdown

Codecov Report

❌ Patch coverage is 33.33333% with 2 lines in your changes missing coverage. Please review.
✅ Project coverage is 92.94%. Comparing base (7321451) to head (68e70d6).
⚠️ Report is 209 commits behind head on main.

Files with missing lines Patch % Lines
lib/core/util.js 33.33% 2 Missing ⚠️
Additional details and impacted files
@@           Coverage Diff           @@
##             main    #4592   +/-   ##
=======================================
  Coverage   92.94%   92.94%           
=======================================
  Files         106      106           
  Lines       32973    32970    -3     
=======================================
- Hits        30646    30644    -2     
+ Misses       2327     2326    -1     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 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.

@Uzlopak

Uzlopak commented Sep 28, 2025

Copy link
Copy Markdown
Contributor Author

@metcoder95

There is no code coverage. And i guess we can remove this code, because https://nodejs.org/api/stream.html#readablereadabledidread is existing since v14.

Comment thread lib/core/util.js
body[kBodyUsed] = false
EE.prototype.on.call(body, 'data', function () {
this[kBodyUsed] = true
body.once('data', () => {

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I believe the initial intention was to avoid tampering, but given that every stream is EE based, it shouldn’t be a problem (bad stream, bad implementation, up to caller).

I’m just more into keep it with this instead of creating another closure; the rest LGTM

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is wrong. It's not because of tampering but because otherwise the stream will start when registering a data listener.

@mcollina mcollina left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That code was added to avoid some edge case that could happen if on() was overwritten. (Like streams do)

I wonder why that code was added without a test.

@Uzlopak can you verify who added this?

@ronag do you recall?

@ronag ronag left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This breaks

@ronag

ronag commented Oct 18, 2025

Copy link
Copy Markdown
Member

Streams will start the stream if on is called with 'data' that's why the EE prototype is used.

@Uzlopak

Uzlopak commented Oct 18, 2025

Copy link
Copy Markdown
Contributor Author

Ok, but the thing is, that the code is not covered by unit tests.

@ronag

ronag commented Oct 19, 2025

Copy link
Copy Markdown
Member

Would be nice to have that fixed.

@gdanigarcia06-creator gdanigarcia06-creator left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ayuda

@ronag ronag left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is breaking

@Uzlopak Uzlopak closed this Feb 5, 2026
@Uzlopak Uzlopak deleted the only-once branch February 5, 2026 10:58
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

6 participants