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

[v18.x backport] lib: refactor transferable AbortSignal #44941

Closed
wants to merge 1 commit into from
Closed

[v18.x backport] lib: refactor transferable AbortSignal #44941

wants to merge 1 commit into from
+105 −12

Conversation

flakey5
Copy link
Member

@flakey5 flakey5 commented Oct 9, 2022

Backport of #44048 to v18.x

cc @mcollina

Co-authored-by: James M Snell jasnell@gmail.com
PR-URL: #44048
Reviewed-By: Matteo Collina matteo.collina@gmail.com
Reviewed-By: Stephen Belanger admin@stephenbelanger.com
Reviewed-By: Paolo Insogna paolo@cowtech.it
Reviewed-By: Joyee Cheung joyeec9h3@gmail.com
Reviewed-By: Antoine du Hamel duhamelantoine1995@gmail.com

@flakey5 @jasnell
lib: refactor transferable AbortSignal
45f0b92 
Co-authored-by: James M Snell <jasnell@gmail.com>
PR-URL: nodejs#44048
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
Reviewed-By: Stephen Belanger <admin@stephenbelanger.com>
Reviewed-By: Paolo Insogna <paolo@cowtech.it>
Reviewed-By: Joyee Cheung <joyeec9h3@gmail.com>
Reviewed-By: Antoine du Hamel <duhamelantoine1995@gmail.com>
@nodejs-github-bot nodejs-github-bot added needs-ci PRs that need a full CI run. util Issues and PRs related to the built-in util module. v18.x Issues that can be reproduced on v18.x or PRs targeting the v18.x-staging branch. worker Issues and PRs related to Worker support. labels Oct 9, 2022
@flakey5 flakey5 mentioned this pull request Oct 9, 2022
Closed
@aduh95 aduh95 changed the title [v18.x] lib: refactor transferable AbortSignal [v18.x backport] lib: refactor transferable AbortSignal Oct 9, 2022
jasnell
jasnell approved these changes Oct 9, 2022
View reviewed changes
Copy link
Member

@jasnell jasnell left a comment

Choose a reason for hiding this comment

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

LGTM but obviously we need a releaser to take a look. @nodejs/releasers

mcollina
mcollina approved these changes Oct 9, 2022
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

@RafaelGSS RafaelGSS added the request-ci Add this label to start a Jenkins CI on a PR. label Oct 9, 2022
@github-actions github-actions bot removed the request-ci Add this label to start a Jenkins CI on a PR. label Oct 9, 2022
@nodejs-github-bot

This comment was marked as outdated.

Sign in to view
@nodejs-github-bot

This comment was marked as outdated.

Sign in to view
@nodejs-github-bot

This comment was marked as outdated.

Sign in to view
@nodejs-github-bot

This comment was marked as outdated.

Sign in to view
@nodejs-github-bot

This comment was marked as outdated.

Sign in to view
@nodejs-github-bot
Copy link
Collaborator

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

@danielleadams
Copy link
Contributor

Hi @flakey5 - it looks like this test is breaking on one of the machines. Since it's a OOM error, I think the tuning on the test needs to be adjusted, and since it's an abort test, I think it's related.

I'm going to move forward with the next v18 release, but once this is fixed it should be good to go for the next v18 release (but it will probably need a rebase).

@danielleadams
Copy link
Contributor

Hi @flakey5 - it looks like this test is breaking on one of the machines. Since it's a OOM error, I think the tuning on the test needs to be adjusted, and since it's an abort test, I think it's related.

I'm going to move forward with the next v18 release, but once this is fixed it should be good to go for the next v18 release (but it will probably need a rebase).

This seems to be flakey in the staging branch too, so I'm going to go ahead and land this.

danielleadams pushed a commit that referenced this pull request Oct 11, 2022
@flakey5 @jasnell @danielleadams
lib: refactor transferable AbortSignal
3f20e5b 
Co-authored-by: James M Snell <jasnell@gmail.com>
PR-URL: #44048
Backport-PR-URL: #44941
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
Reviewed-By: Stephen Belanger <admin@stephenbelanger.com>
Reviewed-By: Paolo Insogna <paolo@cowtech.it>
Reviewed-By: Joyee Cheung <joyeec9h3@gmail.com>
Reviewed-By: Antoine du Hamel <duhamelantoine1995@gmail.com>
@danielleadams
Copy link
Contributor

Landed in 3f20e5b

@github-actions github-actions bot mentioned this pull request Oct 12, 2022
Open
19 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@jasnell jasnell jasnell approved these changes

@mcollina mcollina mcollina approved these changes

Assignees
No one assigned
Labels
needs-ci PRs that need a full CI run. util Issues and PRs related to the built-in util module. v18.x Issues that can be reproduced on v18.x or PRs targeting the v18.x-staging branch. worker Issues and PRs related to Worker support.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
6 participants
@flakey5 @nodejs-github-bot @danielleadams @mcollina @jasnell @RafaelGSS