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 — stream: don't flush destroyed writable by ronag · Pull Request #29028 · nodejs/node

Conversation

@ronag

@ronag ronag commented Aug 6, 2019 •
edited
Loading

Uh oh!

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

Copy link
Copy Markdown
Member

It doesn't make much sense (and is a bit weird) to flush a stream which has been destroyed.

I need help creating a relevant test for this.

Checklist
  • make -j4 test (UNIX), or vcbuild test (Windows) passes
  • tests and/or benchmarks are included
  • documentation is changed or added
  • commit message follows commit guidelines

NOTE TO SELF: Look into needFinish & stream destroyed. errorOrDestroy added in this PR should preferably be async.

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 added the stream Issues and PRs related to Node.js streams. label Aug 6, 2019
@ronag
ronag force-pushed the stream-flush-destroýed-writable branch 4 times, most recently from 3b3305a to 481e6d1 Compare August 6, 2019 20:20
@ronag ronag mentioned this pull request Aug 6, 2019
[x] stream: simplify writable buffering #29026
Closed
4 tasks done
mscdex
mscdex reviewed Aug 6, 2019
View reviewed changes
Comment thread lib/_stream_writable.js Resolved

Uh oh!

There was an error while loading. Please reload this page.
@ronag
ronag force-pushed the stream-flush-destroýed-writable branch from 481e6d1 to 44d3d57 Compare August 6, 2019 23:00
@jasnell
jasnell requested review from mafintosh and mcollina August 7, 2019 00:26
jasnell
jasnell approved these changes Aug 7, 2019
View reviewed changes
@jasnell

jasnell commented Aug 7, 2019

Copy link
Copy Markdown
Member

Hmm.. I'm thinking this is more bug fix than breaking change. What do you think @mcollina?
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
mcollina
mcollina requested changes Aug 7, 2019
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.

This will need a unit test.

I'm happy to land this as a bug fix as long as it does not break anything on CITGM.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
@mafintosh

mafintosh commented Aug 7, 2019

Copy link
Copy Markdown
Member

def bugfix
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@nodejs-github-bot

nodejs-github-bot commented Aug 7, 2019

Copy link
Copy Markdown
Collaborator

CI: https://ci.nodejs.org/job/node-test-pull-request/24881/
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@ronag
ronag force-pushed the stream-flush-destroýed-writable branch from 44d3d57 to 7332cb5 Compare August 7, 2019 09:50
@ronag

ronag commented Aug 7, 2019

Copy link
Copy Markdown
Member Author

test added and fixed
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@ronag
ronag force-pushed the stream-flush-destroýed-writable branch from 7332cb5 to 0906aa6 Compare August 7, 2019 09:56
ronag
ronag commented Aug 7, 2019
View reviewed changes
Comment thread lib/_stream_writable.js Resolved

Uh oh!

There was an error while loading. Please reload this page.
@ronag
ronag force-pushed the stream-flush-destroýed-writable branch 2 times, most recently from f0ea5f3 to 372588a Compare August 7, 2019 09:58
mcollina
mcollina reviewed Aug 7, 2019
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.

I would still consider this a bugfix, but we should:

  1. verify that CITGM passes
  2. let it bake for LTS (10.x) for some time before backporting.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
jasnell reacted with thumbs up emoji
All reactions
  • 👍 1 reaction
Comment thread test/parallel/test-stream-write-destroy.js Resolved

Uh oh!

There was an error while loading. Please reload this page.
@nodejs-github-bot

nodejs-github-bot commented Aug 7, 2019

Copy link
Copy Markdown
Collaborator

CI: https://ci.nodejs.org/job/node-test-pull-request/24882/
All reactions

Sorry, something went wrong.

Uh oh!

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

mcollina commented Aug 7, 2019

Copy link
Copy Markdown
Member

CITGM: https://ci.nodejs.org/view/Node.js-citgm/job/citgm-smoker/1932/
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@ronag
ronag force-pushed the stream-flush-destroýed-writable branch from 372588a to e1542b7 Compare August 7, 2019 12:44
@ronag

ronag commented Aug 7, 2019 •
edited
Loading

Uh oh!

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

Copy link
Copy Markdown
Member Author

@mcollina: Sorry, found further issues.

Consider the updated tests. We now will throw ERR_STREAM_DESTROYED on some tests that assume ERR_STREAM_WRITE_AFTER_END also we now send the err on the write callback and don't emit it as an error on the stream.

How should this be?
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
addaleax
addaleax reviewed Aug 7, 2019
View reviewed changes

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

I wouldn’t mind being careful and labelling this semver-major.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
Comment thread lib/_stream_writable.js Resolved

Uh oh!

There was an error while loading. Please reload this page.
@ronag
ronag force-pushed the stream-flush-destroýed-writable branch from e1542b7 to d7e8b4c Compare August 7, 2019 14:14
@ronag

ronag commented Aug 7, 2019 •
edited
Loading

Uh oh!

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

Copy link
Copy Markdown
Member Author

Ok, I think everything (expect @mcollina's comment on !err && chinksWritten++) is fixed in a sensical manner.
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@ronag ronag mentioned this pull request Aug 8, 2019
stream: Writable should not invoke _destroy while pending ops #29044
Closed
This was referenced Jan 20, 2021
[Snyk] Security upgrade gatsby from 2.13.73 to 2.31.0 meghasfdc/node#25
Open
[Snyk] Security upgrade gatsby from 2.18.18 to 2.31.0 137717unity/node#16
Open
[Snyk] Security upgrade gatsby from 2.23.20 to 2.31.0 turkdevops/node#89
Open
[Snyk] Security upgrade gatsby from 2.18.18 to 2.31.0 Rednickle/node-1#12
Open
This was referenced Sep 5, 2021
[Snyk] Security upgrade gatsby from 2.18.18 to 2.31.0 Rednickle/node-1#28
Open
[Snyk] Security upgrade gatsby from 2.24.27 to 2.31.0 Dmarch28/node#33
Open
[Snyk] Security upgrade gatsby from 2.18.18 to 2.31.0 137717unity/node#33
Open
[Snyk] Security upgrade gatsby from 2.13.73 to 2.31.0 meghasfdc/node#42
Open
[Snyk] Security upgrade gatsby from 2.18.18 to 2.31.0 wedataintelligence/node#9
Open
[Snyk] Security upgrade gatsby from 2.18.18 to 2.31.0 aliceUnhinged613/node#32
Open
[Snyk] Security upgrade gatsby from 2.24.27 to 2.31.0 baby636/node#18
Open
[Snyk] Security upgrade gatsby from 2.18.18 to 2.31.0 ron-a20/node#41
Open
[Snyk] Security upgrade gatsby from 2.13.73 to 2.31.0 kissofire/node#42
Open
[Snyk] Security upgrade gatsby from 2.18.18 to 2.31.0 sitedata/node#32
Open
[Snyk] Security upgrade gatsby from 2.18.18 to 2.31.0 molllyn1/node#21
Open
[Snyk] Security upgrade gatsby from 2.23.20 to 2.31.0 turkdevops/node#318
Closed
@kissofire kissofire mentioned this pull request Jan 1, 2024
[Snyk] Security upgrade gatsby from 2.13.73 to 2.31.0 kissofire/node#116
Open
@aliceUnhinged613 aliceUnhinged613 mentioned this pull request Jan 1, 2024
[Snyk] Security upgrade gatsby from 2.18.18 to 2.31.0 aliceUnhinged613/node#115
Open
@137717unity 137717unity mentioned this pull request Jan 2, 2024
[Snyk] Security upgrade gatsby from 2.18.18 to 2.31.0 137717unity/node#115
Open
@molllyn1 molllyn1 mentioned this pull request Jan 2, 2024
[Snyk] Security upgrade gatsby from 2.18.18 to 2.31.0 molllyn1/node#99
Open
@wedataintelligence wedataintelligence mentioned this pull request Jan 2, 2024
[Snyk] Security upgrade gatsby from 2.18.18 to 2.31.0 wedataintelligence/node#98
Open
@ron-a20 ron-a20 mentioned this pull request Jan 2, 2024
[Snyk] Security upgrade gatsby from 2.18.18 to 2.31.0 ron-a20/node#113
Open
@molllyn1 molllyn1 mentioned this pull request Mar 15, 2024
[Snyk] Security upgrade gatsby from 2.18.18 to 2.31.0 molllyn1/node#105
Open
@ron-a20 ron-a20 mentioned this pull request Mar 15, 2024
[Snyk] Security upgrade gatsby from 2.18.18 to 2.31.0 ron-a20/node#119
Open
@kissofire kissofire mentioned this pull request Mar 15, 2024
[Snyk] Security upgrade gatsby from 2.13.73 to 2.31.0 kissofire/node#122
Open
@aliceUnhinged613 aliceUnhinged613 mentioned this pull request Mar 15, 2024
[Snyk] Security upgrade gatsby from 2.18.18 to 2.31.0 aliceUnhinged613/node#121
Open
@137717unity 137717unity mentioned this pull request Mar 16, 2024
[Snyk] Security upgrade gatsby from 2.18.18 to 2.31.0 137717unity/node#121
Open
@wedataintelligence wedataintelligence mentioned this pull request Mar 18, 2024
[Snyk] Security upgrade gatsby from 2.18.18 to 2.31.0 wedataintelligence/node#104
Open
This was referenced Sep 16, 2026
fix(daemon): pause producers when stream backlogs grow stablyai/orca#20947
Merged
node:net: emit 'error' once when a buffered write on an adopted fd fails oven-sh/bun#43038
Open
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@Trott Trott Trott left review comments
@mcollina mcollina mcollina approved these changes
@jasnell jasnell jasnell approved these changes
@addaleax addaleax addaleax approved these changes
@mafintosh mafintosh Awaiting requested review from mafintosh
+1 more reviewer
@mscdex mscdex mscdex left review comments
Reviewers whose approvals may not affect merge requirements

Uh oh!

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

Assignees

No one assigned

Labels

semver-major PRs that contain breaking changes and should be released in the next major version. stream Issues and PRs related to Node.js streams.

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Uh oh!

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

8 participants

@ronag @jasnell @mafintosh @nodejs-github-bot @mcollina @Trott @mscdex @addaleax
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 01402520 — 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.