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

[v14.x] Backport AbortController and friends #38386

Closed

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.

@nodejs-github-bot
Copy link
Collaborator

@richardlau richardlau added the semver-minor PRs that contain new features and should be released in the next minor version. label Apr 24, 2021
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
Contributor

danielleadams commented Apr 26, 2021

@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

@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

@nodejs-github-bot
Copy link
Collaborator

@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

@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

@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
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>
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>
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>
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>
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>
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>
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>
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
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
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
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
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
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
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
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
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
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
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
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
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
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
- 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
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
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
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
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
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
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
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
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
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
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
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
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
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
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
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
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
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
Development

Successfully merging this pull request may close these issues.