Skip to content

stream: always delay construct callback by a nextTick#46818

Merged
nodejs-github-bot merged 3 commits intonodejs:mainfrom
mcollina:fix-46765
Feb 26, 2023
Merged

stream: always delay construct callback by a nextTick#46818
nodejs-github-bot merged 3 commits intonodejs:mainfrom
mcollina:fix-46765

Conversation

@mcollina
Copy link
Member

@mcollina mcollina commented Feb 24, 2023

Fixes: #46765

@mcollina mcollina requested a review from ronag February 24, 2023 15:48
@nodejs-github-bot
Copy link
Collaborator

nodejs-github-bot commented Feb 24, 2023

Review requested:

  • @nodejs/streams

@nodejs-github-bot nodejs-github-bot added the needs-ci PRs that need a full CI run. label Feb 24, 2023
@mcollina mcollina mentioned this pull request Feb 24, 2023
Closed
@mcollina
stream: always delay construct callback by a nextTick
e2fc14d 
Signed-off-by: Matteo Collina <hello@matteocollina.com>
Fixes: nodejs#46765
@mcollina mcollina added the request-ci Add this label to start a Jenkins CI on a PR. label Feb 24, 2023
@github-actions github-actions bot removed the request-ci Add this label to start a Jenkins CI on a PR. label Feb 24, 2023
@nodejs-github-bot
Copy link
Collaborator

nodejs-github-bot commented Feb 24, 2023

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

ronag
ronag approved these changes Feb 24, 2023
View reviewed changes
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.

LGTM, though jump is a weird name...

@mcollina
Copy link
Member Author

mcollina commented Feb 24, 2023

LGTM, though jump is a weird name...

what would you recommend?

@ronag
Copy link
Member

ronag commented Feb 24, 2023

LGTM, though jump is a weird name...

what would you recommend?

onConstructNT and onConstruct, like we usually do?

@ronag
Copy link
Member

ronag commented Feb 24, 2023

Or:

try {
  stream._construct(err => {
    process.nextTick(onConstruct, err);
  });
} catch (err) {
  process.nextTick(onConstruct, err);
}

@mcollina
fixup
e65f743 
Signed-off-by: Matteo Collina <hello@matteocollina.com>
@mcollina mcollina added the request-ci Add this label to start a Jenkins CI on a PR. label Feb 24, 2023
@github-actions github-actions bot removed the request-ci Add this label to start a Jenkins CI on a PR. label Feb 24, 2023
@nodejs-github-bot
Copy link
Collaborator

nodejs-github-bot commented Feb 24, 2023

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

aduh95
aduh95 reviewed Feb 24, 2023
View reviewed changes
@mcollina @aduh95
Update lib/internal/streams/destroy.js
fb1fba4 
Co-authored-by: Antoine du Hamel <duhamelantoine1995@gmail.com>
@ronag ronag added the request-ci Add this label to start a Jenkins CI on a PR. label Feb 24, 2023
@github-actions github-actions bot removed the request-ci Add this label to start a Jenkins CI on a PR. label Feb 24, 2023
@nodejs-github-bot
Copy link
Collaborator

nodejs-github-bot commented Feb 24, 2023

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

@aduh95 aduh95 added author ready PRs that have at least one approval, no pending requests for changes, and a CI started. 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 Feb 24, 2023
@bnoordhuis
Copy link
Member

bnoordhuis commented Feb 25, 2023 •
edited
Loading

Just double-checking: with this change there are now three (3) process.nextTick calls involved in a construct call: constructNT -> onConstruct -> emitConstructNT. That's intentional?

@github-actions github-actions bot mentioned this pull request Feb 25, 2023
Open
28 tasks
@mcollina
Copy link
Member Author

mcollina commented Feb 25, 2023

Just double-checking: with this change there are now three (3) process.nextTick calls involved in a construct call: constructNT -> onConstruct -> emitConstructNT. That's intentional?

Good spot! I was able to remove one.

@mcollina mcollina added the request-ci Add this label to start a Jenkins CI on a PR. label Feb 25, 2023
@github-actions github-actions bot removed the request-ci Add this label to start a Jenkins CI on a PR. label Feb 25, 2023
@nodejs-github-bot
Copy link
Collaborator

nodejs-github-bot commented Feb 25, 2023

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

@nodejs-github-bot
Copy link
Collaborator

nodejs-github-bot commented Feb 25, 2023

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

@rluvaton rluvaton mentioned this pull request Feb 25, 2023
Closed
@github-actions github-actions bot mentioned this pull request Feb 26, 2023
Open
15 tasks
@nodejs-github-bot nodejs-github-bot removed the commit-queue Add this label to land a pull request using GitHub Actions. label Feb 26, 2023
@nodejs-github-bot nodejs-github-bot merged commit 355bcbc into nodejs:main Feb 26, 2023
@nodejs-github-bot
Copy link
Collaborator

nodejs-github-bot commented Feb 26, 2023

Landed in 355bcbc

This was referenced Feb 27, 2023
Open
Open
targos pushed a commit that referenced this pull request Mar 13, 2023
@mcollina @targos
stream: always delay construct callback by a nextTick
b7bb4e3 
Signed-off-by: Matteo Collina <hello@matteocollina.com>
Fixes: #46765
PR-URL: #46818
Reviewed-By: Robert Nagy <ronagy@icloud.com>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com>
targos pushed a commit that referenced this pull request Mar 14, 2023
@mcollina @targos
stream: always delay construct callback by a nextTick
4ad48d9 
Signed-off-by: Matteo Collina <hello@matteocollina.com>
Fixes: #46765
PR-URL: #46818
Reviewed-By: Robert Nagy <ronagy@icloud.com>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com>
@targos targos mentioned this pull request Mar 14, 2023
Merged
danielleadams pushed a commit that referenced this pull request Apr 11, 2023
@mcollina @danielleadams
stream: always delay construct callback by a nextTick
654b747 
Signed-off-by: Matteo Collina <hello@matteocollina.com>
Fixes: #46765
PR-URL: #46818
Reviewed-By: Robert Nagy <ronagy@icloud.com>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com>
@danielleadams danielleadams mentioned this pull request Apr 11, 2023
Merged
@acutus-fujii acutus-fujii mentioned this pull request Aug 1, 2023
Closed
kt3k added a commit to kt3k/deno that referenced this pull request Nov 22, 2023
@kt3k
fix(ext/node): fix stream.Writable
95e455d 
port nodejs/node#46818
@kt3k kt3k mentioned this pull request Nov 22, 2023
Merged
kt3k added a commit to denoland/deno that referenced this pull request Nov 23, 2023
@kt3k
fix(ext/node): fix node:stream.Writable (#21297)
bf42467 
This change applies the same fix as
nodejs/node#46818, and the original example
given in #20456 works as expected.

closes #20456
crowlKats pushed a commit to denoland/deno that referenced this pull request Nov 24, 2023
@kt3k @crowlKats
fix(ext/node): fix node:stream.Writable (#21297)
545d6d1 
This change applies the same fix as
nodejs/node#46818, and the original example
given in #20456 works as expected.

closes #20456

(cherry picked from commit bf42467)
bartlomieju pushed a commit to denoland/deno that referenced this pull request Nov 24, 2023
@kt3k @bartlomieju
fix(ext/node): fix node:stream.Writable (#21297)
d0574f7 
This change applies the same fix as
nodejs/node#46818, and the original example
given in #20456 works as expected.

closes #20456
@kt3k kt3k mentioned this pull request Nov 25, 2023
Merged
This was referenced Mar 6, 2024
Closed
Closed
@kt3k kt3k mentioned this pull request Apr 5, 2024
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@aduh95 aduh95 aduh95 left review comments

@jasnell jasnell jasnell approved these changes

@benjamingr benjamingr benjamingr approved these changes

@lpinca lpinca lpinca approved these changes

@ronag ronag ronag approved these changes

Assignees

No one assigned

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. needs-ci PRs that need a full CI run.

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

stream.Transform changing order of items

8 participants

@mcollina @nodejs-github-bot @ronag @bnoordhuis @jasnell @benjamingr @lpinca @aduh95