[v14.x] Backport AbortController and friends by targos · Pull Request #38386 · nodejs/node

targos · 2021-04-24T18:27:47Z

This is a first batch of backports for AbortController.

lib: initial experimental AbortController implementation (lib: initial experimental AbortController implementation #33527)
This only backports the first semver-minor commit.
timers: allow promisified timeouts/immediates to be canceled (timers: allow promisified timeouts/immediates to be canceled #33833)
timers: fix multipleResolves in promisified timeouts/immediates (timers: fix multipleResolves in promisified timeouts/immediates #33949)
timers: move promisified timers implementations (timers: introduce timers/promises #33950)
This introduces an internal module (internal/timers/promises) to avoid the semver-major change.
timers: use AbortController with correct name/message (quic,timers: use AbortController with correct name/message #34763)
Only the timers commit.
events: allow use of AbortController with once (events: allow use of AbortController with once #34911)
events: allow use of AbortController with on (events: allow use of AbortController with on #34912)
doc: revise AbortSignal text and example using events.once (doc: revise AbortSignal text and example using events.once() #35005)
doc: make AbortSignal text consistent in events.md (doc: revise AbortSignal text and example using events.once() #35005)
events: make abort_controller event trusted (events: make abort_controller event trusted #35811)
lib: let abort_controller target be EventTarget (lib, test: integrate wpt abort_controller tests and fix onabort event #35869)
test: integrate abort_controller tests from wpt (lib, test: integrate wpt abort_controller tests and fix onabort event #35869)
fs: add support for AbortSignal in readFile (fs: add support for AbortSignal in readFile #35911)
fs: support abortsignal in writeFile (fs: support abortsignal in writeFile #35993)
events: add a few tests (events: add a few tests #35806)
events: support emit on nodeeventtarget (events: support emit on nodeeventtarget #35851)
events: define event handler as enumerable (events: support defining event handlers on prototypes #35931)
events: support event handlers on prototypes (events: support defining event handlers on prototypes #35931)
events: define abort on prototype (events: support defining event handlers on prototypes #35931)
events: fire handlers in correct oder (events: support defining event handlers on prototypes #35931)
events: disabled manual construction AbortSignal (events: disabled manual construction AbortSignal #36094)
events: getEventListeners static (events: getEventListeners static #35991)
http: add support for abortsignal to http.request (http: add support for abortsignal to http.request #36048)
http2: add support for AbortSignal to http2Session.request (http2: add support for AbortSignal to http2Session.request #36070)
child_process: add AbortSignal support (child_process: add AbortSignal support #36308)
test: increase execFile abort coverage (test: increase readfile abort coverage #36429)
child_process: clean event listener correctly (child_process: clean event listener correctly #36424)
child_process: add signal support to spawn (child_process: add signal support to spawn #36432)
child_process: support AbortSignal in fork (child_process: support AbortSignal in fork #36603)
timers: refactor to use validateAbortSignal (lib: refactor to use validateAbortSignal #36604)
dgram: support AbortSignal in createSocket (dgram: support AbortSignal in createSocket #37026)
fs: add AbortSignal support to watch (fs: add AbortSignal support to watch #37190)
fs: fix pre-aborted writeFile AbortSignal file leak (fs: fix pre-aborted writeFile AbortSignal file leak #37393)
doc: recommend checking abortSignal.aborted first (doc: recommend checking abortSignal.aborted first #37714)
events: add max listener warning for EventTarget (events: add max listener warning for EventTarget #36001)
lib: set abort-controller toStringTag (lib: set abort-controller toStringTag #36115)
lib: implement AbortSignal.abort() (lib: implement AbortSignal.abort() #37693)
test: update dom/abort tests (lib: implement AbortSignal.abort() #37693)
lib: add brand checks to AbortController and AbortSignal (lib: add brand checks to AbortController and AbortSignal #37720)

targos · 2021-04-24T18:28:15Z

targos · 2021-04-24T18:29:15Z

I did not include commits adding abort support to streams because there were big conflicts for them and this is already a big patch set.

nodejs-github-bot · 2021-04-24T18:34:20Z

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

benjamingr

This adds two globals in a non-semver-major fashion.

I am LGTMing the actual backport - but please be very careful with the implications for users.

For example:

A user is using an abortcontroller polyfill that implements the interface in a non whatwg spec compliant way.
We backport AbortController as a global
The polyfill now sees the global so it doesn't polyfill
The user relying on the non-spec-compliant behaviour (like AbortSignal being an EventEmitter) has code breakage because of a minor version Node.js update

I am not sure how common or realistic the above is.

targos · 2021-04-25T08:11:22Z

@benjamingr This doesn't backport the global by default (it stays behind the --experimental-abortcontroller flag), so anyone using a polyfill should see no difference in behaviour, and I think they should be able to use our abort-compatible APIs with it?

benjamingr · 2021-04-25T08:13:01Z

Ok, I thought that was "coming", if this stays behind a flag and is only global in our eslint config etc then LGTM

danielleadams · 2021-04-26T00:45:19Z

@targos do you want more reviewers given the number of commits, or should I go ahead and backport this?

nodejs-github-bot · 2021-04-26T07:20:38Z

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

targos · 2021-04-26T07:20:56Z

I'd love to have some more eyes on this as it's not a trivial backport. Let's keep it open 24 more hours.

nodejs-github-bot · 2021-04-26T07:34:29Z

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

nodejs-github-bot · 2021-04-26T12:16:11Z

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

benjamingr · 2021-04-26T13:09:44Z

Fwiw I actually went through the changes here and they look fine - not a rubberstamp lgtm :)

nodejs-github-bot · 2021-04-27T07:50:48Z

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

targos · 2021-04-27T13:17:16Z

I'm starting to think that this somehow breaks test-fs-write-file on Raspberry Pi... That same test failed on every CI run for this PR.

Linkgoron · 2021-04-29T21:19:32Z

@targos that test is broken, the last test in test-fs-write-file is writing to text4.txt, and also the one that's failing is also writing to the same file (see lines 59 and 91).

It was fixed in master here: #36102

AbortController impl based very closely on: https://github.com/mysticatea/abort-controller Marked experimental. Not currently used by any of the existing promise apis. Signed-off-by: James M Snell <[email protected]> PR-URL: nodejs#33527 Reviewed-By: Benjamin Gruenbaum <[email protected]> Reviewed-By: Matteo Collina <[email protected]> Reviewed-By: Anna Henningsen <[email protected]>

Using the new experimental AbortController... Signed-off-by: James M Snell <[email protected]> PR-URL: nodejs#33833 Reviewed-By: Anna Henningsen <[email protected]> Reviewed-By: Matteo Collina <[email protected]> Reviewed-By: Gus Caplan <[email protected]>

After successful timer finish the abort event callback would still reject (already resolved promise) upon calling abortController.abort(). Signed-off-by: Denys Otrishko <[email protected]> PR-URL: nodejs#33949 Reviewed-By: James M Snell <[email protected]> Reviewed-By: Benjamin Gruenbaum <[email protected]> Reviewed-By: Anna Henningsen <[email protected]>

Move the promisified timers implementations into a new internal. submodule. Also adds `ref` option to the promisified versions. Signed-off-by: James M Snell <[email protected]> PR-URL: nodejs#33950 Reviewed-By: Denys Otrishko <[email protected]> Reviewed-By: Benjamin Gruenbaum <[email protected]>

On the web, `AbortError` is the error name, not the error message. Change the code to match that. PR-URL: nodejs#34763 Reviewed-By: James M Snell <[email protected]> Reviewed-By: Rich Trott <[email protected]>

Allows an AbortSignal to be passed in to events.once() to cancel waiting on an event. Signed-off-by: James M Snell <[email protected]> PR-URL: nodejs#34911 Reviewed-By: Denys Otrishko <[email protected]> Reviewed-By: Benjamin Gruenbaum <[email protected]> Reviewed-By: Matteo Collina <[email protected]>

Signed-off-by: James M Snell <[email protected]> PR-URL: nodejs#34912 Reviewed-By: Matteo Collina <[email protected]> Reviewed-By: Benjamin Gruenbaum <[email protected]> Reviewed-By: Denys Otrishko <[email protected]>

Add a line to the example code to clarify what happens if an event is emitted after listening is canceled. Make minor revisions to surrounding text. PR-URL: nodejs#35005 Reviewed-By: Michaël Zasso <[email protected]> Reviewed-By: Benjamin Gruenbaum <[email protected]> Reviewed-By: Anna Henningsen <[email protected]>

The AbortController abort event should have EventTarget as a target property of the argument event. PR-URL: #35869 Backport-PR-URL: #38386 Refs: https://github.com/web-platform-tests/wpt/blob/master/dom/abort/event.any.js Reviewed-By: Rich Trott <[email protected]> Reviewed-By: Michaël Zasso <[email protected]> Reviewed-By: Benjamin Gruenbaum <[email protected]>

Refs: web-platform-tests/wpt#9361 PR-URL: #35869 Backport-PR-URL: #38386 Refs: https://github.com/web-platform-tests/wpt/blob/master/dom/abort/event.any.js Reviewed-By: Rich Trott <[email protected]> Reviewed-By: Michaël Zasso <[email protected]> Reviewed-By: Benjamin Gruenbaum <[email protected]>

PR-URL: #35911 Backport-PR-URL: #38386 Reviewed-By: James M Snell <[email protected]> Reviewed-By: Anna Henningsen <[email protected]> Reviewed-By: Matteo Collina <[email protected]> Reviewed-By: Rich Trott <[email protected]> Reviewed-By: Michaël Zasso <[email protected]>

PR-URL: #35993 Backport-PR-URL: #38386 Reviewed-By: Antoine du Hamel <[email protected]> Reviewed-By: Matteo Collina <[email protected]>

PR-URL: #35806 Backport-PR-URL: #38386 Reviewed-By: Rich Trott <[email protected]> Reviewed-By: James M Snell <[email protected]> Reviewed-By: Antoine du Hamel <[email protected]>

PR-URL: #35851 Backport-PR-URL: #38386 Reviewed-By: Anna Henningsen <[email protected]> Reviewed-By: Rich Trott <[email protected]> Reviewed-By: Yongsheng Zhang <[email protected]> Reviewed-By: Ricky Zhou <[email protected]>

PR-URL: #35931 Backport-PR-URL: #38386 Reviewed-By: Anna Henningsen <[email protected]> Reviewed-By: Antoine du Hamel <[email protected]> Reviewed-By: Daijiro Wachi <[email protected]> Reviewed-By: Rich Trott <[email protected]>

Fixes: #36064 PR-URL: #36094 Backport-PR-URL: #38386 Reviewed-By: James M Snell <[email protected]> Reviewed-By: Benjamin Gruenbaum <[email protected]> Reviewed-By: Luigi Pinca <[email protected]> Reviewed-By: Zeyu Yang <[email protected]> Reviewed-By: Andrey Pechkurov <[email protected]> Reviewed-By: Rich Trott <[email protected]>

PR-URL: #35991 Backport-PR-URL: #38386 Reviewed-By: James M Snell <[email protected]> Reviewed-By: Matteo Collina <[email protected]>

PR-URL: #36048 Backport-PR-URL: #38386 Reviewed-By: Anna Henningsen <[email protected]> Reviewed-By: Denys Otrishko <[email protected]> Reviewed-By: Ricky Zhou <[email protected]> Reviewed-By: Rich Trott <[email protected]>

- Add support - Add test - Docs once PR is up PR-URL: #36070 Backport-PR-URL: #38386 Reviewed-By: Matteo Collina <[email protected]> Reviewed-By: Benjamin Gruenbaum <[email protected]>

PR-URL: #36308 Backport-PR-URL: #38386 Reviewed-By: Rich Trott <[email protected]>

Verify that if something different than Abortcontroller.signal is passed to child_process.execFile(), ERR_INVALID_ARG_TYPE is thrown. PR-URL: #36429 Backport-PR-URL: #38386 Reviewed-By: Benjamin Gruenbaum <[email protected]> Reviewed-By: Rich Trott <[email protected]>

I was working on AbortSignal for spawn and noticed there is a leak in the current code for AbortSignal support in child_process since it removes the wrong listener. I used the new signal as argument feature to make removing the listener easier and added a test. PR-URL: #36424 Backport-PR-URL: #38386 Reviewed-By: Anna Henningsen <[email protected]> Reviewed-By: Rich Trott <[email protected]>

PR-URL: #36432 Backport-PR-URL: #38386 Reviewed-By: James M Snell <[email protected]> Reviewed-By: Rich Trott <[email protected]>

PR-URL: #36603 Backport-PR-URL: #38386 Reviewed-By: Rich Trott <[email protected]> Reviewed-By: James M Snell <[email protected]> Reviewed-By: Anna Henningsen <[email protected]> Reviewed-By: Luigi Pinca <[email protected]>

PR-URL: #36604 Backport-PR-URL: #38386 Reviewed-By: Benjamin Gruenbaum <[email protected]> Reviewed-By: Rich Trott <[email protected]> Reviewed-By: Juan José Arboleda <[email protected]> Reviewed-By: Richard Lau <[email protected]>

PR-URL: #37026 Backport-PR-URL: #38386 Reviewed-By: Benjamin Gruenbaum <[email protected]> Reviewed-By: Rich Trott <[email protected]>

PR-URL: #37190 Backport-PR-URL: #38386 Refs: #37179 Reviewed-By: James M Snell <[email protected]> Reviewed-By: Antoine du Hamel <[email protected]> Reviewed-By: Darshan Sen <[email protected]>

Fix an issue in writeFile where a file is opened, and not closed if the abort signal is aborted after the file was opened but before writing began. PR-URL: #37393 Backport-PR-URL: #38386 Reviewed-By: Benjamin Gruenbaum <[email protected]> Reviewed-By: Antoine du Hamel <[email protected]>

Signed-off-by: James M Snell <[email protected]> PR-URL: #37714 Backport-PR-URL: #38386 Reviewed-By: Antoine du Hamel <[email protected]> Reviewed-By: Trivikram Kamat <[email protected]> Reviewed-By: Darshan Sen <[email protected]> Reviewed-By: Rich Trott <[email protected]>

Signed-off-by: James M Snell <[email protected]> PR-URL: #36001 Backport-PR-URL: #38386 Fixes: #35990 Reviewed-By: Benjamin Gruenbaum <[email protected]> Reviewed-By: Rich Trott <[email protected]>

PR-URL: #36115 Backport-PR-URL: #38386 Reviewed-By: Rich Trott <[email protected]> Reviewed-By: Antoine du Hamel <[email protected]> Reviewed-By: Anna Henningsen <[email protected]> Reviewed-By: James M Snell <[email protected]>

Refs: whatwg/dom#960 PR-URL: #37693 Backport-PR-URL: #38386 Reviewed-By: Antoine du Hamel <[email protected]> Reviewed-By: Michaël Zasso <[email protected]> Reviewed-By: Benjamin Gruenbaum <[email protected]>

Signed-off-by: James M Snell <[email protected]> PR-URL: #37693 Backport-PR-URL: #38386 Refs: whatwg/dom#960 Reviewed-By: Antoine du Hamel <[email protected]> Reviewed-By: Michaël Zasso <[email protected]> Reviewed-By: Benjamin Gruenbaum <[email protected]>

PR-URL: #37720 Backport-PR-URL: #38386 Reviewed-By: James M Snell <[email protected]> Reviewed-By: Antoine du Hamel <[email protected]>

The test uses a file name twice, causing unreliability in CI. In particular, it's failing a lot on the Raspberry Pi devices. Fixes: #36090 PR-URL: #36102 Backport-PR-URL: #38386 Reviewed-By: Luigi Pinca <[email protected]> Reviewed-By: Richard Lau <[email protected]>

nodejs-github-bot added lib / src Issues and PRs related to general changes in the lib or src directory. needs-ci PRs that need a full CI run. v14.x labels Apr 24, 2021

This comment has been minimized.

Sign in to view

richardlau added the semver-minor PRs that contain new features and should be released in the next minor version. label Apr 24, 2021

benjamingr approved these changes Apr 25, 2021

View reviewed changes

danielleadams approved these changes Apr 26, 2021

View reviewed changes

targos force-pushed the backport-abort-controller-v14.x branch from d0c99b1 to a4045c8 Compare April 26, 2021 07:33

jasnell and others added 8 commits April 30, 2021 08:59

timers: use AbortController with correct name/message

26b7626

On the web, `AbortError` is the error name, not the error message. Change the code to match that. PR-URL: nodejs#34763 Reviewed-By: James M Snell <[email protected]> Reviewed-By: Rich Trott <[email protected]>

events: allow use of AbortController with on

51031f3

Signed-off-by: James M Snell <[email protected]> PR-URL: nodejs#34912 Reviewed-By: Matteo Collina <[email protected]> Reviewed-By: Benjamin Gruenbaum <[email protected]> Reviewed-By: Denys Otrishko <[email protected]>

targos pushed a commit that referenced this pull request Apr 30, 2021

fs: support abortsignal in writeFile

219cd00

PR-URL: #35993 Backport-PR-URL: #38386 Reviewed-By: Antoine du Hamel <[email protected]> Reviewed-By: Matteo Collina <[email protected]>

targos pushed a commit that referenced this pull request Apr 30, 2021

events: add a few tests

6558ffa

PR-URL: #35806 Backport-PR-URL: #38386 Reviewed-By: Rich Trott <[email protected]> Reviewed-By: James M Snell <[email protected]> Reviewed-By: Antoine du Hamel <[email protected]>

targos pushed a commit that referenced this pull request Apr 30, 2021

events: getEventListeners static

6e734a8

PR-URL: #35991 Backport-PR-URL: #38386 Reviewed-By: James M Snell <[email protected]> Reviewed-By: Matteo Collina <[email protected]>

targos pushed a commit that referenced this pull request Apr 30, 2021

child_process: add AbortSignal support

a5c0f39

PR-URL: #36308 Backport-PR-URL: #38386 Reviewed-By: Rich Trott <[email protected]>

targos pushed a commit that referenced this pull request Apr 30, 2021

child_process: add signal support to spawn

0b691ce

PR-URL: #36432 Backport-PR-URL: #38386 Reviewed-By: James M Snell <[email protected]> Reviewed-By: Rich Trott <[email protected]>

targos pushed a commit that referenced this pull request Apr 30, 2021

dgram: support AbortSignal in createSocket

81cd06b

PR-URL: #37026 Backport-PR-URL: #38386 Reviewed-By: Benjamin Gruenbaum <[email protected]> Reviewed-By: Rich Trott <[email protected]>

targos pushed a commit that referenced this pull request Apr 30, 2021

lib: add brand checks to AbortController and AbortSignal

b1507c4

PR-URL: #37720 Backport-PR-URL: #38386 Reviewed-By: James M Snell <[email protected]> Reviewed-By: Antoine du Hamel <[email protected]>

Uh oh!

Conversation

targos commented Apr 24, 2021

Uh oh!

targos commented Apr 24, 2021

Uh oh!

targos commented Apr 24, 2021

Uh oh!

This comment has been minimized.

nodejs-github-bot commented Apr 24, 2021

Uh oh!

benjamingr left a comment

Choose a reason for hiding this comment

Uh oh!

targos commented Apr 25, 2021

Uh oh!

benjamingr commented Apr 25, 2021

Uh oh!

danielleadams commented Apr 26, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

nodejs-github-bot commented Apr 26, 2021

Uh oh!

targos commented Apr 26, 2021

Uh oh!

nodejs-github-bot commented Apr 26, 2021

Uh oh!

nodejs-github-bot commented Apr 26, 2021

Uh oh!

benjamingr commented Apr 26, 2021

Uh oh!

nodejs-github-bot commented Apr 27, 2021

Uh oh!

targos commented Apr 27, 2021

Uh oh!

Linkgoron commented Apr 29, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

16 participants

danielleadams commented Apr 26, 2021 •

edited

Loading

Linkgoron commented Apr 29, 2021 •

edited

Loading