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 finished regression when working with legacy Stream #40858

Merged
merged 2 commits into from
Nov 22, 2021

Conversation

mcollina
Copy link
Member

@mcollina mcollina commented Nov 18, 2021

Fixes a regression introduced in Node v17 that made Fastify test fails.
The reason is that using finished() and pipeline() with the send module started to error with "premature close" even if the stream did not close prematurely.

@nodejs-github-bot nodejs-github-bot added the needs-ci PRs that need a full CI run. label Nov 18, 2021
@mcollina mcollina requested a review from lpinca November 18, 2021 15:45
@mcollina
Copy link
Member Author

@ronag do you know which of the PRs introduced this?

@ronag
Copy link
Member

ronag commented Nov 18, 2021

Something is weird here. I want to understand why this happens. I’ll investigate today.

@nodejs-github-bot
Copy link
Collaborator

@mcollina
Copy link
Member Author

Something is weird here. I want to understand why this happens. I’ll investigate today.

The test exposes what would be the correct behavior when used on a legacy stream. As long as it pass that test we should be good.

Note that this fix does not break any other test.

@@ -630,3 +631,9 @@ testClosed((opts) => new Writable({ write() {}, ...opts }));
}));
}));
}

{
// This case is needed to support the send module
Copy link
Contributor

@mscdex mscdex Nov 18, 2021

Choose a reason for hiding this comment

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

This comment is a bit confusing because it sounds like send is an internal module. Explaining what the test is checking for would be more useful than referencing a 3rd party module on npm, especially since as time goes on send may change its implementation and this comment will be outdated.

Copy link
Member Author

@mcollina mcollina Nov 18, 2021

Choose a reason for hiding this comment

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

updated, good idea.

@ronag
Copy link
Member

ronag commented Nov 18, 2021

diff --git a/lib/internal/streams/end-of-stream.js b/lib/internal/streams/end-of-stream.js
index 91316f495c..ab60f5a7ea 100644
--- a/lib/internal/streams/end-of-stream.js
+++ b/lib/internal/streams/end-of-stream.js
@@ -165,13 +165,13 @@ function eos(stream, options, callback) {
   } else if (
     !readable &&
     (!willEmitClose || isReadable(stream)) &&
-    (writableFinished || !isWritable(stream))
+    (writableFinished || isWritable(stream) === false)
   ) {
     process.nextTick(onclose);
   } else if (
     !writable &&
     (!willEmitClose || isWritable(stream)) &&
-    (readableFinished || !isReadableNodeStream(stream))
+    (readableFinished || isReadable(stream) === false)
   ) {
     process.nextTick(onclose);
   } else if ((rState && stream.req && stream.aborted)) {
diff --git a/test/parallel/test-stream-finished.js b/test/parallel/test-stream-finished.js

This is a better fix. Basically what it says is that if we don't know (i.e. === null) then we don't call the callback (at the risk of deadlock).

Copy link
Member

@ronag ronag left a comment

Choose a reason for hiding this comment

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

See my proposal above.

Signed-off-by: Matteo Collina <hello@matteocollina.com>
@ronag ronag added author ready PRs that have at least one approval, no pending requests for changes, and a CI started. request-ci Add this label to start a Jenkins CI on a PR. stream Issues and PRs related to the stream subsystem. and removed needs-ci PRs that need a full CI run. labels Nov 18, 2021
@ronag
Copy link
Member

ronag commented Nov 18, 2021

@nodejs/streams

@nodejs-github-bot
Copy link
Collaborator

@github-actions github-actions bot removed the request-ci Add this label to start a Jenkins CI on a PR. label Nov 18, 2021
@nodejs-github-bot
Copy link
Collaborator

test/parallel/test-stream-finished.js Outdated Show resolved Hide resolved
@nodejs-github-bot
Copy link
Collaborator

@nodejs-github-bot
Copy link
Collaborator

@aduh95 aduh95 added commit-queue Add this label to land a pull request using GitHub Actions. commit-queue-squash Add this label to instruct the Commit Queue to squash all the PR commits into the first one. labels Nov 22, 2021
@nodejs-github-bot nodejs-github-bot removed the commit-queue Add this label to land a pull request using GitHub Actions. label Nov 22, 2021
@nodejs-github-bot nodejs-github-bot merged commit cb75dec into nodejs:master Nov 22, 2021
@nodejs-github-bot
Copy link
Collaborator

Landed in cb75dec

@mcollina mcollina deleted the fix-fastify-regression branch November 22, 2021 12:21
@mcollina mcollina added the notable-change PRs with changes that should be highlighted in changelogs. label Nov 22, 2021
targos pushed a commit that referenced this pull request Nov 26, 2021
Signed-off-by: Matteo Collina <hello@matteocollina.com>

PR-URL: #40858
Reviewed-By: Robert Nagy <ronagy@icloud.com>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: Antoine du Hamel <duhamelantoine1995@gmail.com>
Reviewed-By: Tobias Nießen <tniessen@tnie.de>
Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com>
targos added a commit that referenced this pull request Nov 26, 2021
Notable changes:

build:
  * (SEMVER-MINOR) reset embedder string to "-node.0" (Michaël Zasso) #40488
deps:
  * (SEMVER-MINOR) disable trap handler for Windows cross-compiler (Michaël Zasso) #40488
  * (SEMVER-MINOR) update V8 to 9.6.180.14 (Michaël Zasso) #40488
lib:
  * (SEMVER-MINOR) add reason to AbortSignal (James M Snell) #40807
src:
  * (SEMVER-MINOR) add x509.fingerprint512 to crypto module (3nprob) #39809
stream:
  * deprecate thenable support (Antoine du Hamel) #40860
  * fix finished regression when working with legacy Stream (Matteo Collina) #40858
tools:
  * (SEMVER-MINOR) disable trap handler for Windows cross-compiler (Michaël Zasso) #40488
  * (SEMVER-MINOR) update V8 gypfiles for 9.6 (Michaël Zasso) #40488

PR-URL: TODO
targos added a commit that referenced this pull request Nov 26, 2021
Notable changes:

async_hooks:
  * (SEMVER-MINOR) expose async_wrap providers (Rafael Gonzaga) #40760
deps:
  * (SEMVER-MINOR) update V8 to 9.6.180.14 (Michaël Zasso) #40488
lib:
  * (SEMVER-MINOR) add reason to AbortSignal (James M Snell) #40807
src:
  * (SEMVER-MINOR) add x509.fingerprint512 to crypto module (3nprob) #39809
stream:
  * deprecate thenable support (Antoine du Hamel) #40860
  * fix finished regression when working with legacy Stream (Matteo Collina) #40858

PR-URL: #40983
targos added a commit that referenced this pull request Nov 29, 2021
Notable changes:

async_hooks:
  * (SEMVER-MINOR) expose async_wrap providers (Rafael Gonzaga) #40760
deps:
  * (SEMVER-MINOR) update V8 to 9.6.180.14 (Michaël Zasso) #40488
lib:
  * (SEMVER-MINOR) add reason to AbortSignal (James M Snell) #40807
src:
  * (SEMVER-MINOR) add x509.fingerprint512 to crypto module (3nprob) #39809
stream:
  * deprecate thenable support (Antoine du Hamel) #40860
  * fix finished regression when working with legacy Stream (Matteo Collina) #40858

PR-URL: #40983
targos added a commit that referenced this pull request Nov 30, 2021
Notable changes:

async_hooks:
  * (SEMVER-MINOR) expose async_wrap providers (Rafael Gonzaga) #40760
deps:
  * (SEMVER-MINOR) update V8 to 9.6.180.14 (Michaël Zasso) #40488
lib:
  * (SEMVER-MINOR) add reason to AbortSignal (James M Snell) #40807
src:
  * (SEMVER-MINOR) add x509.fingerprint512 to crypto module (3nprob) #39809
stream:
  * deprecate thenable support (Antoine du Hamel) #40860
  * fix finished regression when working with legacy Stream (Matteo Collina) #40858

PR-URL: #40983
@danielleadams
Copy link
Contributor

@mcollina can you create a backport PR for this if this also appears in 16.x? It broke a test in 16.x when trying to pull it in for the next release.

Linkgoron pushed a commit to Linkgoron/node that referenced this pull request Jan 31, 2022
Notable changes:

async_hooks:
  * (SEMVER-MINOR) expose async_wrap providers (Rafael Gonzaga) nodejs#40760
deps:
  * (SEMVER-MINOR) update V8 to 9.6.180.14 (Michaël Zasso) nodejs#40488
lib:
  * (SEMVER-MINOR) add reason to AbortSignal (James M Snell) nodejs#40807
src:
  * (SEMVER-MINOR) add x509.fingerprint512 to crypto module (3nprob) nodejs#39809
stream:
  * deprecate thenable support (Antoine du Hamel) nodejs#40860
  * fix finished regression when working with legacy Stream (Matteo Collina) nodejs#40858

PR-URL: nodejs#40983
@mcollina
Copy link
Member Author

mcollina commented Feb 1, 2022

This change does not backport cleanly and I don't think backporting it without the previous changes make sense. I have put the dont-land-x labels

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. commit-queue-squash Add this label to instruct the Commit Queue to squash all the PR commits into the first one. notable-change PRs with changes that should be highlighted in changelogs. stream Issues and PRs related to the stream subsystem.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

9 participants