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: Relocate the status checking code in the onWriteComplete #54032

Merged
merged 1 commit into from
Sep 2, 2024
Merged

stream: Relocate the status checking code in the onWriteComplete #54032

merged 1 commit into from
Sep 2, 2024

Conversation

MCprotein
Copy link
Contributor

@MCprotein MCprotein commented Jul 25, 2024 •
edited
Loading

Modified the code to check the status before verifying if the stream is destroyed.

error name refs:

/ / lib/internal/webstreams/adapter.js
  function onWriteComplete(status) {
    if (status < 0) {
      const error = new ErrnoException(status, 'write', this.error);
      this.promise.reject(error);
      this.controller.error(error);
      return;
    }
    this.promise.resolve();
  }

cc. @ronag

@nodejs-github-bot nodejs-github-bot added the needs-ci PRs that need a full CI run. label Jul 25, 2024
@MCprotein
stream: relocate the status checking code in the onwritecomplete
2dddcdd 
relocate the status checking code before verifying if the stream is
destroyed
@MCprotein MCprotein force-pushed the fix/stream-base-commons-todo branch from 7066edb to 2dddcdd Compare July 25, 2024 06:14
@jakecastelli jakecastelli added the stream Issues and PRs related to the stream subsystem. label Jul 27, 2024
@jakecastelli
Copy link
Member

cc. @nodejs/streams

@benjamingr benjamingr added the request-ci Add this label to start a Jenkins CI on a PR. label Jul 27, 2024
@benjamingr
Copy link
Member

I wonder how breaking this is, I'll also trigger a CITGM

@benjamingr
Copy link
Member

CITGM https://ci.nodejs.org/view/Node.js-citgm/job/citgm-smoker/3466/

@github-actions github-actions bot removed the request-ci Add this label to start a Jenkins CI on a PR. label Jul 27, 2024
@nodejs-github-bot
Copy link
Collaborator

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

ronag
ronag approved these changes Jul 27, 2024
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 assuming CITGM is ok

@daeyeon
Copy link
Member

daeyeon commented Jul 28, 2024

module test suite failed
Error: The canary is dead:

CITGM currently has an issue that is being investigated under nodejs/citgm#1067.

@nodejs-github-bot
Copy link
Collaborator

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

@MCprotein
Copy link
Contributor Author

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

@daeyeon
I think that the citgm is still unstable, should I wait for now?

@daeyeon
Copy link
Member

daeyeon commented Jul 29, 2024

Yes. We need to wait until the citgm result is available. Thanks in advance for your patience.

@MCprotein
Copy link
Contributor Author

Yes. We need to wait until the citgm result is available. Thanks in advance for your patience.

thank you for explaining 😄

mcollina
mcollina approved these changes Jul 29, 2024
View reviewed changes
Copy link
Member

@mcollina mcollina left a comment

Choose a reason for hiding this comment

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

lgtm with a green CITGM

@mcollina
Copy link
Member

mcollina commented Aug 2, 2024

CITGM: https://ci.nodejs.org/view/Node.js-citgm/job/citgm-smoker/3467/

@mcollina mcollina added baking-for-lts PRs that need to wait before landing in a LTS release. commit-queue Add this label to land a pull request using GitHub Actions. semver-minor PRs that contain new features and should be released in the next minor version. labels Sep 2, 2024
@nodejs-github-bot nodejs-github-bot removed the commit-queue Add this label to land a pull request using GitHub Actions. label Sep 2, 2024
@nodejs-github-bot nodejs-github-bot merged commit 298dea0 into nodejs:main Sep 2, 2024
65 checks passed
@nodejs-github-bot
Copy link
Collaborator

Landed in 298dea0

aduh95 pushed a commit that referenced this pull request Sep 12, 2024
@MCprotein @aduh95
stream: relocate the status checking code in the onwritecomplete
e2307d8 
relocate the status checking code before verifying if the stream is
destroyed

PR-URL: #54032
Reviewed-By: Robert Nagy <ronagy@icloud.com>
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
RafaelGSS added a commit that referenced this pull request Sep 16, 2024
@RafaelGSS
2024-09-17, Version 22.9.0 (Current)
6a9b4a3 
Notable changes:

lib:
  * (SEMVER-MINOR) add util.getCallSite() API (Rafael Gonzaga) #54380
repl:
  * doc-deprecate instantiating `node:repl` classes without `new` (Aviv Keller) #54842
src:
  * create handle scope in FastInternalModuleStat (Joyee Cheung) #54384
stream:
  * (SEMVER-MINOR) relocate the status checking code in the onwritecomplete (YoonSoo_Shin) #54032
tls:
  * (SEMVER-MINOR) add `allowPartialTrustChain` flag (Anna Henningsen) #54790
v8:
  * Revert "v8: enable maglev on supported architectures (Joyee Cheung) #54384

PR-URL: TODO
@RafaelGSS RafaelGSS mentioned this pull request Sep 16, 2024
Merged
RafaelGSS added a commit that referenced this pull request Sep 16, 2024
@RafaelGSS
2024-09-17, Version 22.9.0 (Current)
805dec0 
Notable changes:

lib:
  * (SEMVER-MINOR) add util.getCallSite() API (Rafael Gonzaga) #54380
repl:
  * doc-deprecate instantiating `node:repl` classes without `new` (Aviv Keller) #54842
src:
  * create handle scope in FastInternalModuleStat (Joyee Cheung) #54384
stream:
  * (SEMVER-MINOR) relocate the status checking code in the onwritecomplete (YoonSoo_Shin) #54032
tls:
  * (SEMVER-MINOR) add `allowPartialTrustChain` flag (Anna Henningsen) #54790
v8:
  * Revert "v8: enable maglev on supported architectures (Joyee Cheung) #54384

PR-URL: #54966
RafaelGSS added a commit that referenced this pull request Sep 16, 2024
@RafaelGSS
2024-09-17, Version 22.9.0 (Current)
3519296 
Notable changes:

lib:
  * (SEMVER-MINOR) add util.getCallSite() API (Rafael Gonzaga) #54380
repl:
  * doc-deprecate instantiating `node:repl` classes without `new` (Aviv Keller) #54842
src:
  * create handle scope in FastInternalModuleStat (Joyee Cheung) #54384
stream:
  * (SEMVER-MINOR) relocate the status checking code in the onwritecomplete (YoonSoo_Shin) #54032
tls:
  * (SEMVER-MINOR) add `allowPartialTrustChain` flag (Anna Henningsen) #54790
v8:
  * Revert "v8: enable maglev on supported architectures (Joyee Cheung) #54384

PR-URL: #54966
RafaelGSS added a commit that referenced this pull request Sep 16, 2024
@RafaelGSS
2024-09-17, Version 22.9.0 (Current)
70628d5 
Notable changes:

lib:
  * (SEMVER-MINOR) add util.getCallSite() API (Rafael Gonzaga) #54380
repl:
  * doc-deprecate instantiating `node:repl` classes without `new` (Aviv Keller) #54842
src:
  * create handle scope in FastInternalModuleStat (Joyee Cheung) #54384
stream:
  * (SEMVER-MINOR) relocate the status checking code in the onwritecomplete (YoonSoo_Shin) #54032
tls:
  * (SEMVER-MINOR) add `allowPartialTrustChain` flag (Anna Henningsen) #54790
v8:
  * Revert "v8: enable maglev on supported architectures (Joyee Cheung) #54384

PR-URL: #54966
RafaelGSS added a commit that referenced this pull request Sep 16, 2024
@RafaelGSS
2024-09-17, Version 22.9.0 (Current)
7790174 
Notable changes:

lib:
  * (SEMVER-MINOR) add util.getCallSite() API (Rafael Gonzaga) #54380
repl:
  * doc-deprecate instantiating `node:repl` classes without `new` (Aviv Keller) #54842
src:
  * create handle scope in FastInternalModuleStat (Joyee Cheung) #54384
stream:
  * (SEMVER-MINOR) relocate the status checking code in the onwritecomplete (YoonSoo_Shin) #54032
tls:
  * (SEMVER-MINOR) add `allowPartialTrustChain` flag (Anna Henningsen) #54790
v8:
  * Revert "v8: enable maglev on supported architectures (Joyee Cheung) #54384

PR-URL: #54966
RafaelGSS added a commit that referenced this pull request Sep 16, 2024
@RafaelGSS
2024-09-17, Version 22.9.0 (Current)
3384542 
Notable changes:

lib:
  * (SEMVER-MINOR) add util.getCallSite() API (Rafael Gonzaga) #54380
repl:
  * doc-deprecate instantiating `node:repl` classes without `new` (Aviv Keller) #54842
src:
  * create handle scope in FastInternalModuleStat (Joyee Cheung) #54384
stream:
  * (SEMVER-MINOR) relocate the status checking code in the onwritecomplete (YoonSoo_Shin) #54032
tls:
  * (SEMVER-MINOR) add `allowPartialTrustChain` flag (Anna Henningsen) #54790
v8:
  * Revert "v8: enable maglev on supported architectures (Joyee Cheung) #54384

PR-URL: #54966
RafaelGSS added a commit that referenced this pull request Sep 16, 2024
@RafaelGSS
2024-09-17, Version 22.9.0 (Current)
9fcecb9 
Notable changes:

lib:
  * (SEMVER-MINOR) add util.getCallSite() API (Rafael Gonzaga) #54380
repl:
  * doc-deprecate instantiating `node:repl` classes without `new` (Aviv Keller) #54842
src:
  * create handle scope in FastInternalModuleStat (Joyee Cheung) #54384
stream:
  * (SEMVER-MINOR) relocate the status checking code in the onwritecomplete (YoonSoo_Shin) #54032
tls:
  * (SEMVER-MINOR) add `allowPartialTrustChain` flag (Anna Henningsen) #54790
v8:
  * Revert "v8: enable maglev on supported architectures (Joyee Cheung) #54384

PR-URL: #54966
RafaelGSS added a commit that referenced this pull request Sep 17, 2024
@RafaelGSS
2024-09-17, Version 22.9.0 (Current)
85a9935 
Notable changes:

lib:
  * (SEMVER-MINOR) add util.getCallSite() API (Rafael Gonzaga) #54380
repl:
  * doc-deprecate instantiating `node:repl` classes without `new` (Aviv Keller) #54842
src:
  * create handle scope in FastInternalModuleStat (Joyee Cheung) #54384
stream:
  * (SEMVER-MINOR) relocate the status checking code in the onwritecomplete (YoonSoo_Shin) #54032
tls:
  * (SEMVER-MINOR) add `allowPartialTrustChain` flag (Anna Henningsen) #54790
v8:
  * Revert "v8: enable maglev on supported architectures (Joyee Cheung) #54384

PR-URL: #54966
RafaelGSS added a commit that referenced this pull request Sep 17, 2024
@RafaelGSS
2024-09-17, Version 22.9.0 (Current)
9cc8a19 
Notable changes:

lib:
  * (SEMVER-MINOR) add util.getCallSite() API (Rafael Gonzaga) #54380
repl:
  * doc-deprecate instantiating `node:repl` classes without `new` (Aviv Keller) #54842
src:
  * create handle scope in FastInternalModuleStat (Joyee Cheung) #54384
stream:
  * (SEMVER-MINOR) relocate the status checking code in the onwritecomplete (YoonSoo_Shin) #54032
tls:
  * (SEMVER-MINOR) add `allowPartialTrustChain` flag (Anna Henningsen) #54790
v8:
  * Revert "v8: enable maglev on supported architectures (Joyee Cheung) #54384

PR-URL: #54966
RafaelGSS added a commit that referenced this pull request Sep 17, 2024
@RafaelGSS
2024-09-17, Version 22.9.0 (Current)
4631be0 
Notable changes:

lib:
  * (SEMVER-MINOR) add util.getCallSite() API (Rafael Gonzaga) #54380
repl:
  * doc-deprecate instantiating `node:repl` classes without `new` (Aviv Keller) #54842
src:
  * create handle scope in FastInternalModuleStat (Joyee Cheung) #54384
stream:
  * (SEMVER-MINOR) relocate the status checking code in the onwritecomplete (YoonSoo_Shin) #54032
tls:
  * (SEMVER-MINOR) add `allowPartialTrustChain` flag (Anna Henningsen) #54790
v8:
  * Revert "v8: enable maglev on supported architectures (Joyee Cheung) #54384

PR-URL: #54966
RafaelGSS added a commit that referenced this pull request Sep 17, 2024
@RafaelGSS
2024-09-17, Version 22.9.0 (Current)
e49cf7a 
Notable changes:

lib:
  * (SEMVER-MINOR) add util.getCallSite() API (Rafael Gonzaga) #54380
repl:
  * doc-deprecate instantiating `node:repl` classes without `new` (Aviv Keller) #54842
src:
  * create handle scope in FastInternalModuleStat (Joyee Cheung) #54384
stream:
  * (SEMVER-MINOR) relocate the status checking code in the onwritecomplete (YoonSoo_Shin) #54032
tls:
  * (SEMVER-MINOR) add `allowPartialTrustChain` flag (Anna Henningsen) #54790
v8:
  * Revert "v8: enable maglev on supported architectures (Joyee Cheung) #54384

PR-URL: #54966
louwers pushed a commit to louwers/node that referenced this pull request Nov 2, 2024
@MCprotein @louwers
stream: relocate the status checking code in the onwritecomplete
1e8fcc0 
relocate the status checking code before verifying if the stream is
destroyed

PR-URL: nodejs#54032
Reviewed-By: Robert Nagy <ronagy@icloud.com>
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
louwers pushed a commit to louwers/node that referenced this pull request Nov 2, 2024
@RafaelGSS @louwers
2024-09-17, Version 22.9.0 (Current)
c49d5c3 
Notable changes:

lib:
  * (SEMVER-MINOR) add util.getCallSite() API (Rafael Gonzaga) nodejs#54380
repl:
  * doc-deprecate instantiating `node:repl` classes without `new` (Aviv Keller) nodejs#54842
src:
  * create handle scope in FastInternalModuleStat (Joyee Cheung) nodejs#54384
stream:
  * (SEMVER-MINOR) relocate the status checking code in the onwritecomplete (YoonSoo_Shin) nodejs#54032
tls:
  * (SEMVER-MINOR) add `allowPartialTrustChain` flag (Anna Henningsen) nodejs#54790
v8:
  * Revert "v8: enable maglev on supported architectures (Joyee Cheung) nodejs#54384

PR-URL: nodejs#54966
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@ronag ronag ronag approved these changes

@jasnell jasnell jasnell approved these changes

@mcollina mcollina mcollina approved these changes

Assignees
No one assigned
Labels
baking-for-lts PRs that need to wait before landing in a LTS release. needs-ci PRs that need a full CI run. semver-minor PRs that contain new features and should be released in the next minor version. 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.
8 participants
@MCprotein @jakecastelli @benjamingr @nodejs-github-bot @daeyeon @mcollina @jasnell @ronag