Skip to content

stream: fix legacy pipe error handling #35257

Closed
ronag wants to merge 2 commits into
nodejs:masterfrom
nxtedition:legacy-error-handling
Closed

stream: fix legacy pipe error handling #35257
ronag wants to merge 2 commits into
nodejs:masterfrom
nxtedition:legacy-error-handling

Conversation

@ronag
Copy link
Copy Markdown
Member

@ronag ronag commented Sep 18, 2020 •
edited
Loading

Fixes: #35237

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
@ronag ronag added the request-ci Add this label to start a Jenkins CI on a PR. label Sep 18, 2020
@ronag ronag requested a review from mcollina September 18, 2020 13:39
@nodejs-github-bot nodejs-github-bot added the stream Issues and PRs related to the stream subsystem. label Sep 18, 2020
@ronag ronag force-pushed the legacy-error-handling branch 2 times, most recently from 0aace1a to c07e7a5 Compare September 18, 2020 13:42
@ronag ronag changed the title Legacy error handling stream: fix legacy pipe error handling Sep 18, 2020
@ronag ronag force-pushed the legacy-error-handling branch from c07e7a5 to 85a930c Compare September 18, 2020 13:52
@github-actions github-actions Bot removed the request-ci Add this label to start a Jenkins CI on a PR. label Sep 18, 2020
@nodejs-github-bot
Copy link
Copy Markdown
Collaborator

nodejs-github-bot commented Sep 18, 2020

CI: https://ci.nodejs.org/job/node-test-pull-request/33162/
mcollina
mcollina requested changes Sep 18, 2020
View reviewed changes
Comment thread lib/internal/streams/util.js Outdated
@ronag ronag force-pushed the legacy-error-handling branch from edf0ed1 to 56e479b Compare September 20, 2020 20:32
@ronag ronag requested a review from mcollina September 20, 2020 20:33
@ronag
Copy link
Copy Markdown
Member Author

ronag commented Sep 20, 2020

@nodejs/streams
@Trott
Copy link
Copy Markdown
Member

Trott commented Sep 22, 2020

Needs a rebase. Looks good to me, but I'm usually hesitant to 👍 a streams change without someone from @nodejs/streams 👍 it first.
mcollina
mcollina approved these changes Sep 22, 2020
View reviewed changes
Copy link
Copy Markdown
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
@ronag ronag force-pushed the legacy-error-handling branch from 56e479b to f750a74 Compare September 22, 2020 14:57
@ronag ronag added the request-ci Add this label to start a Jenkins CI on a PR. label Sep 22, 2020
@github-actions github-actions Bot removed the request-ci Add this label to start a Jenkins CI on a PR. label Sep 22, 2020
@nodejs-github-bot
Copy link
Copy Markdown
Collaborator

nodejs-github-bot commented Sep 22, 2020

CI: https://ci.nodejs.org/job/node-test-pull-request/33193/
lpinca
lpinca approved these changes Sep 22, 2020
View reviewed changes
Comment thread test/parallel/test-stream-pipe-error-handling.js Outdated
@ronag ronag added the request-ci Add this label to start a Jenkins CI on a PR. label Sep 22, 2020
@ronag ronag force-pushed the legacy-error-handling branch from 8f4f1a6 to c215a0e Compare September 22, 2020 20:55
@github-actions github-actions Bot removed the request-ci Add this label to start a Jenkins CI on a PR. label Sep 22, 2020
@nodejs-github-bot
Copy link
Copy Markdown
Collaborator

nodejs-github-bot commented Sep 22, 2020

CI: https://ci.nodejs.org/job/node-test-pull-request/33202/
@ronag ronag added the commit-queue Add this label to land a pull request using GitHub Actions. label Sep 23, 2020
@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 Sep 23, 2020
@github-actions
Copy link
Copy Markdown
Contributor

github-actions Bot commented Sep 23, 2020

Commit Queue failed
- Loading data for nodejs/node/pull/35257
✔  Done loading data for nodejs/node/pull/35257
----------------------------------- PR info ------------------------------------
Title      stream: fix legacy pipe error handling  (#35257)
Author     Robert Nagy  (@ronag)
Branch     ronag:legacy-error-handling -> nodejs:master
Labels     lts-watch-v12.x, stream
Commits    2
 - stream: fix legacy pipe error handling
 - fixup
Committers 1
 - Robert Nagy 
PR-URL: https://github.com/nodejs/node/pull/35257
Fixes: https://github.com/nodejs/node/issues/35237
Reviewed-By: Matteo Collina 
Reviewed-By: Luigi Pinca 
------------------------------ Generated metadata ------------------------------
PR-URL: https://github.com/nodejs/node/pull/35257
Fixes: https://github.com/nodejs/node/issues/35237
Reviewed-By: Matteo Collina 
Reviewed-By: Luigi Pinca 
--------------------------------------------------------------------------------
   ⚠  Commits were pushed since the last review:
   ⚠  - fixup
   ✖  GitHub CI is still running
   ℹ  Last Full PR CI on 2020-09-22T20:58:51Z: https://ci.nodejs.org/job/node-test-pull-request/33202/
- Querying data for job/node-test-pull-request/33202/
✔  Build data downloaded
   ✔  Last Jenkins CI successful
   ℹ  This PR was created on Fri, 18 Sep 2020 13:39:55 GMT
   ✔  Approvals: 2
   ✔  - Matteo Collina (@mcollina) (TSC): https://github.com/nodejs/node/pull/35257#pullrequestreview-493519435
   ✔  - Luigi Pinca (@lpinca): https://github.com/nodejs/node/pull/35257#pullrequestreview-493754259
--------------------------------------------------------------------------------
   ✔  Aborted `git node land` session in /home/runner/work/node/node/.ncu
@ronag
Copy link
Copy Markdown
Member Author

ronag commented Sep 23, 2020

@mcollina do you know if the commit queue auto squashes?
Trott pushed a commit to Trott/io.js that referenced this pull request Sep 23, 2020
@ronag @Trott
stream: fix legacy pipe error handling
6be80e1 
Fixes: nodejs#35237

PR-URL: nodejs#35257
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: Rich Trott <rtrott@gmail.com>
@Trott
Copy link
Copy Markdown
Member

Trott commented Sep 23, 2020

Landed in 6be80e1
@Trott Trott closed this Sep 23, 2020
@lundibundi
Copy link
Copy Markdown
Member

lundibundi commented Sep 23, 2020

@ronag no it doesn't, it will only autosquash if used with --fixup git command
see #34969 (comment) for more information.

I've implemented nodejs/node-core-utils#490 but it is not yet integrated into this repo.
(also - #34770)
@MylesBorins
Copy link
Copy Markdown
Contributor

MylesBorins commented Sep 24, 2020

This doesn't land cleanly on v14.x

@ronag would you be able to backport?
joesepi pushed a commit to joesepi/node that referenced this pull request Jan 8, 2021
@ronag @joesepi
stream: fix legacy pipe error handling
4c279b4 
Fixes: nodejs#35237

PR-URL: nodejs#35257
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: Rich Trott <rtrott@gmail.com>
targos pushed a commit that referenced this pull request May 16, 2021
@ronag @targos
stream: fix legacy pipe error handling
22bf488 
Fixes: #35237

PR-URL: #35257
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: Rich Trott <rtrott@gmail.com>
@targos targos mentioned this pull request Jun 6, 2021
Merged
targos pushed a commit that referenced this pull request Jun 11, 2021
@ronag @targos
stream: fix legacy pipe error handling
c31e2f6 
Fixes: #35237

PR-URL: #35257
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: Rich Trott <rtrott@gmail.com>
@targos targos removed the commit-queue-failed An error occurred while landing this pull request using GitHub Actions. label Sep 5, 2021
@richardlau
Copy link
Copy Markdown
Member

richardlau commented Dec 14, 2021

This has a lts-watch-v12.x label on but doesn't cherry-pick cleanly onto v12.x-staging so will require a manual backport. FWIW the test currently fails on v12.x-staging:

$ ./out/Release/node test/parallel/test-stream-pipe-error-handling.js
internal/streams/legacy.js:61
      throw er; // Unhandled stream error in pipe.
      ^

Error: this should be handled
    at Object.<anonymous> (/home/rlau/sandbox/github/trees/v12.x-staging/test/parallel/test-stream-pipe-error-handling.js:113:16)
    at Module._compile (internal/modules/cjs/loader.js:999:30)
    at Object.Module._extensions..js (internal/modules/cjs/loader.js:1027:10)
    at Module.load (internal/modules/cjs/loader.js:863:32)
    at Function.Module._load (internal/modules/cjs/loader.js:708:14)
    at Function.executeUserEntryPoint [as runMain] (internal/modules/run_main.js:60:12)
    at internal/main/run_main_module.js:17:47
$
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

stream Issues and PRs related to the stream subsystem.

9 participants

@ronag @nodejs-github-bot @Trott @lundibundi @MylesBorins @richardlau @mcollina @lpinca @targos