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.

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

doc: update write callback documentation #38959

Closed
wants to merge 1 commit into from
Closed

doc: update write callback documentation #38959

wants to merge 1 commit into from

Conversation

simoneb
Copy link
Contributor

@simoneb simoneb 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-actions github-actions bot added doc Issues and PRs related to the documentations. stream Issues and PRs related to the stream subsystem. labels Jun 7, 2021
@aduh95 aduh95 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

aduh95 commented Jun 7, 2021

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

@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

@mmarchini mmarchini added the commit-queue Add this label to land a pull request using GitHub Actions. label Jun 10, 2021
@github-actions github-actions bot 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-actions github-actions bot added the commit-queue-failed An error occurred while landing this pull request using GitHub Actions. label Jun 10, 2021
@mmarchini mmarchini 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-actions github-actions bot 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: #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]>
@mmarchini
Copy link
Contributor

Landed in cf609cc

@mmarchini mmarchini closed this Jun 10, 2021
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: #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]>
@danielleadams danielleadams 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: #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]>
@targos targos removed the commit-queue-failed An error occurred while landing this pull request using GitHub Actions. label Sep 5, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
author ready PRs that have at least one approval, no pending requests for changes, and a CI started. doc Issues and PRs related to the documentations. stream Issues 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