FazBrowse GitHub Viewer | Trending |
URL:
| Home
Tools: [Download Repo ZIP]   [Original HTTPS Page]

stream: always delay construct callback by a nextTick by mcollina · Pull Request #46818 · nodejs/node · GitHub

/ node Public

stream: always delay construct callback by a nextTick - #46818

Merged
nodejs-github-bot merged 3 commits into
nodejs:mainfrom
mcollina:fix-46765
Feb 26, 2023
ConversationCommits (3)ChecksFiles changed
Merged

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

Conversation

mcollina commented Feb 24, 2023

Copy link
Copy Markdown
Member

Fixes: #46765

mcollina requested a review from ronag February 24, 2023 15:48

nodejs-github-bot commented Feb 24, 2023

Copy link
Copy Markdown
Collaborator

Review requested:

  • @nodejs/streams

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

Copy link
Copy Markdown
Collaborator

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

ronag approved these changes Feb 24, 2023
View reviewed changes

ronag left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

LGTM, though jump is a weird name...

mcollina commented Feb 24, 2023

Copy link
Copy Markdown
Member Author

LGTM, though jump is a weird name...

what would you recommend?

ronag commented Feb 24, 2023

Copy link
Copy Markdown
Member

LGTM, though jump is a weird name...

what would you recommend?

onConstructNT and onConstruct, like we usually do?

ronag commented Feb 24, 2023

Copy link
Copy Markdown
Member

Or:

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

fixup
e65f743 
Signed-off-by: Matteo Collina <hello@matteocollina.com>
mcollina added the request-ci Add this label to start a Jenkins CI on a PR. label Feb 24, 2023
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 commented Feb 24, 2023

Copy link
Copy Markdown
Collaborator

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

aduh95 reviewed Feb 24, 2023
View reviewed changes
Comment thread lib/internal/streams/destroy.js Outdated
Update lib/internal/streams/destroy.js
fb1fba4 
Co-authored-by: Antoine du Hamel <duhamelantoine1995@gmail.com>
ronag added the request-ci Add this label to start a Jenkins CI on a PR. label Feb 24, 2023
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 commented Feb 24, 2023

Copy link
Copy Markdown
Collaborator

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

aduh95 added author ready PRs that have at least one approval, no outstanding review comments, 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 commented Feb 25, 2023 •
edited
Loading

Copy link
Copy Markdown
Member

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 Bot mentioned this pull request Feb 25, 2023
Open
28 tasks

mcollina commented Feb 25, 2023

Copy link
Copy Markdown
Member Author

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 added the request-ci Add this label to start a Jenkins CI on a PR. label Feb 25, 2023
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 commented Feb 25, 2023

Copy link
Copy Markdown
Collaborator

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

nodejs-github-bot commented Feb 25, 2023

Copy link
Copy Markdown
Collaborator

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

rluvaton mentioned this pull request Feb 25, 2023
Closed
github-actions Bot mentioned this pull request Feb 26, 2023
Open
15 tasks
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 merged commit 355bcbc into nodejs:main Feb 26, 2023

nodejs-github-bot commented Feb 26, 2023

Copy link
Copy Markdown
Collaborator

Landed in 355bcbc

This was referenced Feb 27, 2023
Open
Open
targos pushed a commit that referenced this pull request Mar 13, 2023
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
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 mentioned this pull request Mar 14, 2023
Merged
danielleadams pushed a commit that referenced this pull request Apr 11, 2023
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 mentioned this pull request Apr 11, 2023
Merged
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
fix(ext/node): fix stream.Writable
95e455d 
port nodejs/node#46818
kt3k mentioned this pull request Nov 22, 2023
Merged
kt3k added a commit to denoland/deno that referenced this pull request Nov 23, 2023
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
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
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 mentioned this pull request Nov 25, 2023
Merged
This was referenced Mar 6, 2024
Closed
Closed
kt3k mentioned this pull request Apr 5, 2024
Closed
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters. Learn more about bidirectional Unicode characters
Show hidden characters
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

aduh95 aduh95 left review comments
jasnell jasnell approved these changes
benjamingr benjamingr approved these changes
lpinca lpinca approved these changes
ronag ronag approved these changes

Assignees

No one assigned

Labels

author ready PRs that have at least one approval, no outstanding review comments, 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

Back | FazBrowse Home | New Git URL