GV

GeezerVison

The World Wide Web, restored to 800 pixels of glory.

Home|About|Gaming cabinet|Archives|How it works|Starter sites

On this site

  • Sign in
  • ActionsCI/CD and automation
  • CodespacesCloud development environments
  • All Features
  • Documentation
  • Training
  • Certifications
  • Build with Copilot
  • Customize Copilot
  • Integrations
  • Trending repos
  • GitHub Sponsors
  • Developer articles
  • Enterprises

This view

  • Original page
  • New address

GeezerVison is showing github.com — stream: always delay construct callback by a nextTick by mcollina · Pull Request #46818 · nodejs/node

Conversation

@mcollina

@mcollina mcollina commented Feb 24, 2023

Copy link
Copy Markdown
Member

Fixes: #46765

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
bricss and benjamingr reacted with thumbs up emoji
All reactions
  • 👍 2 reactions
@mcollina
mcollina requested a review from ronag February 24, 2023 15:48
@nodejs-github-bot

nodejs-github-bot commented Feb 24, 2023

Copy link
Copy Markdown
Collaborator

Review requested:

  • @nodejs/streams
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@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
stream.Transform changing order of items #46765
Closed
@mcollina
stream: always delay construct callback by a nextTick
e2fc14d 
Signed-off-by: Matteo Collina <[email protected]>
Fixes: nodejs#46765
@mcollina
mcollina force-pushed the fix-46765 branch from db2554c to e2fc14d Compare February 24, 2023 15:56
@mcollina mcollina added the request-ci Add this label to start a Jenkins CI on a PR. Only starts once the PR has an approving review. label Feb 24, 2023
@github-actions github-actions Bot removed the request-ci Add this label to start a Jenkins CI on a PR. Only starts once the PR has an approving review. label Feb 24, 2023
@nodejs-github-bot

nodejs-github-bot commented Feb 24, 2023

Copy link
Copy Markdown
Collaborator

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

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
ronag
ronag approved these changes Feb 24, 2023
View reviewed changes

@ronag ronag left a comment

Copy link
Copy Markdown
Member

There was a problem hiding this 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...

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
@mcollina

mcollina commented Feb 24, 2023

Copy link
Copy Markdown
Member Author

LGTM, though jump is a weird name...

what would you recommend?
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@ronag

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?
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@ronag

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);
}
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@mcollina
fixup
e65f743 
Signed-off-by: Matteo Collina <[email protected]>
@mcollina mcollina added the request-ci Add this label to start a Jenkins CI on a PR. Only starts once the PR has an approving review. label Feb 24, 2023
ronag
ronag approved these changes Feb 24, 2023
View reviewed changes
@github-actions github-actions Bot removed the request-ci Add this label to start a Jenkins CI on a PR. Only starts once the PR has an approving review. label Feb 24, 2023
@nodejs-github-bot

nodejs-github-bot commented Feb 24, 2023

Copy link
Copy Markdown
Collaborator

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

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
aduh95
aduh95 reviewed Feb 24, 2023
View reviewed changes
Comment thread lib/internal/streams/destroy.js Resolved

Uh oh!

There was an error while loading. Please reload this page.
@mcollina @aduh95
Update lib/internal/streams/destroy.js
fb1fba4 
Co-authored-by: Antoine du Hamel <[email protected]>
lpinca
lpinca approved these changes Feb 24, 2023
View reviewed changes
@ronag ronag added the request-ci Add this label to start a Jenkins CI on a PR. Only starts once the PR has an approving review. label Feb 24, 2023
@github-actions github-actions Bot removed the request-ci Add this label to start a Jenkins CI on a PR. Only starts once the PR has an approving review. label Feb 24, 2023
@nodejs-github-bot

nodejs-github-bot commented Feb 24, 2023

Copy link
Copy Markdown
Collaborator

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

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
jasnell
jasnell approved these changes Feb 24, 2023
View reviewed changes
@aduh95 aduh95 added author ready PRs with CI started, the required approvals, and no outstanding review comments. commit-queue PRs queued for automated landing through the Commit Queue. commit-queue-squash PRs the Commit Queue should land as one squashed commit. labels Feb 24, 2023
@bnoordhuis

bnoordhuis commented Feb 25, 2023 •
edited
Loading

Uh oh!

There was an error while loading. Please reload this page.

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?
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@github-actions github-actions Bot mentioned this pull request Feb 25, 2023
CI Reliability 2023-02-25 nodejs/reliability#506
Open
28 tasks
@mcollina

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.
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@mcollina mcollina added the request-ci Add this label to start a Jenkins CI on a PR. Only starts once the PR has an approving review. label Feb 25, 2023
@github-actions github-actions Bot removed the request-ci Add this label to start a Jenkins CI on a PR. Only starts once the PR has an approving review. label Feb 25, 2023
@nodejs-github-bot

nodejs-github-bot commented Feb 25, 2023

Copy link
Copy Markdown
Collaborator

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

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@nodejs-github-bot

nodejs-github-bot commented Feb 25, 2023

Copy link
Copy Markdown
Collaborator

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

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
benjamingr
benjamingr approved these changes Feb 25, 2023
View reviewed changes
@rluvaton rluvaton mentioned this pull request Feb 25, 2023
stream: when value is already in buffer don't emit the next one #46813
Closed
@github-actions github-actions Bot mentioned this pull request Feb 26, 2023
CI Reliability 2023-02-26 nodejs/reliability#507
Open
15 tasks
@nodejs-github-bot nodejs-github-bot removed the commit-queue PRs queued for automated landing through the Commit Queue. label Feb 26, 2023
@nodejs-github-bot
nodejs-github-bot merged commit 355bcbc into nodejs:main Feb 26, 2023
@nodejs-github-bot

nodejs-github-bot commented Feb 26, 2023

Copy link
Copy Markdown
Collaborator

Landed in 355bcbc
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
This was referenced Feb 27, 2023
CI Reliability 2023-02-27 nodejs/reliability#508
Open
CI Reliability 2023-02-28 nodejs/reliability#509
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 <[email protected]>
Fixes: #46765
PR-URL: #46818
Reviewed-By: Robert Nagy <[email protected]>
Reviewed-By: Luigi Pinca <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Benjamin Gruenbaum <[email protected]>
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 <[email protected]>
Fixes: #46765
PR-URL: #46818
Reviewed-By: Robert Nagy <[email protected]>
Reviewed-By: Luigi Pinca <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Benjamin Gruenbaum <[email protected]>
@targos targos mentioned this pull request Mar 14, 2023
v19.8.0 release proposal #47087
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 <[email protected]>
Fixes: #46765
PR-URL: #46818
Reviewed-By: Robert Nagy <[email protected]>
Reviewed-By: Luigi Pinca <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Benjamin Gruenbaum <[email protected]>
@danielleadams danielleadams mentioned this pull request Apr 11, 2023
v18.16.0 release proposal #47502
Merged
@acutus-fujii acutus-fujii mentioned this pull request Aug 1, 2023
一括登録すると，動画が再生できない npocccties/chibichilo#925
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
fix(ext/node): fix node:stream.Writable denoland/deno#21297
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
chore(ext/node): remove unnecessary test case denoland/deno#21337
Merged
This was referenced Mar 6, 2024
fs.createWriteStream can cause out-of-order writes, in v18.16+ #51993
Closed
stream: do not defer construction by one microtick #52005
Closed
@kt3k kt3k mentioned this pull request Apr 5, 2024
Node compatibility issue with handling multipart/form-data files denoland/deno#20284
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

Uh oh!

There was an error while loading. Please reload this page.

Assignees

No one assigned

Labels

author ready PRs with CI started, the required approvals, and no outstanding review comments. commit-queue-squash PRs the Commit Queue should land as one squashed commit. 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

Uh oh!

There was an error while loading. Please reload this page.

8 participants

@mcollina @nodejs-github-bot @ronag @bnoordhuis @jasnell @benjamingr @lpinca @aduh95
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
You are visitor 01402503 — GeezerVison does not store the pages you look up.
Best viewed with Netscape Navigator 4.7 or Internet Explorer 6 at 800×600. No cookies required.