GV

GeezerVison

The World Wide Web, restored to 800 pixels of glory.

Home|About|Gaming cabinet|Archives|How it works|Starter sites

On this site

  • Sign in
  • ActionsCI/CD and automation
  • CodespacesCloud development environments
  • All Features
  • Documentation
  • Training
  • Certifications
  • Build with Copilot
  • Customize Copilot
  • Integrations
  • Trending repos
  • GitHub Sponsors
  • Developer articles
  • Enterprises

This view

  • Original page
  • New address

GeezerVison is showing github.com — streams: fix fd is null when calling clearBuffer by kylo5aby · Pull Request #50994 · nodejs/node

Conversation

@kylo5aby

@kylo5aby kylo5aby commented Dec 1, 2023

Copy link
Copy Markdown
Contributor

Fixes: #50979

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
bricss reacted with thumbs up emoji
All reactions
  • 👍 1 reaction
@nodejs-github-bot

nodejs-github-bot commented Dec 1, 2023

Copy link
Copy Markdown
Collaborator

Review requested:

  • @nodejs/streams
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@nodejs-github-bot nodejs-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

@mcollina mcollina left a comment

Copy link
Copy Markdown
Member

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

lgtm

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
mcollina
mcollina requested changes Dec 1, 2023
View reviewed changes

@mcollina mcollina left a comment

Copy link
Copy Markdown
Member

There was a problem hiding this comment.

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?

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
anonrig reacted with thumbs up emoji
All reactions
  • 👍 1 reaction
@kylo5aby
kylo5aby force-pushed the fix/streams branch from bc2a89b to 7b5c8e1 Compare December 3, 2023 16:36
@kylo5aby

kylo5aby commented Dec 3, 2023

Copy link
Copy Markdown
Contributor Author

Can you please add a test?

Resolved. PTAL
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
rluvaton
rluvaton reviewed Dec 3, 2023
View reviewed changes

@rluvaton rluvaton left a comment

Copy link
Copy Markdown
Member

There was a problem hiding this comment.

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

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
Comment thread test/parallel/test-file-write-stream5.js Outdated
Comment on lines 18 to 28

@rluvaton rluvaton Dec 3, 2023

Copy link
Copy Markdown
Member

There was a problem hiding this comment.

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', () => {
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);
}));

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
Comment thread test/parallel/test-file-write-stream5.js Resolved

Uh oh!

There was an error while loading. Please reload this page.
ronag
ronag reviewed Dec 3, 2023
View reviewed changes
Comment thread lib/internal/streams/writable.js Outdated
Comment on lines 736 to 737

@ronag ronag Dec 3, 2023

Copy link
Copy Markdown
Member

There was a problem hiding this comment.

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)) {

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
benjamingr reacted with thumbs up emoji
All reactions
  • 👍 1 reaction
benjamingr
benjamingr approved these changes Dec 3, 2023
View reviewed changes
@kylo5aby
stream: fix fd is null when calling clearBuffer
e0d8242
@kylo5aby
kylo5aby force-pushed the fix/streams branch from 7b5c8e1 to e0d8242 Compare December 4, 2023 03:01
@kylo5aby

kylo5aby commented Dec 4, 2023

Copy link
Copy Markdown
Contributor Author

Resolved
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@ronag ronag added the request-ci Add this label to start a Jenkins CI on a PR. Only starts once the PR has an approving review. label Dec 4, 2023
@github-actions github-actions Bot removed the request-ci Add this label to start a Jenkins CI on a PR. Only starts once the PR has an approving review. 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/
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
mcollina
mcollina approved these changes Dec 4, 2023
View reviewed changes

@mcollina mcollina left a comment

Copy link
Copy Markdown
Member

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

lgtm

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
rluvaton
rluvaton approved these changes Dec 5, 2023
View reviewed changes
ronag
ronag approved these changes Dec 6, 2023
View reviewed changes
@rluvaton rluvaton added author ready PRs with CI started, the required approvals, and no outstanding review comments. request-ci Add this label to start a Jenkins CI on a PR. Only starts once the PR has an approving review. labels Dec 6, 2023
@github-actions github-actions Bot removed the request-ci Add this label to start a Jenkins CI on a PR. Only starts once the PR has an approving review. 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/
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@mcollina mcollina added the commit-queue PRs queued for automated landing through the Commit Queue. label Dec 7, 2023
@nodejs-github-bot nodejs-github-bot removed the commit-queue PRs queued for automated landing through the Commit Queue. label Dec 7, 2023
@nodejs-github-bot
nodejs-github-bot merged commit 639c366 into nodejs:main Dec 7, 2023
@nodejs-github-bot

nodejs-github-bot commented Dec 7, 2023

Copy link
Copy Markdown
Collaborator

Landed in 639c366
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
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: #50994
Fixes: #50979
Reviewed-By: Matteo Collina <[email protected]>
Reviewed-By: Benjamin Gruenbaum <[email protected]>
Reviewed-By: Raz Luvaton <[email protected]>
Reviewed-By: Robert Nagy <[email protected]>
@RafaelGSS RafaelGSS mentioned this pull request Dec 15, 2023
v21.5.0 proposal #51166
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.
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@mcollina

mcollina commented Dec 23, 2023

Copy link
Copy Markdown
Member

this should be in v21.5.0.
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@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.
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
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: #50994
Fixes: #50979
Reviewed-By: Matteo Collina <[email protected]>
Reviewed-By: Benjamin Gruenbaum <[email protected]>
Reviewed-By: Raz Luvaton <[email protected]>
Reviewed-By: Robert Nagy <[email protected]>
@richardlau richardlau mentioned this pull request Mar 25, 2024
v20.12.0 proposal #52212
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

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

Uh oh!

There was an error while loading. Please reload this page.

Assignees

No one assigned

Labels

author ready PRs with CI started, the required approvals, and no outstanding review comments. 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

Uh oh!

There was an error while loading. Please reload this page.

7 participants

@kylo5aby @nodejs-github-bot @SamTV12345 @mcollina @benjamingr @ronag @rluvaton
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
You are visitor 01402503 — GeezerVison does not store the pages you look up.
Best viewed with Netscape Navigator 4.7 or Internet Explorer 6 at 800×600. No cookies required.