FazBrowse GitHub Viewer | Trending |
URL:
| Home
Tools: [Download Repo ZIP]   [Original HTTPS Page]

streams: fix fd is null when calling clearBuffer by kylo5aby · Pull Request #50994 · nodejs/node · GitHub

/ node Public

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
ConversationCommits (1)ChecksFiles changed
Merged

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

Conversation

kylo5aby commented Dec 1, 2023

Copy link
Copy Markdown
Contributor

Fixes: #50979

nodejs-github-bot commented Dec 1, 2023

Copy link
Copy Markdown
Collaborator

Review requested:

  • @nodejs/streams

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

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.

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

lgtm

mcollina requested changes Dec 1, 2023
View reviewed changes

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.

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

Can you please add a test?

kylo5aby commented Dec 3, 2023

Copy link
Copy Markdown
Contributor Author

Can you please add a test?

Resolved. PTAL

rluvaton reviewed Dec 3, 2023
View reviewed changes

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

Thank you for your contribution

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

rluvaton Dec 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.

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality
Suggested change
fileWriteStream.on('finish', () => {
const writtenData = fs.readFileSync(filepath, 'utf8');
assert.strictEqual(writtenData, data);
});
fileWriteStream.on('finish', common.mustCall(() => {
const writtenData = fs.readFileSync(filepath, 'utf8');
assert.strictEqual(writtenData, data);
}));

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

ronag Dec 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.

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality
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 commented Dec 4, 2023

Copy link
Copy Markdown
Contributor Author

Resolved

ronag added the request-ci Add this label to start a Jenkins CI on a PR. label Dec 4, 2023
github-actions Bot removed the request-ci Add this label to start a Jenkins CI on a PR. label Dec 4, 2023

nodejs-github-bot commented Dec 4, 2023

Copy link
Copy Markdown
Collaborator

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

mcollina approved these changes Dec 4, 2023
View reviewed changes

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.

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

lgtm

rluvaton added author ready PRs that have at least one approval, no outstanding review comments, and a CI started. request-ci Add this label to start a Jenkins CI on a PR. labels Dec 6, 2023
github-actions Bot removed the request-ci Add this label to start a Jenkins CI on a PR. label Dec 6, 2023

nodejs-github-bot commented Dec 6, 2023

Copy link
Copy Markdown
Collaborator

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

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

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
stream: fix fd is null when calling clearBuffer
5112306 
PR-URL: #50994
Fixes: #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>
RafaelGSS mentioned this pull request Dec 15, 2023
Merged

SamTV12345 commented Dec 23, 2023

Copy link
Copy Markdown

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

mcollina commented Dec 23, 2023

Copy link
Copy Markdown
Member

this should be in v21.5.0.

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
stream: fix fd is null when calling clearBuffer
8750070 
PR-URL: #50994
Fixes: #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>
richardlau mentioned this pull request Mar 25, 2024
Merged
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters. Learn more about bidirectional Unicode characters
Show hidden characters
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

mcollina mcollina approved these changes
benjamingr benjamingr approved these changes
ronag ronag approved these changes
rluvaton rluvaton approved these changes

Assignees

No one assigned

Labels

author ready PRs that have at least one approval, no outstanding review comments, and a CI started. needs-ci PRs 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

Back | FazBrowse Home | New Git URL