GV

GeezerVison

The World Wide Web, restored to 800 pixels of glory.

Home|About|Gaming cabinet|Archives|How it works|Starter sites

On this site

  • Sign in
  • ActionsCI/CD and automation
  • CodespacesCloud development environments
  • All Features
  • Documentation
  • Training
  • Certifications
  • Build with Copilot
  • Customize Copilot
  • Integrations
  • Trending repos
  • GitHub Sponsors
  • Developer articles
  • Enterprises

This view

  • Original page
  • New address

GeezerVison is showing github.com — fs: add support for AbortSignal in readFile by benjamingr · Pull Request #35911 · nodejs/node

Conversation

@benjamingr

@benjamingr benjamingr commented Nov 1, 2020

Copy link
Copy Markdown
Member

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

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
bricss reacted with thumbs up emoji
All reactions
  • 👍 1 reaction
@nodejs-github-bot nodejs-github-bot added the fs Issues and PRs related to file-system APIs and the fs module. label Nov 1, 2020
jasnell
jasnell approved these changes Nov 1, 2020
View reviewed changes
@mscdex

mscdex commented Nov 1, 2020 •
edited
Loading

Uh oh!

There was an error while loading. Please reload this page.

Copy link
Copy Markdown
Contributor

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, mcollina, Bnaya, and styfle reacted with thumbs up emoji
All reactions
  • 👍 4 reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@benjamingr

benjamingr commented Nov 1, 2020

Copy link
Copy Markdown
Member Author

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

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@addaleax addaleax added the semver-minor PRs that contain new features and should be released in the next minor version. label Nov 1, 2020
addaleax
addaleax approved these changes Nov 1, 2020
View reviewed changes
mcollina
mcollina requested changes Nov 2, 2020
View reviewed changes
Comment thread lib/internal/fs/promises.js Outdated

@mcollina mcollina Nov 2, 2020

Copy link
Copy Markdown
Member

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

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

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions

@targos targos Nov 2, 2020

Copy link
Copy Markdown
Member

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

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'

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
benjamingr and addaleax reacted with thumbs up emoji mcollina reacted with thumbs down emoji
All reactions
  • 👍 2 reactions
  • 👎 1 reaction

@mcollina mcollina Nov 2, 2020

Copy link
Copy Markdown
Member

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

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.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions

@benjamingr benjamingr Nov 2, 2020

Copy link
Copy Markdown
Member Author

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

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

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
addaleax and styfle reacted with thumbs up emoji
All reactions
  • 👍 2 reactions

@mcollina mcollina Nov 2, 2020

Copy link
Copy Markdown
Member

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

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.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions

@benjamingr benjamingr Nov 2, 2020

Copy link
Copy Markdown
Member Author

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

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?

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
@mcollina
mcollina requested review from addaleax, jasnell and targos November 2, 2020 09:24
@mcollina

mcollina commented Nov 2, 2020

Copy link
Copy Markdown
Member

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

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
mcollina
mcollina approved these changes Nov 2, 2020
View reviewed changes

@mcollina mcollina left a comment

Copy link
Copy Markdown
Member

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

lgtm

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
Trott
Trott approved these changes Nov 2, 2020
View reviewed changes
targos
targos approved these changes Nov 2, 2020
View reviewed changes
@benjamingr
fs: add support for AbortSignal in readFile
ad6eac3
@benjamingr benjamingr added the request-ci Add this label to start a Jenkins CI on a PR. Only starts once the PR has an approving review. label Nov 4, 2020
@benjamingr
benjamingr force-pushed the readfile-abort-signal branch from 838cffb to ad6eac3 Compare November 4, 2020 10:20
@github-actions github-actions Bot removed the request-ci Add this label to start a Jenkins CI on a PR. Only starts once the PR has an approving review. label Nov 4, 2020
@nodejs-github-bot

nodejs-github-bot commented Nov 4, 2020

Copy link
Copy Markdown
Collaborator

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

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@benjamingr benjamingr added the request-ci Add this label to start a Jenkins CI on a PR. Only starts once the PR has an approving review. label Nov 5, 2020
@github-actions github-actions Bot removed the request-ci Add this label to start a Jenkins CI on a PR. Only starts once the PR has an approving review. label Nov 5, 2020
@nodejs-github-bot

nodejs-github-bot commented Nov 5, 2020

Copy link
Copy Markdown
Collaborator

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

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@nodejs-github-bot

nodejs-github-bot commented Nov 5, 2020

Copy link
Copy Markdown
Collaborator

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

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@benjamingr benjamingr added the commit-queue PRs queued for automated landing through the Commit Queue. label Nov 5, 2020
@github-actions github-actions Bot removed the commit-queue PRs queued for automated landing through the Commit Queue. label Nov 5, 2020
@github-actions

github-actions Bot commented Nov 5, 2020

Copy link
Copy Markdown
Contributor
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
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@github-actions github-actions Bot added the commit-queue-failed PRs whose Commit Queue landing failed and need manual intervention before retrying. label Nov 5, 2020
benjamingr added a commit that referenced this pull request Nov 6, 2020
@benjamingr
fs: add support for AbortSignal in readFile
b5a136c 
PR-URL: #35911
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]>
@benjamingr

benjamingr commented Nov 6, 2020

Copy link
Copy Markdown
Member Author

Landed manually in b5a136c
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@benjamingr benjamingr closed this Nov 6, 2020
@benjamingr
benjamingr deleted the readfile-abort-signal branch November 6, 2020 11:16
@benjamingr benjamingr mentioned this pull request Nov 6, 2020
fs: support abortsignal in writeFile #35993
Closed
danielleadams pushed a commit that referenced this pull request Nov 9, 2020
@benjamingr @danielleadams
fs: add support for AbortSignal in readFile
92bdfd1 
PR-URL: #35911
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]>
@danielleadams 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
@danielleadams
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
@danielleadams
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
danielleadams added a commit that referenced this pull request Nov 10, 2020
@danielleadams
2020-11-10, Version 15.2.0 (Current)
3acd275 
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
danielleadams added a commit that referenced this pull request Nov 10, 2020
@danielleadams
2020-11-10, Version 15.2.0 (Current)
5ecd1c9 
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
danielleadams added a commit that referenced this pull request Nov 10, 2020
@danielleadams
2020-11-10, Version 15.2.0 (Current)
f5c508c 
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
@mmomtchev mmomtchev mentioned this pull request Nov 24, 2020
errors: eliminate all overhead for hidden calls #35644
Closed
3 tasks done
@targos targos added backport-open-v14.x and removed commit-queue-failed PRs whose Commit Queue landing failed and need manual intervention before retrying. labels Apr 24, 2021
@targos targos mentioned this pull request Apr 24, 2021
[v14.x] Backport AbortController and friends #38386
Closed
@Linkgoron Linkgoron mentioned this pull request Apr 24, 2021
[v14.x backport] fs: improve fsPromises readFile performance #37703
Closed
targos pushed a commit to targos/node that referenced this pull request Apr 30, 2021
@benjamingr @targos
fs: add support for AbortSignal in readFile
dc3d852 
PR-URL: nodejs#35911
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]>
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 <[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]>
@targos targos added backported-to-v14.x and removed backport-open-v14.x labels Apr 30, 2021
@danielleadams 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
@aduh95
doc: fix missing history version in fs.md
9db9ae6 
Refs: nodejs#33716
Refs: nodejs#35993
Refs: nodejs#35911
@aduh95 aduh95 mentioned this pull request Sep 1, 2021
doc: fix missing history version in fs.md #39972
Closed
nodejs-github-bot pushed a commit that referenced this pull request Sep 6, 2021
@aduh95 @nodejs-github-bot
doc: fix missing history version in fs.md
3f78545 
Refs: #33716
Refs: #35993
Refs: #35911

PR-URL: #39972
Reviewed-By: Michaël Zasso <[email protected]>
Reviewed-By: Luigi Pinca <[email protected]>
Reviewed-By: Juan José Arboleda <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Darshan Sen <[email protected]>
Reviewed-By: Tobias Nießen <[email protected]>
nodejs-github-bot pushed a commit that referenced this pull request Sep 6, 2021
@aduh95 @nodejs-github-bot
doc: fix history for fs.WriteStream open event
3e774a0 
PR-URL: #39972
Refs: #33716
Refs: #35993
Refs: #35911
Reviewed-By: Michaël Zasso <[email protected]>
Reviewed-By: Luigi Pinca <[email protected]>
Reviewed-By: Juan José Arboleda <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Darshan Sen <[email protected]>
Reviewed-By: Tobias Nießen <[email protected]>
BethGriggs pushed a commit that referenced this pull request Sep 21, 2021
@aduh95 @BethGriggs
doc: fix missing history version in fs.md
b6dd2ea 
Refs: #33716
Refs: #35993
Refs: #35911

PR-URL: #39972
Reviewed-By: Michaël Zasso <[email protected]>
Reviewed-By: Luigi Pinca <[email protected]>
Reviewed-By: Juan José Arboleda <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Darshan Sen <[email protected]>
Reviewed-By: Tobias Nießen <[email protected]>
@coderabbitai coderabbitai Bot mentioned this pull request Aug 10, 2026
fix(multimodal): deliver file content to the model instead of describing it juspay/neurolink#1309
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@mcollina mcollina mcollina approved these changes
@Trott Trott Trott approved these changes
@addaleax addaleax Awaiting requested review from addaleax
@jasnell jasnell Awaiting requested review from jasnell
+1 more reviewer
@targos targos targos approved these changes
Reviewers whose approvals may not affect merge requirements

Uh oh!

There was an error while loading. Please reload this page.

Assignees

No one assigned

Labels

fs Issues and PRs related to file-system APIs and the fs module. 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.

Uh oh!

There was an error while loading. Please reload this page.

8 participants

@benjamingr @mscdex @mcollina @nodejs-github-bot @jasnell @Trott @addaleax @targos
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
You are visitor 01402503 — GeezerVison does not store the pages you look up.
Best viewed with Netscape Navigator 4.7 or Internet Explorer 6 at 800×600. No cookies required.