fs: add support for AbortSignal in readFile #35911

benjamingr · 2020-11-01T16:25:00Z

Add basic support for AbortSignal in readFile.

This adds support just to readFile and fs.promises.readFile and not anything else in FS (unless you include fileHandle.readFile).

This is because it's significantly easier to do for readFile since the cancellation doesn't have to happen in libuv land and since readFile is a really heavy and pretty common action.

cc @jasnell @mcollina @addaleax

mscdex · 2020-11-01T16:53:03Z

It's probably worth noting what happens when a readFile() is aborted (an error is raised in one form or another) and the limitations of aborting a readFile() call (e.g. it does not abort queued requests at the libuv level, so it will always wait for the current request to finish first).

benjamingr · 2020-11-01T16:58:05Z

@mscdex good suggestion, is this what you had in mind? d6100b1

mcollina · 2020-11-02T07:10:10Z

lib/internal/fs/promises.js

+  if (DOMException === undefined)
+    DOMException = internalBinding('messaging').DOMException;
+  return new DOMException(message, name);
+});


Why using a DOMException? As a user I would prefer to receive one of our error objects with a code.

This is recommended by the spec: https://dom.spec.whatwg.org/#aborting-ongoing-activities

Users are supposed to check for it with error.name === 'AbortError'

We are telling our users to check for an error code. I think that should be the way to go here as well.

Note that I'd like the error code to be specific for fs.readFile, so it's easy to debug.

@mcollina this is what we use everywhere else that uses AbortSignals (like events.once and timers/promises) and everywhere in the web standard (like fetch). I feel strongly that this should be compatible with how exceptions work everywhere else.

A code is already set on DOMExceptions (as ABORT_ERR ) see internal/per_context/domexception - we can recommend the code as the way to check rather than the name in the docs if you think that's better.

I've likely missed it everywhere else. This seems really hard to debug: users will not know what this aborterror is about in a complex application.

Let's not hold this for it.. I'll open an issue when I have the time.

I've likely missed it everywhere else. This seems really hard to debug: users will not know what this aborterror is about in a complex application.

Why? Would it help if this had a different .code? I am happy to set the code to something different here and reject with a subclass if you think that improves DX. I can also do that for timers (though like you said probably better in a different PX).

Note I'm not convinced DX is actually better with "different aborts cancel with different codes since by far the most common the use case I saw is checking for cancellation (in general) to ignore the error (since cancellation is not an error most times). Users can also figure where cancellation came from from the stack trace.

Let's not hold this for it.. I'll open an issue when I have the time.

Sure, note you are currently blocking - is that intentional?

mcollina · 2020-11-02T09:24:58Z

(Sorry I made a mess from the mobile interface).

mcollina

lgtm

nodejs-github-bot · 2020-11-04T10:28:20Z

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

nodejs-github-bot · 2020-11-05T15:18:51Z

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

nodejs-github-bot · 2020-11-05T18:03:50Z

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

github-actions · 2020-11-05T19:16:12Z

Commit Queue failed

- Loading data for nodejs/node/pull/35911
✔  Done loading data for nodejs/node/pull/35911
----------------------------------- PR info ------------------------------------
Title      fs: add support for AbortSignal in readFile (#35911)
   ⚠  Could not retrieve the email or name of the PR author's from user's GitHub profile!
Branch     benjamingr:readfile-abort-signal -> nodejs:master
Labels     fs, semver-minor
Commits    1
 - fs: add support for AbortSignal in readFile
Committers 1
 - Benjamin Gruenbaum 
PR-URL: https://github.com/nodejs/node/pull/35911
Reviewed-By: James M Snell 
Reviewed-By: Anna Henningsen 
Reviewed-By: Matteo Collina 
Reviewed-By: Rich Trott 
Reviewed-By: Michaël Zasso 
------------------------------ Generated metadata ------------------------------
PR-URL: https://github.com/nodejs/node/pull/35911
Reviewed-By: James M Snell 
Reviewed-By: Anna Henningsen 
Reviewed-By: Matteo Collina 
Reviewed-By: Rich Trott 
Reviewed-By: Michaël Zasso 
--------------------------------------------------------------------------------
   ⚠  Commits were pushed since the last review:
   ⚠  - fs: add support for AbortSignal in readFile
   ✖  Last GitHub CI failed
   ℹ  Last Full PR CI on 2020-11-05T18:03:50Z: https://ci.nodejs.org/job/node-test-pull-request/34099/
- Querying data for job/node-test-pull-request/34099/
✔  Build data downloaded
   ✔  Last Jenkins CI successful
   ℹ  This PR was created on Sun, 01 Nov 2020 16:25:00 GMT
   ✔  Approvals: 5
   ✔  - James M Snell (@jasnell) (TSC): https://github.com/nodejs/node/pull/35911#pullrequestreview-521239795
   ✔  - Anna Henningsen (@addaleax): https://github.com/nodejs/node/pull/35911#pullrequestreview-521251093
   ✔  - Matteo Collina (@mcollina) (TSC): https://github.com/nodejs/node/pull/35911#pullrequestreview-521540648
   ✔  - Rich Trott (@Trott) (TSC): https://github.com/nodejs/node/pull/35911#pullrequestreview-521671149
   ✔  - Michaël Zasso (@targos) (TSC): https://github.com/nodejs/node/pull/35911#pullrequestreview-521863433
--------------------------------------------------------------------------------
   ✔  Aborted `git node land` session in /home/runner/work/node/node/.ncu

Commit Queue action: https://github.com/nodejs/node/actions/runs/348132668

PR-URL: #35911 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>

benjamingr · 2020-11-06T09:07:11Z

Landed manually in b5a136c

PR-URL: #35911 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>

Notable changes: * events: getEventListeners static (Benjamin Gruenbaum) (#35991) * fs: add support for AbortSignal in readFile (Benjamin Gruenbaum) (#35911) PR URL: #36055

Notable changes: * events: * getEventListeners static (Benjamin Gruenbaum) (#35991) * fs: * support abortsignal in writeFile (Benjamin Gruenbaum) (#35993) * add support for AbortSignal in readFile (Benjamin Gruenbaum) (#35911) * stream: * fix thrown object reference (Gil Pedersen) (#36065) PR URL: #36055

PR-URL: nodejs#35911 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>

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>

Refs: nodejs#33716 Refs: nodejs#35993 Refs: nodejs#35911

Refs: #33716 Refs: #35993 Refs: #35911 PR-URL: #39972 Reviewed-By: Michaël Zasso <targos@protonmail.com> Reviewed-By: Luigi Pinca <luigipinca@gmail.com> Reviewed-By: Juan José Arboleda <soyjuanarbol@gmail.com> Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Darshan Sen <raisinten@gmail.com> Reviewed-By: Tobias Nießen <tniessen@tnie.de>

PR-URL: #39972 Refs: #33716 Refs: #35993 Refs: #35911 Reviewed-By: Michaël Zasso <targos@protonmail.com> Reviewed-By: Luigi Pinca <luigipinca@gmail.com> Reviewed-By: Juan José Arboleda <soyjuanarbol@gmail.com> Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Darshan Sen <raisinten@gmail.com> Reviewed-By: Tobias Nießen <tniessen@tnie.de>

Refs: #33716 Refs: #35993 Refs: #35911 PR-URL: #39972 Reviewed-By: Michaël Zasso <targos@protonmail.com> Reviewed-By: Luigi Pinca <luigipinca@gmail.com> Reviewed-By: Juan José Arboleda <soyjuanarbol@gmail.com> Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Darshan Sen <raisinten@gmail.com> Reviewed-By: Tobias Nießen <tniessen@tnie.de>

nodejs-github-bot added the fs Issues and PRs related to the fs subsystem / file system. label Nov 1, 2020

jasnell approved these changes Nov 1, 2020

View reviewed changes

addaleax added the semver-minor PRs that contain new features and should be released in the next minor version. label Nov 1, 2020

addaleax approved these changes Nov 1, 2020

View reviewed changes

mcollina requested changes Nov 2, 2020

View reviewed changes

mcollina requested review from targos, addaleax and jasnell November 2, 2020 09:24

mcollina approved these changes Nov 2, 2020

View reviewed changes

Trott approved these changes Nov 2, 2020

View reviewed changes

targos approved these changes Nov 2, 2020

View reviewed changes

fs: add support for AbortSignal in readFile

ad6eac3

benjamingr added the request-ci Add this label to start a Jenkins CI on a PR. label Nov 4, 2020

benjamingr force-pushed the readfile-abort-signal branch from 838cffb to ad6eac3 Compare November 4, 2020 10:20

github-actions bot removed the request-ci Add this label to start a Jenkins CI on a PR. label Nov 4, 2020

benjamingr added the request-ci Add this label to start a Jenkins CI on a PR. label Nov 5, 2020

github-actions bot removed the request-ci Add this label to start a Jenkins CI on a PR. label Nov 5, 2020

benjamingr added the commit-queue Add this label to land a pull request using GitHub Actions. label Nov 5, 2020

github-actions bot removed the commit-queue Add this label to land a pull request using GitHub Actions. label Nov 5, 2020

github-actions bot added the commit-queue-failed An error occurred while landing this pull request using GitHub Actions. label Nov 5, 2020

benjamingr closed this Nov 6, 2020

benjamingr deleted the readfile-abort-signal branch November 6, 2020 11:16

benjamingr mentioned this pull request Nov 6, 2020

fs: support abortsignal in writeFile #35993

Closed

danielleadams mentioned this pull request Nov 9, 2020

v15.2.0 proposal #36055

Merged

danielleadams added a commit that referenced this pull request Nov 9, 2020

2020-11-10, Version 15.2.0 (Current)

f24be89

Notable changes: * events: getEventListeners static (Benjamin Gruenbaum) (#35991) * fs: add support for AbortSignal in readFile (Benjamin Gruenbaum) (#35911) PR URL: #36055

danielleadams added a commit that referenced this pull request Nov 10, 2020

2020-11-10, Version 15.2.0 (Current)

5d9154f

Notable changes: * events: getEventListeners static (Benjamin Gruenbaum) (#35991) * fs: add support for AbortSignal in readFile (Benjamin Gruenbaum) (#35911) PR URL: #36055

mmomtchev mentioned this pull request Nov 24, 2020

errors: eliminate all overhead for hidden calls #35644

Closed

3 tasks

targos added backport-open-v14.x and removed commit-queue-failed An error occurred while landing this pull request using GitHub Actions. labels Apr 24, 2021

targos mentioned this pull request Apr 24, 2021

[v14.x] Backport AbortController and friends #38386

Closed

Linkgoron mentioned this pull request Apr 24, 2021

[v14.x backport] fs: improve fsPromises readFile performance #37703

Closed

targos added backported-to-v14.x and removed backport-open-v14.x labels Apr 30, 2021

danielleadams mentioned this pull request May 3, 2021

v14.17.0 proposal #38507

Merged

aduh95 added a commit to aduh95/node that referenced this pull request Sep 1, 2021

doc: fix missing history version in fs.md

9db9ae6

Refs: nodejs#33716 Refs: nodejs#35993 Refs: nodejs#35911

aduh95 mentioned this pull request Sep 1, 2021

doc: fix missing history version in fs.md #39972

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fs: add support for AbortSignal in readFile #35911

fs: add support for AbortSignal in readFile #35911

benjamingr commented Nov 1, 2020

mscdex commented Nov 1, 2020 •

edited

Loading

benjamingr commented Nov 1, 2020

mcollina Nov 2, 2020

targos Nov 2, 2020

mcollina Nov 2, 2020

benjamingr Nov 2, 2020

mcollina Nov 2, 2020

benjamingr Nov 2, 2020

mcollina commented Nov 2, 2020

mcollina left a comment

nodejs-github-bot commented Nov 4, 2020

nodejs-github-bot commented Nov 5, 2020

nodejs-github-bot commented Nov 5, 2020

github-actions bot commented Nov 5, 2020

benjamingr commented Nov 6, 2020

fs: add support for AbortSignal in readFile #35911

fs: add support for AbortSignal in readFile #35911

Conversation

benjamingr commented Nov 1, 2020

mscdex commented Nov 1, 2020 • edited Loading

benjamingr commented Nov 1, 2020

mcollina Nov 2, 2020

Choose a reason for hiding this comment

targos Nov 2, 2020

Choose a reason for hiding this comment

mcollina Nov 2, 2020

Choose a reason for hiding this comment

benjamingr Nov 2, 2020

Choose a reason for hiding this comment

mcollina Nov 2, 2020

Choose a reason for hiding this comment

benjamingr Nov 2, 2020

Choose a reason for hiding this comment

mcollina commented Nov 2, 2020

mcollina left a comment

Choose a reason for hiding this comment

nodejs-github-bot commented Nov 4, 2020

nodejs-github-bot commented Nov 5, 2020

nodejs-github-bot commented Nov 5, 2020

github-actions bot commented Nov 5, 2020

benjamingr commented Nov 6, 2020

mscdex commented Nov 1, 2020 •

edited

Loading