Skip to content

streams: fix fd is null when calling clearBuffer - #50994

Merged
nodejs-github-bot merged 1 commit into
nodejs:mainfrom
kylo5aby:fix/streams
Dec 7, 2023
Merged

streams: fix fd is null when calling clearBuffer#50994
nodejs-github-bot merged 1 commit into
nodejs:mainfrom
kylo5aby:fix/streams

Conversation

@kylo5aby

@kylo5abykylo5aby commented Dec 1, 2023

Copy link
Copy Markdown
Contributor

Fixes: #50979

@nodejs-github-bot

nodejs-github-bot commented Dec 1, 2023

Copy link
Copy Markdown
Collaborator

Review requested:

  • @nodejs/streams

@nodejs-github-botnodejs-github-bot added the needs-ci PRs that need a full CI run. label Dec 1, 2023
mcollina
mcollina approved these changes Dec 1, 2023
View reviewed changes

@mcollinamcollina 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.

lgtm

mcollina
mcollina requested changes Dec 1, 2023
View reviewed changes

@mcollinamcollina 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.

Can you please add a test?

@kylo5aby

kylo5aby commented Dec 3, 2023

Copy link
Copy Markdown
ContributorAuthor

Can you please add a test?

Resolved. PTAL

rluvaton
rluvaton reviewed Dec 3, 2023
View reviewed changes

@rluvatonrluvaton 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.

Thank you for your contribution

Comment threadtest/parallel/test-file-write-stream5.js Outdated
Comment on lines 18 to 28

@rluvatonrluvatonDec 3, 2023

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.

Suggested change
fileWriteStream.on('finish',()=>{
constwrittenData=fs.readFileSync(filepath,'utf8');
assert.strictEqual(writtenData,data);
});
fileWriteStream.on('finish',common.mustCall(()=>{
constwrittenData=fs.readFileSync(filepath,'utf8');
assert.strictEqual(writtenData,data);
}));

Comment threadtest/parallel/test-file-write-stream5.js Outdated
ronag
ronag reviewed Dec 3, 2023
View reviewed changes
Comment threadlib/internal/streams/writable.js Outdated
Comment on lines 736 to 737

@ronagronagDec 3, 2023

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.

Suggested change
if((state[kState]&(kDestroyed|kBufferProcessing|kCorked|kBuffered))!==kBuffered||
(state[kState]&(kDestroyed|kBufferProcessing|kCorked|kConstructed))!==kConstructed){
if((state[kState]&(kDestroyed|kBufferProcessing|kCorked|kBuffered|kConstructed))!==(kBuffered|kConstructed)){

@kylo5aby

kylo5aby commented Dec 4, 2023

Copy link
Copy Markdown
ContributorAuthor

Resolved

@ronagronag added the request-ci Add this label to start a Jenkins CI on a PR. label Dec 4, 2023
@github-actionsgithub-actionsBot removed the request-ci Add this label to start a Jenkins CI on a PR. label Dec 4, 2023
@nodejs-github-bot

nodejs-github-bot commented Dec 4, 2023

Copy link
Copy Markdown
Collaborator

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

mcollina
mcollina approved these changes Dec 4, 2023
View reviewed changes

@mcollinamcollina 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.

lgtm

@rluvatonrluvaton added author ready PRs that have at least one approval, no pending requests for changes, and a CI started. request-ci Add this label to start a Jenkins CI on a PR. labels Dec 6, 2023
@github-actionsgithub-actionsBot removed the request-ci Add this label to start a Jenkins CI on a PR. label Dec 6, 2023
@nodejs-github-bot

nodejs-github-bot commented Dec 6, 2023

Copy link
Copy Markdown
Collaborator

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

@mcollinamcollina added the commit-queue Add this label to land a pull request using GitHub Actions. label Dec 7, 2023
@nodejs-github-botnodejs-github-bot removed the commit-queue Add this label to land a pull request using GitHub Actions. label Dec 7, 2023
@nodejs-github-bot
nodejs-github-bot merged commit 639c366 into nodejs:mainDec 7, 2023
@nodejs-github-bot

nodejs-github-bot commented Dec 7, 2023

Copy link
Copy Markdown
Collaborator

Landed in 639c366

RafaelGSS pushed a commit that referenced this pull request Dec 15, 2023
@kylo5aby@RafaelGSS
stream: fix fd is null when calling clearBuffer
5112306
PR-URL: #50994Fixes: #50979
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com>
Reviewed-By: Raz Luvaton <rluvaton@gmail.com>
Reviewed-By: Robert Nagy <ronagy@icloud.com>
@RafaelGSSRafaelGSS mentioned this pull request Dec 15, 2023
Merged
@SamTV12345

SamTV12345 commented Dec 23, 2023

Copy link
Copy Markdown

@mcollina Is this already in the latest release? I am still getting the error.

@mcollina

mcollina commented Dec 23, 2023

Copy link
Copy Markdown
Member

this should be in v21.5.0.

@SamTV12345

SamTV12345 commented Dec 23, 2023

Copy link
Copy Markdown

Thanks. 21 works. 20 doesn't but isn't a problem as I could simply update to 21.

richardlau pushed a commit that referenced this pull request Mar 25, 2024
@kylo5aby@richardlau
stream: fix fd is null when calling clearBuffer
8750070
PR-URL: #50994Fixes: #50979
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com>
Reviewed-By: Raz Luvaton <rluvaton@gmail.com>
Reviewed-By: Robert Nagy <ronagy@icloud.com>
@richardlaurichardlau mentioned this pull request Mar 25, 2024
Merged
Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@mcollinamcollinamcollina approved these changes
@benjamingrbenjamingrbenjamingr approved these changes
@ronagronagronag approved these changes
@rluvatonrluvatonrluvaton approved these changes

Assignees

No one assigned

Labels

author readyPRs that have at least one approval, no pending requests for changes, and a CI started.needs-ciPRs that need a full CI run.

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

The "fd" argument must be of type number. Received null - Erroring in recent Node versions 20 and 21

7 participants

@kylo5aby@nodejs-github-bot@SamTV12345@mcollina@benjamingr@ronag@rluvaton