lib: refactor transferable AbortSignal by flakey5 · Pull Request #44048 · nodejs/node · GitHub

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

+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

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) {

lgtm

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

LGTM!

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

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

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

+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

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

@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 :-)

@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

@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

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>

v18.x backport: #44941

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>

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

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 mentioned this pull request Jul 30, 2022

lib: refactor transferable AbortSignal #43388

Closed

jasnell requested review from joyeecheung and mcollina July 30, 2022 03:23

joyeecheung reviewed Jul 30, 2022

View reviewed changes

Comment thread lib/internal/abort_controller.js Outdated

mcollina approved these changes Jul 30, 2022

View reviewed changes

Qard approved these changes Jul 30, 2022

View reviewed changes

ShogunPanda approved these changes Jul 30, 2022

View reviewed changes

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

github-actions Bot removed the request-ci Add this label to start a Jenkins CI on a PR. label Jul 30, 2022

aduh95 reviewed Jul 30, 2022

View reviewed changes

Comment thread doc/api/util.md Outdated

Comment thread doc/api/util.md Outdated

Comment thread doc/api/util.md Outdated

Comment thread lib/internal/abort_controller.js Outdated

aduh95 added the semver-minor PRs that contain new features and should be released in the next minor version. label Jul 30, 2022

This was referenced Aug 1, 2022

CI Reliability 2022-08-01 nodejs/reliability#334

Open

CI Reliability 2022-08-02 nodejs/reliability#335

Open

mcollina added the request-ci Add this label to start a Jenkins CI on a PR. label Aug 2, 2022

github-actions Bot removed the request-ci Add this label to start a Jenkins CI on a PR. label Aug 2, 2022

This comment was marked as outdated.

Sign in to view

github-actions Bot mentioned this pull request Aug 3, 2022

CI Reliability 2022-08-03 nodejs/reliability#336

Open

15 tasks

Qard approved these changes Aug 3, 2022

View reviewed changes

This comment was marked as outdated.

Sign in to view

This was referenced Aug 5, 2022

CI Reliability 2022-08-05 nodejs/reliability#337

Open

CI Reliability 2022-08-06 nodejs/reliability#338

Open

CI Reliability 2022-08-07 nodejs/reliability#339

Open

CI Reliability 2022-08-08 nodejs/reliability#340

Open

danielleadams added the backport-requested-v18.x label Aug 16, 2022

danielleadams removed the backport-requested-v18.x label Aug 16, 2022

mcollina mentioned this pull request Aug 16, 2022

Transferrable AbortController is very slow #43160

Closed

danielleadams added the backport-requested-v18.x label Aug 16, 2022

RafaelGSS mentioned this pull request Sep 13, 2022

2022-10-18, Version 19.0.0 (Current) #44626

Merged

juanarbol added the backport-requested-v16.x label Oct 3, 2022

flakey5 mentioned this pull request Oct 9, 2022

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

Closed

danielleadams added backported-to-v18.x and removed backport-requested-v18.x labels Oct 11, 2022

danielleadams mentioned this pull request Oct 11, 2022

v18.11.0 proposal #44968

Merged

RafaelGSS mentioned this pull request Oct 26, 2022

fetch() performance parity nodejs/undici#1203

Closed

lib: refactor transferable AbortSignal by flakey5 · Pull Request #44048 · nodejs/node · GitHub

Uh oh!

Uh oh!

lib: refactor transferable AbortSignal - #44048

lib: refactor transferable AbortSignal#44048flakey5 wants to merge 1 commit intonodejs:mainnodejs/node:mainfrom flakey5:flakey5/finish-refactor-transferable-abortsignalflakey5/node:flakey5/finish-refactor-transferable-abortsignalCopy head branch name to clipboard

Conversation

flakey5 commented Jul 30, 2022

Uh oh!

jasnell commented Jul 30, 2022

Uh oh!

jasnell commented Jul 30, 2022

Uh oh!

Uh oh!

mcollina left a comment

Choose a reason for hiding this comment

Uh oh!

ShogunPanda left a comment

Choose a reason for hiding this comment

Uh oh!

ShogunPanda commented Jul 30, 2022

Uh oh!

mcollina commented Jul 30, 2022

Uh oh!

nodejs-github-bot commented Jul 30, 2022

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

rluvaton commented Jul 30, 2022

Uh oh!

This comment was marked as outdated.

targos commented Aug 2, 2022

Uh oh!

jasnell commented Aug 3, 2022

Uh oh!

This comment was marked as outdated.

danielleadams commented Aug 16, 2022

Uh oh!

juanarbol commented Oct 3, 2022

Uh oh!

flakey5 commented Oct 9, 2022

Uh oh!

flakey5 commented Oct 9, 2022

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

14 participants

lib: refactor transferable AbortSignal#44048
flakey5 wants to merge 1 commit into
nodejs:mainfrom
flakey5:flakey5/finish-refactor-transferable-abortsignal