Skip to content

Conversation

@simoneb
Copy link
Contributor

@simonebsimoneb commented Jun 7, 2021

  • replace may or may not be called with will be called because the callback is always called
  • remove To reliably detect write errors, add a listener for the 'error' event because the error event will NOT be emitted if a write occurs after the stream has been closed

Fixes: #38704

- replace _*may or may not* be called_ with _will be called_ because the callback is always called - remove _To reliably detect write errors, add a listener for the `'error'` event_ because the `error` event will NOT be emitted if a write occurs after the stream has been closed
@github-actionsgithub-actionsbot added doc Issues and PRs related to the documentations. stream Issues and PRs related to the stream subsystem. labels Jun 7, 2021
@aduh95aduh95 added dont-land-on-v12.x author ready PRs that have at least one approval, no pending requests for changes, and a CI started. labels Jun 7, 2021
@aduh95
Copy link
Contributor

If I understand the linked issue correctly, this is true only for Node.js v15+. Adding the labels so it doesn't get backported, feel free to remove them if I misunderstood the situation.

@benjamingr
Copy link
Member

@nodejs/streams

Copy link
Member

@mcollinamcollina left a comment

Choose a reason for hiding this comment

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

lgtm

@mmarchinimmarchini added the commit-queue Add this label to land a pull request using GitHub Actions. label Jun 10, 2021
@github-actionsgithub-actionsbot removed the commit-queue Add this label to land a pull request using GitHub Actions. label Jun 10, 2021
@github-actions
Copy link
Contributor

Commit Queue failed
- Loading data for nodejs/node/pull/38959 ✔ Done loading data for nodejs/node/pull/38959 ----------------------------------- PR info ------------------------------------ Title doc: update write callback documentation (#38959) ⚠ Could not retrieve the email or name of the PR author's from user's GitHub profile! Branch simoneb:patch-1 -> nodejs:master Labels author ready, doc, dont-land-on-v12.x, dont-land-on-v14.x, stream Commits 1 - doc: update write callback documentation Committers 1 - GitHub PR-URL: https://github.com/nodejs/node/pull/38959 Fixes: https://github.com/nodejs/node/issues/38704 Reviewed-By: James M Snell Reviewed-By: Antoine du Hamel Reviewed-By: Benjamin Gruenbaum Reviewed-By: Robert Nagy Reviewed-By: Matteo Collina ------------------------------ Generated metadata ------------------------------ PR-URL: https://github.com/nodejs/node/pull/38959 Fixes: https://github.com/nodejs/node/issues/38704 Reviewed-By: James M Snell Reviewed-By: Antoine du Hamel Reviewed-By: Benjamin Gruenbaum Reviewed-By: Robert Nagy Reviewed-By: Matteo Collina -------------------------------------------------------------------------------- ℹ This PR was created on Mon, 07 Jun 2021 14:02:22 GMT ✔ Approvals: 5 ✔ - James M Snell (@jasnell) (TSC): https://github.com/nodejs/node/pull/38959#pullrequestreview-677503321 ✔ - Antoine du Hamel (@aduh95) (TSC): https://github.com/nodejs/node/pull/38959#pullrequestreview-677663819 ✔ - Benjamin Gruenbaum (@benjamingr): https://github.com/nodejs/node/pull/38959#pullrequestreview-677841411 ✔ - Robert Nagy (@ronag) (TSC): https://github.com/nodejs/node/pull/38959#pullrequestreview-677850823 ✔ - Matteo Collina (@mcollina) (TSC): https://github.com/nodejs/node/pull/38959#pullrequestreview-678297907 ✖ Last GitHub CI failed ℹ Green GitHub Actions CI is sufficient -------------------------------------------------------------------------------- ✔ Aborted `git node land` session in /home/runner/work/node/node/.ncu
https://github.com/nodejs/node/actions/runs/926267471

@github-actionsgithub-actionsbot added the commit-queue-failed An error occurred while landing this pull request using GitHub Actions. label Jun 10, 2021
@mmarchinimmarchini added commit-queue Add this label to land a pull request using GitHub Actions. and removed commit-queue-failed An error occurred while landing this pull request using GitHub Actions. labels Jun 10, 2021
@github-actionsgithub-actionsbot added commit-queue-failed An error occurred while landing this pull request using GitHub Actions. and removed commit-queue Add this label to land a pull request using GitHub Actions. labels Jun 10, 2021
@github-actions
Copy link
Contributor

Commit Queue failed
- Loading data for nodejs/node/pull/38959 ✔ Done loading data for nodejs/node/pull/38959 ----------------------------------- PR info ------------------------------------ Title doc: update write callback documentation (#38959) ⚠ Could not retrieve the email or name of the PR author's from user's GitHub profile! Branch simoneb:patch-1 -> nodejs:master Labels author ready, doc, dont-land-on-v12.x, dont-land-on-v14.x, stream Commits 1 - doc: update write callback documentation Committers 1 - GitHub PR-URL: https://github.com/nodejs/node/pull/38959 Fixes: https://github.com/nodejs/node/issues/38704 Reviewed-By: James M Snell Reviewed-By: Antoine du Hamel Reviewed-By: Benjamin Gruenbaum Reviewed-By: Robert Nagy Reviewed-By: Matteo Collina ------------------------------ Generated metadata ------------------------------ PR-URL: https://github.com/nodejs/node/pull/38959 Fixes: https://github.com/nodejs/node/issues/38704 Reviewed-By: James M Snell Reviewed-By: Antoine du Hamel Reviewed-By: Benjamin Gruenbaum Reviewed-By: Robert Nagy Reviewed-By: Matteo Collina -------------------------------------------------------------------------------- ℹ This PR was created on Mon, 07 Jun 2021 14:02:22 GMT ✔ Approvals: 5 ✔ - James M Snell (@jasnell) (TSC): https://github.com/nodejs/node/pull/38959#pullrequestreview-677503321 ✔ - Antoine du Hamel (@aduh95) (TSC): https://github.com/nodejs/node/pull/38959#pullrequestreview-677663819 ✔ - Benjamin Gruenbaum (@benjamingr): https://github.com/nodejs/node/pull/38959#pullrequestreview-677841411 ✔ - Robert Nagy (@ronag) (TSC): https://github.com/nodejs/node/pull/38959#pullrequestreview-677850823 ✔ - Matteo Collina (@mcollina) (TSC): https://github.com/nodejs/node/pull/38959#pullrequestreview-678297907 ✔ Last GitHub Actions successful ℹ Green GitHub Actions CI is sufficient -------------------------------------------------------------------------------- ✔ No git cherry-pick in progress ✔ No git am in progress ✔ No git rebase in progress -------------------------------------------------------------------------------- - Bringing origin/master up to date... From https://github.com/nodejs/node * branch master -> FETCH_HEAD ✔ origin/master is now up-to-date - Downloading patch for 38959 From https://github.com/nodejs/node * branch refs/pull/38959/merge -> FETCH_HEAD ✔ Fetched commits as 52f8471b5281..453d7dbcbd99 -------------------------------------------------------------------------------- [master d77e235f62] doc: update write callback documentation Author: Simone Busoli Date: Mon Jun 7 16:01:49 2021 +0200 1 file changed, 2 insertions(+), 3 deletions(-) ✔ Patches applied -------------------------------------------------------------------------------- --------------------------------- New Message ---------------------------------- doc: update write callback documentation 
  • replace may or may not be called with will be called because the callback is always called
  • remove To reliably detect write errors, add a listener for the 'error' event because the error event will NOT be emitted if a write occurs after the stream has been closed

PR-URL: #38959
Fixes: #38704
Reviewed-By: James M Snell [email protected]
Reviewed-By: Antoine du Hamel [email protected]
Reviewed-By: Benjamin Gruenbaum [email protected]
Reviewed-By: Robert Nagy [email protected]
Reviewed-By: Matteo Collina [email protected]

[master 8806c955a2] doc: update write callback documentation
Author: Simone Busoli [email protected]
Date: Mon Jun 7 16:01:49 2021 +0200
1 file changed, 2 insertions(+), 3 deletions(-)
✖ 8806c955a270b602ab78330502cd8694e756ec8b
✔ 5:7 Valid fixes URL. fixes-url
✔ 0:0 blank line after title line-after-title
✖ 1:72 Line should be <= 72 columns. line-length
✖ 2:72 Line should be <= 72 columns. line-length
✔ 0:0 metadata is at end of message metadata-end
✔ 4:8 PR-URL is valid. pr-url
✔ 0:0 reviewers are valid reviewers
✔ 0:0 valid subsystems subsystem
✔ 0:0 Title is formatted correctly. title-format
✔ 0:0 Title is <= 50 columns. title-length
ℹ Please fix the commit message and try again.

https://github.com/nodejs/node/actions/runs/926415209

mmarchini pushed a commit that referenced this pull request Jun 10, 2021
- replace _*may or may not* be called_ with _will be called_ because the callback is always called - remove _To reliably detect write errors, add a listener for the `'error'` event_ because the `error` event will NOT be emitted if a write occurs after the stream has been closed PR-URL: #38959Fixes: #38704 Reviewed-By: James M Snell <[email protected]> Reviewed-By: Antoine du Hamel <[email protected]> Reviewed-By: Benjamin Gruenbaum <[email protected]> Reviewed-By: Robert Nagy <[email protected]> Reviewed-By: Matteo Collina <[email protected]>
@mmarchini
Copy link
Contributor

Landed in cf609cc

targos pushed a commit that referenced this pull request Jun 11, 2021
- replace _*may or may not* be called_ with _will be called_ because the callback is always called - remove _To reliably detect write errors, add a listener for the `'error'` event_ because the `error` event will NOT be emitted if a write occurs after the stream has been closed PR-URL: #38959Fixes: #38704 Reviewed-By: James M Snell <[email protected]> Reviewed-By: Antoine du Hamel <[email protected]> Reviewed-By: Benjamin Gruenbaum <[email protected]> Reviewed-By: Robert Nagy <[email protected]> Reviewed-By: Matteo Collina <[email protected]>
@danielleadamsdanielleadams mentioned this pull request Jun 14, 2021
danielleadams pushed a commit that referenced this pull request Jun 17, 2021
- replace _*may or may not* be called_ with _will be called_ because the callback is always called - remove _To reliably detect write errors, add a listener for the `'error'` event_ because the `error` event will NOT be emitted if a write occurs after the stream has been closed PR-URL: #38959Fixes: #38704 Reviewed-By: James M Snell <[email protected]> Reviewed-By: Antoine du Hamel <[email protected]> Reviewed-By: Benjamin Gruenbaum <[email protected]> Reviewed-By: Robert Nagy <[email protected]> Reviewed-By: Matteo Collina <[email protected]>
@targostargos removed the commit-queue-failed An error occurred while landing this pull request using GitHub Actions. label Sep 5, 2021
Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Labels

author readyPRs that have at least one approval, no pending requests for changes, and a CI started.docIssues and PRs related to the documentations.streamIssues and PRs related to the stream subsystem.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

node 16.x does not raise error on write when server closes connection

8 participants

@simoneb@aduh95@benjamingr@mmarchini@mcollina@jasnell@ronag@targos