Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

doc: clarify stream errors while reading and writing #29653

Closed
wants to merge 11 commits into from
Closed

doc: clarify stream errors while reading and writing #29653

wants to merge 11 commits into from

Conversation

ronag
Copy link
Member

@ronag ronag commented Sep 22, 2019 •
edited
Loading

Errors should be propagated through destroy(err) for readable and callback for writable, anything else is undefined behaviour.

Refs: #29584

Checklist
  • make -j4 test (UNIX), or vcbuild test (Windows) passes
  • documentation is changed or added
  • commit message follows commit guidelines

@nodejs-github-bot nodejs-github-bot added doc Issues and PRs related to the documentations. stream Issues and PRs related to the stream subsystem. labels Sep 22, 2019
@ronag ronag force-pushed the fix-doc-readable-error branch from ce06247 to 9c91dd2 Compare September 22, 2019 14:13
@ronag
Copy link
Member Author

ronag commented Sep 22, 2019

@jasnell @mcollina Hope this makes more sense.

@ronag ronag changed the title doc: clarify stream errors while reading doc: clarify stream errors while reading and writing Sep 22, 2019
@ronag
doc: clarify stream errors while readingi and writing
82f209e 
Errors should be propagated through destroy(err), anything else
is basically undefined behaviour.
@ronag ronag force-pushed the fix-doc-readable-error branch 3 times, most recently from df3867e to ee486d4 Compare September 22, 2019 14:23
@ronag ronag force-pushed the fix-doc-readable-error branch from ee486d4 to 25c4396 Compare September 22, 2019 14:47
@trivikr
Copy link
Member

trivikr commented Sep 22, 2019

CI-lite: https://ci.nodejs.org/job/node-test-pull-request-lite-pipeline/3921/

@ronag ronag mentioned this pull request Sep 22, 2019
Closed
Fishrock123 added a commit to Fishrock123/bob that referenced this pull request Sep 23, 2019
@Fishrock123
fixup: always use destroy(err) instead of emit
bd6a3d1 
See nodejs/node#29653
Fishrock123
Fishrock123 approved these changes Sep 23, 2019
View reviewed changes
Copy link
Contributor

@Fishrock123 Fishrock123 left a comment

Choose a reason for hiding this comment

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

This seems like the correct, better approach to me. I'd really like other streams folks confirmation though.

billouboq
billouboq reviewed Sep 24, 2019
View reviewed changes
doc/api/stream.md Outdated Show resolved Hide resolved
ronag
ronag commented Sep 24, 2019
View reviewed changes
doc/api/stream.md Outdated Show resolved Hide resolved
jasnell
jasnell reviewed Sep 25, 2019
View reviewed changes
doc/api/stream.md Outdated Show resolved Hide resolved
jasnell
jasnell reviewed Sep 25, 2019
View reviewed changes
doc/api/stream.md Outdated Show resolved Hide resolved
ronag and others added 2 commits September 25, 2019 18:11
@ronag @jasnell
Update doc/api/stream.md
09b2f98 
Co-Authored-By: James M Snell <jasnell@gmail.com>
@ronag @jasnell
Update doc/api/stream.md
c02435b 
Co-Authored-By: James M Snell <jasnell@gmail.com>
vsemozhetbyt
vsemozhetbyt reviewed Sep 25, 2019
View reviewed changes
doc/api/stream.md Outdated Show resolved Hide resolved
vsemozhetbyt
vsemozhetbyt reviewed Sep 25, 2019
View reviewed changes
doc/api/stream.md Outdated Show resolved Hide resolved
vsemozhetbyt
vsemozhetbyt reviewed Sep 25, 2019
View reviewed changes
doc/api/stream.md Outdated Show resolved Hide resolved
vsemozhetbyt
vsemozhetbyt reviewed Sep 25, 2019
View reviewed changes
doc/api/stream.md Outdated Show resolved Hide resolved
ronag and others added 2 commits September 25, 2019 20:08
@ronag @vsemozhetbyt
Update doc/api/stream.md
63ec522 
Co-Authored-By: Vse Mozhet Byt <vsemozhetbyt@gmail.com>
@ronag @vsemozhetbyt
Update doc/api/stream.md
de8dc2d 
Co-Authored-By: Vse Mozhet Byt <vsemozhetbyt@gmail.com>
ronag and others added 3 commits September 25, 2019 20:09
@ronag @vsemozhetbyt
Update doc/api/stream.md
25cf976 
Co-Authored-By: Vse Mozhet Byt <vsemozhetbyt@gmail.com>
@ronag @vsemozhetbyt
Update doc/api/stream.md
e1d6651 
Co-Authored-By: Vse Mozhet Byt <vsemozhetbyt@gmail.com>
@ronag
fixup: ascii order
e7709c1
vsemozhetbyt
vsemozhetbyt reviewed Sep 25, 2019
View reviewed changes
test.js Outdated Show resolved Hide resolved
mcollina
mcollina approved these changes Sep 27, 2019
View reviewed changes
Copy link
Member

@mcollina mcollina left a comment

Choose a reason for hiding this comment

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

LGTM

@Trott
Copy link
Member

Trott commented Sep 27, 2019

Lite CI: https://ci.nodejs.org/job/node-test-pull-request-lite-pipeline/3939/

Trott pushed a commit that referenced this pull request Sep 27, 2019
@ronag @Trott
doc: clarify stream errors while reading and writing
7223ce2 
Errors should be propagated through destroy(err). Anything else
is basically undefined behaviour.

PR-URL: #29653
Refs: #29584
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
Reviewed-By: Jeremiah Senkpiel <fishrock123@rocketmail.com>
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
@Trott
Copy link
Member

Trott commented Sep 27, 2019

Landed in 7223ce2

@Trott Trott closed this Sep 27, 2019
targos pushed a commit that referenced this pull request Oct 1, 2019
@ronag @targos
doc: clarify stream errors while reading and writing
d258e02 
Errors should be propagated through destroy(err). Anything else
is basically undefined behaviour.

PR-URL: #29653
Refs: #29584
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
Reviewed-By: Jeremiah Senkpiel <fishrock123@rocketmail.com>
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
@targos targos mentioned this pull request Oct 1, 2019
Merged
@beevelop beevelop mentioned this pull request Oct 1, 2019
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@vsemozhetbyt vsemozhetbyt vsemozhetbyt left review comments

@billouboq billouboq billouboq left review comments

@jasnell jasnell jasnell left review comments

@BridgeAR BridgeAR BridgeAR approved these changes

@lpinca lpinca lpinca approved these changes

@Fishrock123 Fishrock123 Fishrock123 approved these changes

@mcollina mcollina mcollina approved these changes

Assignees
No one assigned
Labels
doc Issues and PRs related to the documentations. stream Issues and PRs related to the stream subsystem.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.