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: support abortsignal in writeFile by benjamingr · Pull Request #35993 · nodejs/node

Conversation

@benjamingr

@benjamingr benjamingr commented Nov 6, 2020

Copy link
Copy Markdown
Member

Now that #35911 is landed the next suggestion in #35877 by @mcollina was fs.writeFile (here #35877 (comment) ) - so I am going over the suggestions one by one and adding AbortSignal support.

This PR lets you abort an ongoing writeFile request.

const controller = new AbortController();
const { signal } = controller;
const data = new Uint8Array(Buffer.from('Hello Node.js'));
(async () => {
  try {
    await fs.writeFile('message.txt', data, { signal });
  } catch (err) {
  // When a request is aborted - err is an AbortError
  }
})();
// When the request should be aborted
controller.abort();

Note that unlike readFile this is very stateful - so some of the data will be written to the file (this is expected IMO). I vaguely recall in the summit @addaleax has something smart to say regarding the error but I don't remember the conclusion. I am happy to tack the offset (how many bytes were written) on the AbortError if that would help.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
qballer, bricss, and metcoder95 reacted with thumbs up emoji
All reactions
  • 👍 3 reactions
@nodejs-github-bot nodejs-github-bot added the fs Issues and PRs related to file-system APIs and the fs module. label Nov 6, 2020
@benjamingr
benjamingr force-pushed the abort-signal-write-file branch from 01d0a51 to cd7b999 Compare November 6, 2020 11:49
aduh95
aduh95 reviewed Nov 6, 2020
View reviewed changes
Comment thread lib/internal/fs/promises.js Outdated

@aduh95 aduh95 Nov 6, 2020

Copy link
Copy Markdown
Contributor

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.

Suggested change
if (options.signal && options.signal.aborted) {
if (options.signal?.aborted) {

Sorry, something went wrong.

Uh oh!

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

@benjamingr benjamingr Nov 6, 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.

Bacause no backporting and 12.x?

Sorry, something went wrong.

Uh oh!

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

@aduh95 aduh95 Nov 10, 2020

Copy link
Copy Markdown
Contributor

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.

AbortController is not available on v12, is it?

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
Comment thread lib/internal/fs/promises.js Resolved

Uh oh!

There was an error while loading. Please reload this page.
Comment thread lib/internal/fs/promises.js Resolved

Uh oh!

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

benjamingr commented Nov 6, 2020

Copy link
Copy Markdown
Member Author

@nodejs/documentation anyone can help me with the "SyntaxError: Unexpected token (1421:13)" acorn error I'm seeing?
All reactions

Sorry, something went wrong.

Uh oh!

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

aduh95 commented Nov 6, 2020

Copy link
Copy Markdown
Contributor

@nodejs/documentation anyone can help me with the "SyntaxError: Unexpected token (1421:13)" acorn error I'm seeing?

It's related to optional chaining. Fixed by #35994.
benjamingr reacted with thumbs up emoji
All reactions
  • 👍 1 reaction

Sorry, something went wrong.

Uh oh!

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

benjamingr commented Nov 6, 2020

Copy link
Copy Markdown
Member Author

@aduh95

It's related to optional chaining. Fixed by #35994.

That makes sense sort of but... I did not use optional chaining in the docs so why is that failing?
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@baruchiro baruchiro mentioned this pull request Nov 6, 2020
fs: replace finally with PromisePrototypeFinally #35995
Closed
1 of 2 tasks
@aduh95

aduh95 commented Nov 6, 2020

Copy link
Copy Markdown
Contributor

That makes sense sort of but... I did not use optional chaining in the docs so why is that failing?

Not sure either, acorn parsing was added back in #22405.
All reactions

Sorry, something went wrong.

Uh oh!

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

richardlau commented Nov 6, 2020

Copy link
Copy Markdown
Member

The docs build parses the JavaScript source to generate apilinks.json, e.g. https://nodejs.org/docs/v15.1.0/apilinks.json. This file maps methods to the line numbers in the source file and I believe it's used by some IDEs.
benjamingr reacted with thumbs up emoji
All reactions
  • 👍 1 reaction

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
jasnell
jasnell reviewed Nov 6, 2020
View reviewed changes
Comment thread doc/api/fs.md Resolved

Uh oh!

There was an error while loading. Please reload this page.
jasnell
jasnell reviewed Nov 6, 2020
View reviewed changes
Comment thread doc/api/fs.md Outdated

@jasnell jasnell Nov 6, 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 may be a bit difficult, and is likely something that we should tackle later in a separate PR, but it would be helpful if this reported the number of bytes known to have been written at the point of failure/cancelation.

Sorry, something went wrong.

Uh oh!

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

@benjamingr benjamingr Nov 7, 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.

Yeah, I asked in the description it's actually pretty easy to do - but I remember there were really good arguments regarding whether or not we should do this (in the summit, by @addaleax IIRC) and I don't remember what the conclusion was.

Adding the number of bytes written is actually technically pretty easy here if we just want to approximate how much was written since the last write since we don't actually abort writes in progress only writeAll

Sorry, something went wrong.

Uh oh!

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

@addaleax addaleax Nov 7, 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.

Right – I think we basically agreed to see cancellation is a way of expressing disinterest in the result of an operation. Not reporting the number of written bytes goes in line with that. More importantly, if we add cancellation support to the actual fs operations underneath – as we should – then these numbers become inherently unreliable. I would not be in favour of adding this, if somebody wants it, then they should use fs.write() directly without cancelling.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
benjamingr, mcollina, and xprommer reacted with thumbs up emoji
All reactions
  • 👍 3 reactions
jasnell
jasnell reviewed Nov 6, 2020
View reviewed changes
Comment thread doc/api/fs.md Outdated

@jasnell jasnell Nov 6, 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.

Users who are unfamiliar with how writeFile works under the covers may find this sentence confusing.

Sorry, something went wrong.

Uh oh!

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

@benjamingr benjamingr Nov 7, 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.

How do you propose we deal with this? Add an extra note to writeFile explaining how it works and suggesting a createWriteStream for performacne sensitive stuff like we do with readFile?

Sorry, something went wrong.

Uh oh!

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

@mcollina mcollina Nov 7, 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 think it's better to explain how this works underneath instead, and then include the above sentence.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
benjamingr reacted with thumbs up emoji
All reactions
  • 👍 1 reaction

@benjamingr benjamingr Nov 9, 2020 •
edited
Loading

Uh oh!

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

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 @jasnell done PTAL

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
@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 7, 2020
@benjamingr
benjamingr requested a review from mcollina November 7, 2020 16:28
@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 7, 2020
@nodejs-github-bot

nodejs-github-bot commented Nov 7, 2020

Copy link
Copy Markdown
Collaborator

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

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
baruchiro added a commit to baruchiro/node that referenced this pull request Nov 8, 2020
@baruchiro
fs: replace finally with PromisePrototypeFinally
b3d82e0 
nodejs#35993 (comment)
@benjamingr
benjamingr force-pushed the abort-signal-write-file branch from ed7d240 to 1072fc9 Compare November 9, 2020 10:34
@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 9, 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 9, 2020
nodejs-github-bot pushed a commit that referenced this pull request Nov 9, 2020
@baruchiro @nodejs-github-bot
fs: replace finally with PromisePrototypeFinally
a8b9ede 
#35993 (comment)

PR-URL: #35995
Reviewed-By: Antoine du Hamel <[email protected]>
Reviewed-By: Rich Trott <[email protected]>
@benjamingr

benjamingr commented Nov 10, 2020

Copy link
Copy Markdown
Member Author

This needs a rebase and some reviews ^^

@jasnell @mcollina any chance this could get a look? It's weird we support it in readFile but not writeFile
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
aduh95
aduh95 reviewed Nov 10, 2020
View reviewed changes
Comment thread lib/fs.js Outdated

@aduh95 aduh95 Nov 10, 2020

Copy link
Copy Markdown
Contributor

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.

Suggested change
if (options.signal && options.signal.aborted) {
if (options.signal?.aborted) {

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
@benjamingr
fs: support abortsignal in writeFile
017246b
@benjamingr
benjamingr force-pushed the abort-signal-write-file branch from 1072fc9 to 017246b Compare November 10, 2020 08:56
@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 10, 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 10, 2020
@nodejs-github-bot

nodejs-github-bot commented Nov 10, 2020

Copy link
Copy Markdown
Collaborator

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

Sorry, something went wrong.

Uh oh!

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

github-actions Bot commented Nov 10, 2020

Copy link
Copy Markdown
Contributor

Landed in 9afe2b6...d9dadac
benjamingr reacted with rocket emoji
All reactions
  • 🚀 1 reaction

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@github-actions github-actions Bot closed this Nov 10, 2020
nodejs-github-bot pushed a commit that referenced this pull request Nov 10, 2020
@benjamingr @nodejs-github-bot
fs: support abortsignal in writeFile
d9dadac 
PR-URL: #35993
Reviewed-By: Antoine du Hamel <[email protected]>
Reviewed-By: Matteo Collina <[email protected]>
@benjamingr
benjamingr deleted the abort-signal-write-file branch November 10, 2020 14:31
@benjamingr benjamingr added the semver-minor PRs that contain new features and should be released in the next minor version. label Nov 10, 2020
danielleadams pushed a commit that referenced this pull request Nov 10, 2020
@baruchiro @danielleadams
fs: replace finally with PromisePrototypeFinally
7ef9c70 
#35993 (comment)

PR-URL: #35995
Reviewed-By: Antoine du Hamel <[email protected]>
Reviewed-By: Rich Trott <[email protected]>
danielleadams pushed a commit that referenced this pull request Nov 10, 2020
@benjamingr @danielleadams
fs: support abortsignal in writeFile
ccf9f0e 
PR-URL: #35993
Reviewed-By: Antoine du Hamel <[email protected]>
Reviewed-By: Matteo Collina <[email protected]>
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 danielleadams mentioned this pull request Nov 10, 2020
v15.2.0 proposal #36055
Merged
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
BethGriggs pushed a commit that referenced this pull request Dec 9, 2020
@baruchiro @BethGriggs
fs: replace finally with PromisePrototypeFinally
1ed0a03 
#35993 (comment)

PR-URL: #35995
Reviewed-By: Antoine du Hamel <[email protected]>
Reviewed-By: Rich Trott <[email protected]>
BethGriggs pushed a commit that referenced this pull request Dec 10, 2020
@baruchiro @BethGriggs
fs: replace finally with PromisePrototypeFinally
affa710 
#35993 (comment)

PR-URL: #35995
Reviewed-By: Antoine du Hamel <[email protected]>
Reviewed-By: Rich Trott <[email protected]>
BethGriggs pushed a commit that referenced this pull request Dec 15, 2020
@baruchiro @BethGriggs
fs: replace finally with PromisePrototypeFinally
7c3b6f1 
#35993 (comment)

PR-URL: #35995
Reviewed-By: Antoine du Hamel <[email protected]>
Reviewed-By: Rich Trott <[email protected]>
@targos targos added the backport-open-v14.x label Apr 24, 2021
@targos targos mentioned this pull request Apr 24, 2021
[v14.x] Backport AbortController and friends #38386
Closed
targos pushed a commit to targos/node that referenced this pull request Apr 30, 2021
@benjamingr @targos
fs: support abortsignal in writeFile
ceef7e3 
PR-URL: nodejs#35993
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
@benjamingr @targos
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 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 Jun 1, 2026
Add CLI cancellation propagation prisma/prisma-cli#59
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@jasnell jasnell jasnell left review comments
@addaleax addaleax addaleax left review comments
@mcollina mcollina mcollina approved these changes
@aduh95 aduh95 aduh95 approved these changes
+1 more reviewer
@baruchiro baruchiro baruchiro left review comments
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.

9 participants

@benjamingr @aduh95 @richardlau @nodejs-github-bot @mcollina @jasnell @addaleax @baruchiro @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.