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

[v14.x] Backport AbortController and friends #38386

Closed
wants to merge 40 commits into from
Closed

[v14.x] Backport AbortController and friends #38386

wants to merge 40 commits into from

Conversation

targos
Copy link
Member

@targos targos commented Apr 24, 2021

This is a first batch of backports for AbortController.

@nodejs-github-bot 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
@targos
Copy link
Member Author

targos commented Apr 24, 2021

/cc @jasnell @benjamingr

@targos
Copy link
Member Author

targos commented Apr 24, 2021

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

This comment has been minimized.

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

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

@richardlau richardlau added the semver-minor PRs that contain new features and should be released in the next minor version. label Apr 24, 2021
benjamingr
benjamingr approved these changes Apr 25, 2021
View reviewed changes
Copy link
Member

@benjamingr benjamingr left a comment

Choose a reason for hiding this comment

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

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
Copy link
Member Author

targos commented Apr 25, 2021

@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
Copy link
Member

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

@danielleadams
Copy link
Member

danielleadams commented Apr 26, 2021 •
edited

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

@nodejs-github-bot
Copy link
Collaborator

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

@targos
Copy link
Member Author

targos commented Apr 26, 2021

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.

@targos targos force-pushed the backport-abort-controller-v14.x branch from d0c99b1 to a4045c8 Compare April 26, 2021 07:33
@nodejs-github-bot
Copy link
Collaborator

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

@nodejs-github-bot
Copy link
Collaborator

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

@benjamingr
Copy link
Member

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

@nodejs-github-bot
Copy link
Collaborator

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

@targos
Copy link
Member Author

targos commented Apr 27, 2021

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
Copy link
Member

Linkgoron commented Apr 29, 2021 •
edited

@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

jasnell and others added 8 commits April 30, 2021 08:59
@jasnell @targos
lib: initial experimental AbortController implementation
1cff8fd 
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 <jasnell@gmail.com>

PR-URL: nodejs#33527
Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com>
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
Reviewed-By: Anna Henningsen <anna@addaleax.net>
@jasnell @targos
timers: allow promisified timeouts/immediates to be canceled
c86e131 
Using the new experimental AbortController...

Signed-off-by: James M Snell <jasnell@gmail.com>

PR-URL: nodejs#33833
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
Reviewed-By: Gus Caplan <me@gus.host>
@lundibundi @targos
timers: fix multipleResolves in promisified timeouts/immediates
290ce9f 
After successful timer finish the abort event callback would still
reject (already resolved promise) upon calling abortController.abort().

Signed-off-by: Denys Otrishko <shishugi@gmail.com>

PR-URL: nodejs#33949
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com>
Reviewed-By: Anna Henningsen <anna@addaleax.net>
@jasnell @targos
timers: move promisified timers implementations
d097171 
Move the promisified timers implementations into a new internal.
submodule.

Also adds `ref` option to the promisified versions.

Signed-off-by: James M Snell <jasnell@gmail.com>

PR-URL: nodejs#33950
Reviewed-By: Denys Otrishko <shishugi@gmail.com>
Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com>
@addaleax @targos
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 <jasnell@gmail.com>
Reviewed-By: Rich Trott <rtrott@gmail.com>
@jasnell @targos
events: allow use of AbortController with once
268686a 
Allows an AbortSignal to be passed in to events.once() to cancel
waiting on an event.

Signed-off-by: James M Snell <jasnell@gmail.com>

PR-URL: nodejs#34911
Reviewed-By: Denys Otrishko <shishugi@gmail.com>
Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com>
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
@jasnell @targos
events: allow use of AbortController with on
51031f3 
Signed-off-by: James M Snell <jasnell@gmail.com>

PR-URL: nodejs#34912
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com>
Reviewed-By: Denys Otrishko <shishugi@gmail.com>
@Trott @targos
doc: revise AbortSignal text and example using events.once()
affb52d 
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 <targos@protonmail.com>
Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com>
Reviewed-By: Anna Henningsen <anna@addaleax.net>
targos pushed a commit that referenced this pull request Apr 30, 2021
@watilde @targos
lib: let abort_controller target be EventTarget
b2f8e8d 
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 <rtrott@gmail.com>
Reviewed-By: Michaël Zasso <targos@protonmail.com>
Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com>
targos pushed a commit that referenced this pull request Apr 30, 2021
@watilde @targos
test: integrate abort_controller tests from wpt
88f4261 
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 <rtrott@gmail.com>
Reviewed-By: Michaël Zasso <targos@protonmail.com>
Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com>
targos pushed a commit that referenced this pull request Apr 30, 2021
@benjamingr @targos
fs: add support for AbortSignal in readFile
5f88b64 
PR-URL: #35911
Backport-PR-URL: #38386
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
Reviewed-By: Rich Trott <rtrott@gmail.com>
Reviewed-By: Michaël Zasso <targos@protonmail.com>
targos pushed a commit that referenced this pull request Apr 30, 2021
@benjamingr @targos
fs: support abortsignal in writeFile
219cd00 
PR-URL: #35993
Backport-PR-URL: #38386
Reviewed-By: Antoine du Hamel <duhamelantoine1995@gmail.com>
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
targos pushed a commit that referenced this pull request Apr 30, 2021
@benjamingr @targos
events: add a few tests
6558ffa 
PR-URL: #35806
Backport-PR-URL: #38386
Reviewed-By: Rich Trott <rtrott@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Antoine du Hamel <duhamelantoine1995@gmail.com>
targos pushed a commit that referenced this pull request Apr 30, 2021
@benjamingr @targos
events: support emit on nodeeventtarget
e51d7c5 
PR-URL: #35851
Backport-PR-URL: #38386
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Rich Trott <rtrott@gmail.com>
Reviewed-By: Yongsheng Zhang <zyszys98@gmail.com>
Reviewed-By: Ricky Zhou <0x19951125@gmail.com>
targos pushed a commit that referenced this pull request Apr 30, 2021
@benjamingr @targos
events: define event handler as enumerable
6e21e82 
PR-URL: #35931
Backport-PR-URL: #38386
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Antoine du Hamel <duhamelantoine1995@gmail.com>
Reviewed-By: Daijiro Wachi <daijiro.wachi@gmail.com>
Reviewed-By: Rich Trott <rtrott@gmail.com>
targos pushed a commit that referenced this pull request Apr 30, 2021
@benjamingr @targos
events: support event handlers on prototypes
bdad1bc 
PR-URL: #35931
Backport-PR-URL: #38386
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Antoine du Hamel <duhamelantoine1995@gmail.com>
Reviewed-By: Daijiro Wachi <daijiro.wachi@gmail.com>
Reviewed-By: Rich Trott <rtrott@gmail.com>
targos pushed a commit that referenced this pull request Apr 30, 2021
@benjamingr @targos
events: define abort on prototype
7c95cfc 
PR-URL: #35931
Backport-PR-URL: #38386
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Antoine du Hamel <duhamelantoine1995@gmail.com>
Reviewed-By: Daijiro Wachi <daijiro.wachi@gmail.com>
Reviewed-By: Rich Trott <rtrott@gmail.com>
targos pushed a commit that referenced this pull request Apr 30, 2021
@benjamingr @targos
events: fire handlers in correct oder
daad521 
PR-URL: #35931
Backport-PR-URL: #38386
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Antoine du Hamel <duhamelantoine1995@gmail.com>
Reviewed-By: Daijiro Wachi <daijiro.wachi@gmail.com>
Reviewed-By: Rich Trott <rtrott@gmail.com>
targos pushed a commit that referenced this pull request Apr 30, 2021
@RaisinTen @targos
events: disabled manual construction AbortSignal
64cd54b 
Fixes: #36064

PR-URL: #36094
Backport-PR-URL: #38386
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: Zeyu Yang <himself65@outlook.com>
Reviewed-By: Andrey Pechkurov <apechkurov@gmail.com>
Reviewed-By: Rich Trott <rtrott@gmail.com>
targos pushed a commit that referenced this pull request Apr 30, 2021
@benjamingr @targos
events: getEventListeners static
6e734a8 
PR-URL: #35991
Backport-PR-URL: #38386
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
targos pushed a commit that referenced this pull request Apr 30, 2021
@benjamingr @targos
http: add support for abortsignal to http.request
524b713 
PR-URL: #36048
Backport-PR-URL: #38386
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Denys Otrishko <shishugi@gmail.com>
Reviewed-By: Ricky Zhou <0x19951125@gmail.com>
Reviewed-By: Rich Trott <rtrott@gmail.com>
targos pushed a commit that referenced this pull request Apr 30, 2021
@MadaraUchiha @targos
http2: add support for AbortSignal to http2Session.request
336fb18 
- Add support
- Add test
- Docs once PR is up

PR-URL: #36070
Backport-PR-URL: #38386
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com>
targos pushed a commit that referenced this pull request Apr 30, 2021
@benjamingr @targos
child_process: add AbortSignal support
a5c0f39 
PR-URL: #36308
Backport-PR-URL: #38386
Reviewed-By: Rich Trott <rtrott@gmail.com>
targos pushed a commit that referenced this pull request Apr 30, 2021
@shootermv @targos
test: increase execFile abort coverage
dc63ca6 
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 <benjamingr@gmail.com>
Reviewed-By: Rich Trott <rtrott@gmail.com>
targos pushed a commit that referenced this pull request Apr 30, 2021
@benjamingr @targos
child_process: clean event listener correctly
6c08c9d 
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 <anna@addaleax.net>
Reviewed-By: Rich Trott <rtrott@gmail.com>
targos pushed a commit that referenced this pull request Apr 30, 2021
@benjamingr @targos
child_process: add signal support to spawn
0b691ce 
PR-URL: #36432
Backport-PR-URL: #38386
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Rich Trott <rtrott@gmail.com>
targos pushed a commit that referenced this pull request Apr 30, 2021
@benjamingr @targos
child_process: support AbortSignal in fork
385e8e8 
PR-URL: #36603
Backport-PR-URL: #38386
Reviewed-By: Rich Trott <rtrott@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
targos pushed a commit that referenced this pull request Apr 30, 2021
@Lxxyx @targos
timers: refactor to use validateAbortSignal
1b74a08 
PR-URL: #36604
Backport-PR-URL: #38386
Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com>
Reviewed-By: Rich Trott <rtrott@gmail.com>
Reviewed-By: Juan José Arboleda <soyjuanarbol@gmail.com>
Reviewed-By: Richard Lau <rlau@redhat.com>
targos pushed a commit that referenced this pull request Apr 30, 2021
@targos
dgram: support AbortSignal in createSocket
81cd06b 
PR-URL: #37026
Backport-PR-URL: #38386
Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com>
Reviewed-By: Rich Trott <rtrott@gmail.com>
targos pushed a commit that referenced this pull request Apr 30, 2021
@benjamingr @targos
fs: add AbortSignal support to watch
7aa2e5d 
PR-URL: #37190
Backport-PR-URL: #38386
Refs: #37179
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Antoine du Hamel <duhamelantoine1995@gmail.com>
Reviewed-By: Darshan Sen <raisinten@gmail.com>
targos pushed a commit that referenced this pull request Apr 30, 2021
@targos
fs: fix pre-aborted writeFile AbortSignal file leak
0269413 
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 <benjamingr@gmail.com>
Reviewed-By: Antoine du Hamel <duhamelantoine1995@gmail.com>
targos pushed a commit that referenced this pull request Apr 30, 2021
@jasnell @targos
doc: recommend checking abortSignal.aborted first
cf4fa79 
Signed-off-by: James M Snell <jasnell@gmail.com>

PR-URL: #37714
Backport-PR-URL: #38386
Reviewed-By: Antoine du Hamel <duhamelantoine1995@gmail.com>
Reviewed-By: Trivikram Kamat <trivikr.dev@gmail.com>
Reviewed-By: Darshan Sen <raisinten@gmail.com>
Reviewed-By: Rich Trott <rtrott@gmail.com>
targos pushed a commit that referenced this pull request Apr 30, 2021
@jasnell @targos
events: add max listener warning for EventTarget
3006302 
Signed-off-by: James M Snell <jasnell@gmail.com>

PR-URL: #36001
Backport-PR-URL: #38386
Fixes: #35990
Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com>
Reviewed-By: Rich Trott <rtrott@gmail.com>
targos pushed a commit that referenced this pull request Apr 30, 2021
@benjamingr @targos
lib: set abort-controller toStringTag
4cd9f39 
PR-URL: #36115
Backport-PR-URL: #38386
Reviewed-By: Rich Trott <rtrott@gmail.com>
Reviewed-By: Antoine du Hamel <duhamelantoine1995@gmail.com>
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: James M Snell <jasnell@gmail.com>
targos pushed a commit that referenced this pull request Apr 30, 2021
@jasnell @targos
lib: implement AbortSignal.abort()
448a6a2 
Refs: whatwg/dom#960

PR-URL: #37693
Backport-PR-URL: #38386
Reviewed-By: Antoine du Hamel <duhamelantoine1995@gmail.com>
Reviewed-By: Michaël Zasso <targos@protonmail.com>
Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com>
targos pushed a commit that referenced this pull request Apr 30, 2021
@jasnell @targos
test: update dom/abort tests
397d937 
Signed-off-by: James M Snell <jasnell@gmail.com>

PR-URL: #37693
Backport-PR-URL: #38386
Refs: whatwg/dom#960
Reviewed-By: Antoine du Hamel <duhamelantoine1995@gmail.com>
Reviewed-By: Michaël Zasso <targos@protonmail.com>
Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com>
targos pushed a commit that referenced this pull request Apr 30, 2021
@MattiasBuelens @targos
lib: add brand checks to AbortController and AbortSignal
b1507c4 
PR-URL: #37720
Backport-PR-URL: #38386
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Antoine du Hamel <duhamelantoine1995@gmail.com>
targos pushed a commit that referenced this pull request Apr 30, 2021
@Trott @targos
test: fix unreliable test-fs-write-file.js
de654bf 
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 <luigipinca@gmail.com>
Reviewed-By: Richard Lau <rlau@redhat.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@benjamingr benjamingr benjamingr approved these changes

@danielleadams danielleadams danielleadams approved these changes

Assignees
No one assigned
Labels
lib / src Issues and PRs related to general changes in the lib or src directory. 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.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet