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

fs: support abortsignal in writeFile #35993

Closed
wants to merge 1 commit into from

Conversation

@benjamingr
Copy link
Member

@benjamingr benjamingr commented Nov 6, 2020

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.

@benjamingr benjamingr force-pushed the abort-signal-write-file branch from 01d0a51 to cd7b999 Nov 6, 2020

const fd = await open(path, flag, options.mode);
return writeFileHandle(fd, data).finally(fd.close);
if (options.signal && options.signal.aborted) {
Copy link
Contributor

@aduh95 aduh95 Nov 6, 2020

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) {

Loading

Copy link
Member Author

@benjamingr benjamingr Nov 6, 2020

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?

Loading

Copy link
Contributor

@aduh95 aduh95 Nov 10, 2020

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?

Loading

lib/internal/fs/promises.js Outdated Show resolved Hide resolved
Loading
lib/internal/fs/promises.js Outdated Show resolved Hide resolved
Loading
@benjamingr
Copy link
Member Author

@benjamingr benjamingr commented Nov 6, 2020

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

Loading

@aduh95
Copy link
Contributor

@aduh95 aduh95 commented Nov 6, 2020

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

Loading

@benjamingr
Copy link
Member Author

@benjamingr benjamingr commented Nov 6, 2020

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

Loading

@aduh95
Copy link
Contributor

@aduh95 aduh95 commented Nov 6, 2020

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.

Loading

@richardlau
Copy link
Member

@richardlau richardlau commented Nov 6, 2020

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.

Loading

doc/api/fs.md Outdated Show resolved Hide resolved
Loading
const { signal } = controller;
const data = new Uint8Array(Buffer.from('Hello Node.js'));
fs.writeFile('message.txt', data, { signal }, (err) => {
// When a request is aborted - the callback is called with an AbortError
Copy link
Member

@jasnell jasnell Nov 6, 2020

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.

Loading

Copy link
Member Author

@benjamingr benjamingr Nov 7, 2020

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

Loading

Copy link
Member

@addaleax addaleax Nov 7, 2020

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.

Loading

```

Aborting an ongoing request does not abort individual operating
system requests but rather the internal buffering `fs.writeFile` performs.
Copy link
Member

@jasnell jasnell Nov 6, 2020

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.

Loading

Copy link
Member Author

@benjamingr benjamingr Nov 7, 2020

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?

Loading

Copy link
Member

@mcollina mcollina Nov 7, 2020

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.

Loading

Copy link
Member Author

@benjamingr benjamingr Nov 9, 2020

Choose a reason for hiding this comment

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

@mcollina @jasnell done PTAL

Loading

@nodejs-github-bot
Copy link
Contributor

@nodejs-github-bot nodejs-github-bot commented Nov 7, 2020

Loading

baruchiro added a commit to baruchiro/node that referenced this issue Nov 8, 2020
@benjamingr benjamingr force-pushed the abort-signal-write-file branch from ed7d240 to 1072fc9 Nov 9, 2020
nodejs-github-bot added a commit that referenced this issue Nov 9, 2020
#35993 (comment)

PR-URL: #35995
Reviewed-By: Antoine du Hamel <duhamelantoine1995@gmail.com>
Reviewed-By: Rich Trott <rtrott@gmail.com>
@benjamingr
Copy link
Member Author

@benjamingr benjamingr commented Nov 10, 2020

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

Loading

lib/fs.js Outdated
return;
}

if (options.signal && options.signal.aborted) {
Copy link
Contributor

@aduh95 aduh95 Nov 10, 2020

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) {

Loading

@nodejs-github-bot
Copy link
Contributor

@nodejs-github-bot nodejs-github-bot commented Nov 10, 2020

Loading

@github-actions
Copy link

@github-actions github-actions bot commented Nov 10, 2020

Landed in 9afe2b6...d9dadac

Loading

@github-actions github-actions bot closed this Nov 10, 2020
nodejs-github-bot added a commit that referenced this issue Nov 10, 2020
PR-URL: #35993
Reviewed-By: Antoine du Hamel <duhamelantoine1995@gmail.com>
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
@benjamingr benjamingr deleted the abort-signal-write-file branch Nov 10, 2020
danielleadams added a commit that referenced this issue Nov 10, 2020
#35993 (comment)

PR-URL: #35995
Reviewed-By: Antoine du Hamel <duhamelantoine1995@gmail.com>
Reviewed-By: Rich Trott <rtrott@gmail.com>
danielleadams added a commit that referenced this issue Nov 10, 2020
PR-URL: #35993
Reviewed-By: Antoine du Hamel <duhamelantoine1995@gmail.com>
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
danielleadams added a commit that referenced this issue Nov 10, 2020
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
danielleadams added a commit that referenced this issue Nov 10, 2020
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 issue Nov 10, 2020
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 added a commit that referenced this issue Dec 9, 2020
#35993 (comment)

PR-URL: #35995
Reviewed-By: Antoine du Hamel <duhamelantoine1995@gmail.com>
Reviewed-By: Rich Trott <rtrott@gmail.com>
BethGriggs added a commit that referenced this issue Dec 10, 2020
#35993 (comment)

PR-URL: #35995
Reviewed-By: Antoine du Hamel <duhamelantoine1995@gmail.com>
Reviewed-By: Rich Trott <rtrott@gmail.com>
BethGriggs added a commit that referenced this issue Dec 15, 2020
#35993 (comment)

PR-URL: #35995
Reviewed-By: Antoine du Hamel <duhamelantoine1995@gmail.com>
Reviewed-By: Rich Trott <rtrott@gmail.com>
targos added a commit to targos/node that referenced this issue Apr 24, 2021
PR-URL: nodejs#35993
Reviewed-By: Antoine du Hamel <duhamelantoine1995@gmail.com>
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
targos added a commit to targos/node that referenced this issue Apr 26, 2021
PR-URL: nodejs#35993
Reviewed-By: Antoine du Hamel <duhamelantoine1995@gmail.com>
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
targos added a commit to targos/node that referenced this issue Apr 30, 2021
PR-URL: nodejs#35993
Reviewed-By: Antoine du Hamel <duhamelantoine1995@gmail.com>
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
targos added a commit that referenced this issue 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>
@danielleadams danielleadams mentioned this pull request May 3, 2021
aduh95 added a commit to aduh95/node that referenced this issue Sep 1, 2021
nodejs-github-bot added a commit that referenced this issue Sep 6, 2021
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 a commit that referenced this issue Sep 6, 2021
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>
BethGriggs added a commit that referenced this issue Sep 21, 2021
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>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

9 participants