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

stream: fix legacy pipe error handling #35257

Closed
wants to merge 2 commits into from

Conversation

ronag
Copy link
Member

@ronag ronag commented Sep 18, 2020

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
@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
Collaborator

lib/internal/streams/util.js Outdated Show resolved Hide resolved
@ronag
Copy link
Member Author

ronag commented Sep 20, 2020

@nodejs/streams

@Trott
Copy link
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.

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

@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
Collaborator

test/parallel/test-stream-pipe-error-handling.js Outdated Show resolved Hide resolved
@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
Collaborator

@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
Contributor

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
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
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
Member

Trott commented Sep 23, 2020

Landed in 6be80e1

@Trott Trott closed this Sep 23, 2020
@lundibundi
Copy link
Member

@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
Contributor

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
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
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 pushed a commit that referenced this pull request Jun 11, 2021
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
Member

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.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Legacy stream throws unhandled stream error when it is handled
9 participants