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: don't push null from closed promise #42694 #45026

Merged
merged 1 commit into from
Oct 22, 2022
Merged

stream: don't push null from closed promise #42694 #45026

merged 1 commit into from
Oct 22, 2022

Conversation

davedoesdev
Copy link
Contributor

@davedoesdev davedoesdev commented Oct 16, 2022

closed promise is subscribed to first so will be
resolved first, before any read promise.

This causes data after EOF error to be thrown.

Remove the push null from the closed promise handler. The push null gets done from the read handler
when it detects done.

Fixes: #42694

@nodejs-github-bot nodejs-github-bot added needs-ci PRs that need a full CI run. web streams labels Oct 16, 2022
@davedoesdev
Copy link
Contributor Author

Fixes #42694

@Trott Trott added the request-ci Add this label to start a Jenkins CI on a PR. label Oct 16, 2022
@github-actions github-actions bot removed the request-ci Add this label to start a Jenkins CI on a PR. label Oct 16, 2022
@nodejs-github-bot
Copy link
Collaborator

Copy link
Member

@daeyeon daeyeon left a comment

Choose a reason for hiding this comment

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

Nit: Adding a test for stream.Duplex would be better since it's also fixed.

closed promise is subscribed to first so will be
resolved first, before any read promise.

This causes data after EOF error to be thrown.

Remove the push null from the closed promise handler.
The push null gets done from the read handler
when it detects done.
@daeyeon daeyeon 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. labels Oct 19, 2022
@github-actions github-actions bot removed the request-ci Add this label to start a Jenkins CI on a PR. label Oct 19, 2022
@nodejs-github-bot

This comment was marked as outdated.

@nodejs-github-bot

This comment was marked as outdated.

@nodejs-github-bot
Copy link
Collaborator

@daeyeon daeyeon added the commit-queue Add this label to land a pull request using GitHub Actions. label Oct 22, 2022
@nodejs-github-bot nodejs-github-bot removed the commit-queue Add this label to land a pull request using GitHub Actions. label Oct 22, 2022
@nodejs-github-bot nodejs-github-bot merged commit 538d8ce into nodejs:main Oct 22, 2022
@nodejs-github-bot
Copy link
Collaborator

Landed in 538d8ce

RafaelGSS pushed a commit that referenced this pull request Nov 1, 2022
closed promise is subscribed to first so will be
resolved first, before any read promise.

This causes data after EOF error to be thrown.

Remove the push null from the closed promise handler.
The push null gets done from the read handler
when it detects done.

PR-URL: #45026
Fixes: #42694
Reviewed-By: Daeyeon Jeong <daeyeon.dev@gmail.com>
Reviewed-By: Minwoo Jung <nodecorelab@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
@RafaelGSS RafaelGSS mentioned this pull request Nov 1, 2022
@BethGriggs BethGriggs added the lts-watch-v18.x PRs that may need to be released in v18.x. label Nov 3, 2022
RafaelGSS pushed a commit that referenced this pull request Nov 10, 2022
closed promise is subscribed to first so will be
resolved first, before any read promise.

This causes data after EOF error to be thrown.

Remove the push null from the closed promise handler.
The push null gets done from the read handler
when it detects done.

PR-URL: #45026
Fixes: #42694
Reviewed-By: Daeyeon Jeong <daeyeon.dev@gmail.com>
Reviewed-By: Minwoo Jung <nodecorelab@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
@Holi0317
Copy link

Sorry to interrupt. Will this PR get backported to node 18? I am currently getting hit by this bug.

danielleadams pushed a commit that referenced this pull request Dec 30, 2022
closed promise is subscribed to first so will be
resolved first, before any read promise.

This causes data after EOF error to be thrown.

Remove the push null from the closed promise handler.
The push null gets done from the read handler
when it detects done.

PR-URL: #45026
Fixes: #42694
Reviewed-By: Daeyeon Jeong <daeyeon.dev@gmail.com>
Reviewed-By: Minwoo Jung <nodecorelab@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
danielleadams pushed a commit that referenced this pull request Dec 30, 2022
closed promise is subscribed to first so will be
resolved first, before any read promise.

This causes data after EOF error to be thrown.

Remove the push null from the closed promise handler.
The push null gets done from the read handler
when it detects done.

PR-URL: #45026
Fixes: #42694
Reviewed-By: Daeyeon Jeong <daeyeon.dev@gmail.com>
Reviewed-By: Minwoo Jung <nodecorelab@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
danielleadams pushed a commit that referenced this pull request Jan 3, 2023
closed promise is subscribed to first so will be
resolved first, before any read promise.

This causes data after EOF error to be thrown.

Remove the push null from the closed promise handler.
The push null gets done from the read handler
when it detects done.

PR-URL: #45026
Fixes: #42694
Reviewed-By: Daeyeon Jeong <daeyeon.dev@gmail.com>
Reviewed-By: Minwoo Jung <nodecorelab@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
@targos targos added backported-to-v18.x PRs backported to the v18.x-staging branch. and removed lts-watch-v18.x PRs that may need to be released in v18.x. labels Oct 27, 2023
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. backported-to-v18.x PRs backported to the v18.x-staging branch. needs-ci PRs that need a full CI run. web streams
Projects
None yet
Development

Successfully merging this pull request may close these issues.

'stream.push() after EOF' error when using Readable.fromWeb
9 participants