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.

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

lib: refactor transferable AbortSignal #44048

Conversation

flakey5
Copy link
Contributor

@flakey5 flakey5 commented Jul 30, 2022

Took over the pr from @jasnell with his permission. Original pr was #43388 which can be closed now, feedback comments from there were addressed here.

Co-authored-by: James M Snell jasnell@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. worker Issues and PRs related to Worker support. labels Jul 30, 2022
@jasnell
Copy link
Member

jasnell commented Jul 30, 2022

+1! Btw, so folks know, this is my son. He wanted something to help get started contributing so I recommended that he take over this change.

/Cc @mcollina

@jasnell
Copy link
Member

jasnell commented Jul 30, 2022

Ok, it looks like there was a change that landed recently that changed AbortController to use a private #signal field instead of the internal kSymbol.... unfortunately that isn't going to work with the new util.transferableAbortController() API since that needs to be able to create the AbortController instance without using the default constructor. /cc @joyeecheung ...

@flakey5 ... the patch below fixes the error here. You'll want to apply this patch locally, add it as a fixup commit (lookup how to use git commit --fixup) and push the additional commit up

diff --git a/lib/internal/abort_controller.js b/lib/internal/abort_controller.js
index 313aee54fe..c5842a6638 100644
--- a/lib/internal/abort_controller.js
+++ b/lib/internal/abort_controller.js
@@ -82,6 +82,7 @@ const kAborted = Symbol('kAborted');
 const kReason = Symbol('kReason');
 const kCloneData = Symbol('kCloneData');
 const kTimeout = Symbol('kTimeout');
+const kSignal = Symbol('kSignal');
 
 function customInspect(self, obj, depth, options) {
   if (depth < 0)
@@ -309,20 +310,23 @@ function abortSignal(signal, reason) {
 }
 
 class AbortController {
-  #signal = createAbortSignal();
+  // We can't use a private symbol here because doing so would prevent
+  // transferableAbortController from being able to set the transferable
+  // signal.
+  [kSignal] = createAbortSignal();
 
   /**
    * @type {AbortSignal}
    */
   get signal() {
-    return this.#signal;
+    return this[kSignal];
   }
 
   /**
    * @param {any} reason
    */
   abort(reason = new DOMException('This operation was aborted', 'AbortError')) {
-    abortSignal(this.#signal, reason);
+    abortSignal(this[kSignal], reason);
   }
 
   [customInspectSymbol](depth, options) {

@jasnell jasnell requested review from joyeecheung and mcollina Jul 30, 2022
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

Adding a benchmark would be nice, but not strictly necessary.

Qard
Qard approved these changes Jul 30, 2022
Copy link
Contributor

@ShogunPanda ShogunPanda left a comment

Choose a reason for hiding this comment

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

LGTM!

@ShogunPanda
Copy link
Contributor

Adding a benchmark would be nice, but not strictly necessary.

@mcollina I think that's the first time I've read this from you. XD

@mcollina mcollina added the request-ci Add this label to start a Jenkins CI on a PR. label Jul 30, 2022
@mcollina
Copy link
Member

This has been thoroughly identified as a major fetch issue by @RafaelGSS and @devsnek in nodejs/undici#1203 (comment) and nodejs/undici#1203 (comment).

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

doc/api/util.md Show resolved Hide resolved
doc/api/util.md Show resolved Hide resolved
doc/api/util.md Outdated Show resolved Hide resolved
lib/internal/abort_controller.js Outdated Show resolved Hide resolved
@aduh95 aduh95 added the semver-minor PRs that contain new features and should be released in the next minor version. label Jul 30, 2022
@rluvaton
Copy link
Contributor

+1! Btw, so folks know, this is my son. He wanted something to help get started contributing so I recommended that he take over this change.

Family goals

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

This comment was marked as outdated.

@targos
Copy link
Member

targos commented Aug 2, 2022

There's a failing test:

=== release test-abortcontroller ===
Path: parallel/test-abortcontroller
Error: --- stderr ---
node:assert:124
  throw new AssertionError(obj);
  ^

AssertionError [ERR_ASSERTION]: Missing expected exception (TypeError).
    at Object.<anonymous> (/home/runner/work/node/node/test/parallel/test-abortcontroller.js:109:5)
    at Module._compile (node:internal/modules/cjs/loader:1120:14)
    at Module._extensions..js (node:internal/modules/cjs/loader:1174:10)
    at Module.load (node:internal/modules/cjs/loader:998:32)
    at Module._load (node:internal/modules/cjs/loader:839:12)
    at Function.executeUserEntryPoint [as runMain] (node:internal/modules/run_main:86:12)
    at node:internal/main/run_main_module:17:47 {
  generatedMessage: false,
  code: 'ERR_ASSERTION',
  actual: undefined,
  expected: { name: 'TypeError' },
  operator: 'throws'
}

Node.js v19.0.0-pre
Command: out/Release/node --no-warnings --expose-gc --expose-internals /home/runner/work/node/node/test/parallel/test-abortcontroller.js

@jasnell
Copy link
Member

jasnell commented Aug 3, 2022

@flakey5 ... after the github action checks complete here, if they all come up green, go ahead and squash all of the commits here into a single commit. After, I will run this through the regular CI. Assuming that comes up green, we should be good to go to get this landed :-)

Qard
Qard approved these changes Aug 3, 2022
@nodejs-github-bot

This comment was marked as outdated.

@danielleadams
Copy link
Member

@flakey5 do you mind creating a backport PR to this for v18.x? This broke tests on the release line. Thank you.

@danielleadams danielleadams removed the backport-requested-v18.x PRs awaiting manual backport to the v18.x-staging branch. label Aug 16, 2022
@danielleadams danielleadams added the backport-requested-v18.x PRs awaiting manual backport to the v18.x-staging branch. label Aug 16, 2022
Fyko pushed a commit to Fyko/node that referenced this pull request Sep 15, 2022
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>
@juanarbol
Copy link
Member

@flakey5 do you mind creating a backport PR to this for v18.x? This broke tests on the release line. Thank you.

Same for v16.x

@juanarbol juanarbol added the backport-requested-v16.x PRs awaiting manual backport to the v16.x-staging branch. label Oct 3, 2022
@flakey5
Copy link
Contributor Author

flakey5 commented Oct 9, 2022

@flakey5 do you mind creating a backport PR to this for v18.x? This broke tests on the release line. Thank you.

@flakey5 do you mind creating a backport PR to this for v18.x? This broke tests on the release line. Thank you.

Same for v16.x

My bad didn't see either of these. I'll start working on the v18.x backport in a sec

flakey5 added a commit to flakey5/node that referenced this pull request Oct 9, 2022
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>
@flakey5
Copy link
Contributor Author

flakey5 commented Oct 9, 2022

v18.x backport: #44941

danielleadams pushed a commit that referenced this pull request Oct 11, 2022
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 danielleadams added backported-to-v18.x PRs backported to the v18.x-staging branch. and removed backport-requested-v18.x PRs awaiting manual backport to the v18.x-staging branch. labels Oct 11, 2022
danielleadams added a commit that referenced this pull request Oct 11, 2022
Notable changes:

* cli:
  * (SEMVER-MINOR) add `--watch` (Moshe Atlow) #44366
* fs:
  * (SEMVER-MINOR) add `FileHandle.prototype.readLines` (Antoine du Hamel)
  #42590
* http:
  * (SEMVER-MINOR) add writeEarlyHints function to ServerResponse (Wing)
  #44180
* http2:
  * (SEMVER-MINOR) make early hints generic (Yagiz Nizipli) #44820
* lib:
  * (SEMVER-MINOR) refactor transferable AbortSignal (flakey5) #44048
* src:
  * (SEMVER-MINOR) add detailed embedder process initialization API (Anna
  Henningsen) #44121
* util:
  * (SEMVER-MINOR) add default value option to parsearg (Manuel Spigolon)
  #44631

PR-URL: #44968
danielleadams added a commit that referenced this pull request Oct 12, 2022
watch mode (experimental):

Running in 'watch' mode using `node --watch` restarts the process when an
imported file is changed.

Contributed by Moshe Atlow in [#44366](#44366)

Other notable changes:

* fs:
  * (SEMVER-MINOR) add `FileHandle.prototype.readLines` (Antoine du Hamel)
  #42590
* http:
  * (SEMVER-MINOR) add writeEarlyHints function to ServerResponse (Wing)
  #44180
* http2:
  * (SEMVER-MINOR) make early hints generic (Yagiz Nizipli) #44820
* lib:
  * (SEMVER-MINOR) refactor transferable AbortSignal (flakey5) #44048
* src:
  * (SEMVER-MINOR) add detailed embedder process initialization API (Anna
  Henningsen) #44121
* util:
  * (SEMVER-MINOR) add default value option to parsearg (Manuel Spigolon)
  #44631

PR-URL: #44968
danielleadams added a commit that referenced this pull request Oct 12, 2022
watch mode (experimental):

Running in 'watch' mode using `node --watch` restarts the process when an
imported file is changed.

Contributed by Moshe Atlow in [#44366](#44366)

Other notable changes:

* fs:
  * (SEMVER-MINOR) add `FileHandle.prototype.readLines` (Antoine du Hamel)
  #42590
* http:
  * (SEMVER-MINOR) add writeEarlyHints function to ServerResponse (Wing)
  #44180
* http2:
  * (SEMVER-MINOR) make early hints generic (Yagiz Nizipli) #44820
* lib:
  * (SEMVER-MINOR) refactor transferable AbortSignal (flakey5) #44048
* src:
  * (SEMVER-MINOR) add detailed embedder process initialization API (Anna
  Henningsen) #44121
* util:
  * (SEMVER-MINOR) add default value option to parsearg (Manuel Spigolon)
  #44631

PR-URL: #44968
danielleadams added a commit that referenced this pull request Oct 13, 2022
watch mode (experimental):

Running in 'watch' mode using `node --watch` restarts the process when an
imported file is changed.

Contributed by Moshe Atlow in [#44366](#44366)

Other notable changes:

* fs:
  * (SEMVER-MINOR) add `FileHandle.prototype.readLines` (Antoine du Hamel)
  #42590
* http:
  * (SEMVER-MINOR) add writeEarlyHints function to ServerResponse (Wing)
  #44180
* http2:
  * (SEMVER-MINOR) make early hints generic (Yagiz Nizipli) #44820
* lib:
  * (SEMVER-MINOR) refactor transferable AbortSignal (flakey5) #44048
* src:
  * (SEMVER-MINOR) add detailed embedder process initialization API (Anna
  Henningsen) #44121
* util:
  * (SEMVER-MINOR) add default value option to parsearg (Manuel Spigolon)
  #44631

PR-URL: #44968
danielleadams added a commit that referenced this pull request Oct 13, 2022
watch mode (experimental):

Running in 'watch' mode using `node --watch` restarts the process when an
imported file is changed.

Contributed by Moshe Atlow in [#44366](#44366)

Other notable changes:

* fs:
  * (SEMVER-MINOR) add `FileHandle.prototype.readLines` (Antoine du Hamel)
  #42590
* http:
  * (SEMVER-MINOR) add writeEarlyHints function to ServerResponse (Wing)
  #44180
* http2:
  * (SEMVER-MINOR) make early hints generic (Yagiz Nizipli) #44820
* lib:
  * (SEMVER-MINOR) refactor transferable AbortSignal (flakey5) #44048
* src:
  * (SEMVER-MINOR) add detailed embedder process initialization API (Anna
  Henningsen) #44121
* util:
  * (SEMVER-MINOR) add default value option to parsearg (Manuel Spigolon)
  #44631

PR-URL: #44968
danielleadams added a commit that referenced this pull request Oct 13, 2022
watch mode (experimental):

Running in 'watch' mode using `node --watch` restarts the process when an
imported file is changed.

Contributed by Moshe Atlow in #44366

Other notable changes:

* fs:
  * (SEMVER-MINOR) add `FileHandle.prototype.readLines` (Antoine du Hamel)
  #42590
* http:
  * (SEMVER-MINOR) add writeEarlyHints function to ServerResponse (Wing)
  #44180
* http2:
  * (SEMVER-MINOR) make early hints generic (Yagiz Nizipli) #44820
* lib:
  * (SEMVER-MINOR) refactor transferable AbortSignal (flakey5) #44048
* src:
  * (SEMVER-MINOR) add detailed embedder process initialization API (Anna
  Henningsen) #44121
* util:
  * (SEMVER-MINOR) add default value option to parsearg (Manuel Spigolon)
  #44631

PR-URL: #44968
danielleadams added a commit that referenced this pull request Oct 13, 2022
watch mode (experimental):

Running in 'watch' mode using `node --watch` restarts the process when an
imported file is changed.

Contributed by Moshe Atlow in #44366

Other notable changes:

* fs:
  * (SEMVER-MINOR) add `FileHandle.prototype.readLines` (Antoine du Hamel)
  #42590
* http:
  * (SEMVER-MINOR) add writeEarlyHints function to ServerResponse (Wing)
  #44180
* http2:
  * (SEMVER-MINOR) make early hints generic (Yagiz Nizipli) #44820
* lib:
  * (SEMVER-MINOR) refactor transferable AbortSignal (flakey5) #44048
* src:
  * (SEMVER-MINOR) add detailed embedder process initialization API (Anna
  Henningsen) #44121
* util:
  * (SEMVER-MINOR) add default value option to parsearg (Manuel Spigolon)
  #44631

PR-URL: #44968
danielleadams added a commit that referenced this pull request Oct 13, 2022
watch mode (experimental):

Running in 'watch' mode using `node --watch` restarts the process when an
imported file is changed.

Contributed by Moshe Atlow in #44366

Other notable changes:

* fs:
  * (SEMVER-MINOR) add `FileHandle.prototype.readLines` (Antoine du Hamel)
  #42590
* http:
  * (SEMVER-MINOR) add writeEarlyHints function to ServerResponse (Wing)
  #44180
* http2:
  * (SEMVER-MINOR) make early hints generic (Yagiz Nizipli) #44820
* lib:
  * (SEMVER-MINOR) refactor transferable AbortSignal (flakey5) #44048
* src:
  * (SEMVER-MINOR) add detailed embedder process initialization API (Anna
  Henningsen) #44121
* util:
  * (SEMVER-MINOR) add default value option to parsearg (Manuel Spigolon)
  #44631

PR-URL: #44968
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
author ready PRs that have at least one approval, no pending requests for changes, and a CI started. backport-requested-v16.x PRs awaiting manual backport to the v16.x-staging branch. backported-to-v18.x PRs backported to the v18.x-staging branch. commit-queue-failed An error occurred while landing this pull request using GitHub Actions. semver-minor PRs that contain new features and should be released in the next minor version. util Issues and PRs related to the built-in util module. worker Issues and PRs related to Worker support.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet