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.

Sign up for GitHub

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

Jump to bottom

stream: make sure 'readable' is emitted before ending the stream #26059

Closed
wants to merge 3 commits into from
Closed

stream: make sure 'readable' is emitted before ending the stream #26059

wants to merge 3 commits into from

Conversation

mcollina
Copy link
Member

This change is born out of #25810, as I explored how to fix that difference.

This change could be considered semver-major or a bug fix. We need also to check some of the benchmarks if they are impacted before landing.

Fixes #25810

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

@nodejs-github-bot
Copy link
Collaborator

@mcollina build started: https://ci.nodejs.org/blue/organizations/jenkins/node-test-pull-request-lite-pipeline/detail/node-test-pull-request-lite-pipeline/2580/pipeline

@nodejs-github-bot nodejs-github-bot added the stream Issues and PRs related to the stream subsystem. label Feb 12, 2019
@mcollina mcollina mentioned this pull request Feb 14, 2019
Closed
lpinca
lpinca reviewed Feb 14, 2019
View reviewed changes
test/parallel/test-stream-readable-emit-readable-short-stream.js
const t = new stream.Transform({
transform: common.mustCall(function(chunk, encoding, callback) {
this.push(chunk);
return callback();
Copy link
Member

Choose a reason for hiding this comment

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

Nit that can be ignored, can we remove the return statement?

@mcollina mcollina force-pushed the readable-next-tick branch from dec410a to 833cb28 Compare March 2, 2019 19:36
@mcollina
Copy link
Member Author

mcollina commented Mar 2, 2019

CI: https://ci.nodejs.org/job/node-test-pull-request/21144/

@lpinca @jasnell PTAL

@jasnell
Copy link
Member

jasnell commented Mar 2, 2019

Still LGTM

@mcollina mcollina force-pushed the readable-next-tick branch from 833cb28 to c393c44 Compare March 4, 2019 15:29
@mcollina
Copy link
Member Author

mcollina commented Mar 4, 2019

CI: https://ci.nodejs.org/job/node-test-pull-request/21188/

lpinca
lpinca reviewed Mar 4, 2019
View reviewed changes
Copy link
Member

@lpinca lpinca left a comment •
edited
Loading

Choose a reason for hiding this comment

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

This is what I mean, it saves a common.mustNotCall() call :)

test/parallel/test-stream-readable-emittedReadable.js Outdated Show resolved Hide resolved
test/parallel/test-stream-readable-emittedReadable.js Outdated Show resolved Hide resolved
lpinca and others added 2 commits March 5, 2019 16:57
@lpinca @mcollina
Update test/parallel/test-stream-readable-emittedReadable.js
a90eb4f 
Co-Authored-By: mcollina <matteo.collina@gmail.com>
@lpinca @mcollina
Update test/parallel/test-stream-readable-emittedReadable.js
8c973a7 
Co-Authored-By: mcollina <matteo.collina@gmail.com>
@mcollina
Copy link
Member Author

mcollina commented Mar 5, 2019

I understand what you mean now!

CI: https://ci.nodejs.org/job/node-test-pull-request/21219/

@mcollina mcollina added the author ready PRs that have at least one approval, no pending requests for changes, and a CI started. label Mar 5, 2019
@mcollina
Copy link
Member Author

mcollina commented Mar 6, 2019

Landed in e95e7f9

@mcollina mcollina closed this Mar 6, 2019
@mcollina mcollina deleted the readable-next-tick branch March 6, 2019 08:40
mcollina added a commit that referenced this pull request Mar 6, 2019
@mcollina
stream: make sure 'readable' is emitted before ending the stream
e95e7f9 
Fixes: #25810

PR-URL: #26059
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
BridgeAR pushed a commit to BridgeAR/node that referenced this pull request Mar 12, 2019
@mcollina @BridgeAR
stream: make sure 'readable' is emitted before ending the stream
4796c7c 
Fixes: nodejs#25810

PR-URL: nodejs#26059
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
mcollina added a commit to mcollina/node that referenced this pull request Mar 13, 2019
@mcollina
streams: fix regression introduced in nodejs#26059
70c6b7d 
In nodejs#26059, we introduced a bug that caused 'readable' to be nextTicked
on EOF of a ReadableStream. This breaks the dicer module on CITGM.
That change was partially reverted to still fix the bug in nodejs#25810 and
not break dicer.

See: nodejs#26059
Fixes: nodejs#25810
@mcollina mcollina mentioned this pull request Mar 13, 2019
Closed
3 tasks
@BridgeAR BridgeAR mentioned this pull request Mar 13, 2019
Merged
@BridgeAR
Copy link
Member

I marked this as do not land on all release lines and I'll back this out again from the release proposal for v11.12.0.

@mcollina
Copy link
Member Author

@BridgeAR this must land with 269103a.

mcollina added a commit that referenced this pull request Mar 16, 2019
@mcollina
stream: fix regression introduced in #26059
269103a 
In #26059, we introduced a bug that caused 'readable' to be nextTicked
on EOF of a ReadableStream. This breaks the dicer module on CITGM.
That change was partially reverted to still fix the bug in #25810 and
not break dicer.

See: #26059
Fixes: #25810

PR-URL: #26643
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
targos pushed a commit to targos/node that referenced this pull request Mar 27, 2019
@mcollina @targos
stream: make sure 'readable' is emitted before ending the stream
6dbce34 
Fixes: nodejs#25810

PR-URL: nodejs#26059
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
targos pushed a commit to targos/node that referenced this pull request Mar 27, 2019
@mcollina @targos
stream: fix regression introduced in nodejs#26059
9fac2c3 
In nodejs#26059, we introduced a bug that caused 'readable' to be nextTicked
on EOF of a ReadableStream. This breaks the dicer module on CITGM.
That change was partially reverted to still fix the bug in nodejs#25810 and
not break dicer.

See: nodejs#26059
Fixes: nodejs#25810

PR-URL: nodejs#26643
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
targos pushed a commit that referenced this pull request Mar 27, 2019
@mcollina @targos
stream: make sure 'readable' is emitted before ending the stream
0b2f900 
Fixes: #25810

PR-URL: #26059
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
targos pushed a commit that referenced this pull request Mar 27, 2019
@mcollina @targos
stream: fix regression introduced in #26059
249bf50 
In #26059, we introduced a bug that caused 'readable' to be nextTicked
on EOF of a ReadableStream. This breaks the dicer module on CITGM.
That change was partially reverted to still fix the bug in #25810 and
not break dicer.

See: #26059
Fixes: #25810

PR-URL: #26643
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
@targos targos mentioned this pull request Mar 27, 2019
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@lpinca lpinca lpinca approved these changes

@jasnell jasnell jasnell approved these changes

@mafintosh mafintosh Awaiting requested review from mafintosh

@addaleax addaleax Awaiting requested review from addaleax

Assignees
No one assigned
Labels
author ready PRs that have at least one approval, no pending requests for changes, and a CI started. stream Issues and PRs related to the stream subsystem.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Transform stream misses final readable event for small inputs
7 participants
@mcollina @nodejs-github-bot @jasnell @BridgeAR @lpinca @targos @BethGriggs