fs: allow writing files with ArrayBuffers #46490

SRHerzog · 2023-02-03T22:09:25Z

Extended most file writing functions to accept ArrayBuffers rather than only strings, buffers, or data views. Does not include functions that accept arrays of dataviews, such as writev.

Fixes: #42228

aduh95

This would need a documentation update. Could you avoid creating a Uint8Array? It seems like ArrayBuffers could probably be passed directly to the C++ bindings and be more efficient.

lib/internal/fs/promises.js

addaleax · 2023-02-04T13:29:52Z

Could you avoid creating a Uint8Array? It seems like ArrayBuffers could probably be passed directly to the C++ bindings and be more efficient.

I would imagine both of these cases to come out roughly the same, tbh. Creating an Uint8Array for an existing ArrayBuffer is not a complex operation, and it’s also not really less efficient to unwrap an Uint8Array on the C++ side than an ArrayBuffer, so I think it’s fine to have readability as the main concern here.

SRHerzog · 2023-02-06T20:01:13Z

I've just tested passing an ArrayBuffer directly to the C++ binding (changed the validation in filehandle.write to accept either a typed array or an ArrayBuffer), and it causes the program to crash.

async function validateWrite() {
  const filePathForHandle = path.resolve(tmpDir, 'tmp-write.txt');
  const fileHandle = await open(filePathForHandle, 'w+');
  const buffer = Buffer.from('Hello world'.repeat(100), 'utf8');

  await fileHandle.write(buffer.buffer, 0, buffer.length);
  const readFileData = fs.readFileSync(filePathForHandle);
  assert.deepStrictEqual(buffer, readFileData);

  await fileHandle.close();
}

../../src/node_file.cc:2026:void node::fs::WriteBuffer(const FunctionCallbackInfo<v8::Value> &): Assertion `Buffer::HasInstance(args[1])' failed.
 1: 0x102fcc05c node::Abort() [/projects/node/node/out/Release/node]
 2: 0x102fcbe90 node::AppendExceptionLine(node::Environment*, v8::Local<v8::Value>, v8::Local<v8::Message>, node::ErrorHandlingMode) [/projects/node/node/out/Release/node]
 3: 0x102fdfffc node::fs::WriteBuffer(v8::FunctionCallbackInfo<v8::Value> const&) [/projects/node/node/out/Release/node]
 ... 30 more lines in stack trace
--- CRASHED (Signal: 6) ---

jasnell · 2023-02-11T15:21:58Z

Should this also allow for SharedArrayBuffer instances?

LiviaMedeiros

This would be a great thing to have, thank you!

lib/internal/fs/promises.js

lib/internal/fs/utils.js

lib/internal/fs/promises.js

Extended most file writing functions to accept ArrayBuffers rather than only strings, buffers, or data views. Does not include functions that accept arrays of dataviews, such as writev. Fixes: nodejs#42228

test/parallel/test-fs-promises-file-handle-write.js

lib/fs.js

lib/internal/fs/utils.js

LiviaMedeiros

The code LGTM!
If there are no objections, it seems to be a semver-minor PRs that contain new features and should be released in the next minor version. change.

Support of ArrayBuffer and SharedArrayBuffer should be added as changelog entries in documentation, e.g. here:

node/doc/api/fs.md

Lines 655 to 662 in fdad5c2

    
           <!-- YAML 
        
           added: v10.0.0 
        
           changes: 
        
             - version: v14.0.0 
        
               pr-url: https://github.com/nodejs/node/pull/31030 
        
               description: The `buffer` parameter won't coerce unsupported input to 
        
                            buffers anymore. 
        
           -->

LiviaMedeiros · 2023-03-24T11:36:04Z

doc/api/fs.md

@@ -661,7 +661,7 @@ changes:
                 buffers anymore.
 -->

-* `buffer` {Buffer|TypedArray|DataView}
+* `buffer` {Buffer|TypedArray|DataView|ArrayBuffer}


Support of SharedArrayBuffer should be documented

Suggested change

* `buffer` {Buffer|TypedArray|DataView|ArrayBuffer}

* `buffer` {Buffer|TypedArray|DataView|ArrayBuffer|SharedArrayBuffer}

LiviaMedeiros · 2023-03-24T11:36:33Z

lib/fs.js

@@ -2163,7 +2169,7 @@ function writeAll(fd, isUserFd, buffer, offset, length, signal, callback) {
 /**
 * Asynchronously writes data to the file.
 * @param {string | Buffer | URL | number} path
- * @param {string | Buffer | TypedArray | DataView} data
+ * @param {string | Buffer | TypedArray | DataView | ArrayBuffer} data


Suggested change

* @param {string | Buffer | TypedArray | DataView | ArrayBuffer} data

* @param {string | Buffer | TypedArray | DataView | ArrayBuffer | SharedArrayBuffer} data

LiviaMedeiros · 2023-03-24T11:37:17Z

lib/internal/fs/utils.js

@@ -887,7 +909,7 @@ const validateStringAfterArrayBufferView = hideStackFrames((buffer, name) => {
  if (typeof buffer !== 'string') {
    throw new ERR_INVALID_ARG_TYPE(
      name,
-      ['string', 'Buffer', 'TypedArray', 'DataView'],
+      ['string', 'Buffer', 'TypedArray', 'DataView', 'ArrayBuffer'],


Suggested change

['string', 'Buffer', 'TypedArray', 'DataView', 'ArrayBuffer'],

['string', 'Buffer', 'TypedArray', 'DataView', 'ArrayBuffer', 'SharedArrayBuffer'],

nodejs-github-bot · 2023-03-24T12:55:14Z

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

nodejs-github-bot · 2023-03-24T13:36:26Z

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

zbjornson · 2023-08-12T02:44:13Z

lib/fs.js

@@ -819,7 +821,8 @@ function write(fd, buffer, offsetOrOptions, length, position, callback) {
  fd = getValidatedFd(fd);

  let offset = offsetOrOptions;
-  if (isArrayBufferView(buffer)) {
+  const bufferToWrite = isArrayBuffer(buffer) || isSharedArrayBuffer(buffer) ? new Uint8Array(buffer) : buffer;


ArrayBuffers can have arbitrary length, but TypedArrays are limited to kMaxLength, so this line can fail:

> k = require("buffer").kMaxLength 4294967296 > ab = new ArrayBuffer(k * 2) ArrayBuffer { (detached), byteLength: 8589934592 } > new Uint8Array(ab) Uncaught RangeError: Start offset undefined is outside the bounds of the buffer at new Uint8Array (<anonymous>)

That's a reason to consider passing the ArrayBuffer into C++ and unwrapping it there, or with a bit more waste, validating the int32 length earlier (currently line 845R) and making this ~new Uint8Array(buffer, offset, length).

(uv_fs_write is limited to int32 size, but the offset parameter is int64 and allows calling fs.write() in a loop to write out larger files. See libuv/libuv#3360 and linked issues.)

Also, V8 is removing the kMaxLength limit sometime soon, which would make this a non-issue. https://bugs.chromium.org/p/v8/issues/detail?id=4153

nodejs-github-bot added fs Issues and PRs related to the fs subsystem / file system. needs-ci PRs that need a full CI run. labels Feb 3, 2023

aduh95 reviewed Feb 4, 2023

View reviewed changes

lib/internal/fs/promises.js Outdated Show resolved Hide resolved

SRHerzog force-pushed the write-arraybuffer branch from 74ff0bd to 00952f7 Compare February 9, 2023 15:40

SRHerzog marked this pull request as ready for review February 10, 2023 19:50

LiviaMedeiros reviewed Feb 11, 2023

View reviewed changes

lib/internal/fs/promises.js Outdated Show resolved Hide resolved

lib/internal/fs/utils.js Outdated Show resolved Hide resolved

lib/internal/fs/promises.js Outdated Show resolved Hide resolved

SRHerzog added 4 commits February 17, 2023 13:31

fs: allow writing files with ArrayBuffers

5802e95

Extended most file writing functions to accept ArrayBuffers rather than only strings, buffers, or data views. Does not include functions that accept arrays of dataviews, such as writev. Fixes: nodejs#42228

implement writev for arraybuffers, update tests and docs

ad09466

update docs and tests

d9bd41f

minor fixes

7b5fe63

SRHerzog force-pushed the write-arraybuffer branch from 5c3e7b3 to 7b5fe63 Compare February 17, 2023 19:32

LiviaMedeiros reviewed Feb 18, 2023

View reviewed changes

test/parallel/test-fs-promises-file-handle-write.js Outdated Show resolved Hide resolved

fix buffer offset in test

7f63710

jasnell reviewed Feb 24, 2023

View reviewed changes

lib/fs.js Outdated Show resolved Hide resolved

jasnell reviewed Feb 24, 2023

View reviewed changes

lib/internal/fs/utils.js Outdated Show resolved Hide resolved

allow SharedArrayBuffer as well as ArrayBuffer

fdad5c2

LiviaMedeiros added semver-minor PRs that contain new features and should be released in the next minor version. request-ci Add this label to start a Jenkins CI on a PR. labels Mar 24, 2023

github-actions bot removed the request-ci Add this label to start a Jenkins CI on a PR. label Mar 24, 2023

LiviaMedeiros reviewed Mar 24, 2023

View reviewed changes

github-actions bot mentioned this pull request Mar 30, 2023

CI Reliability 2023-03-30 nodejs/reliability#534

Open

18 tasks

zbjornson reviewed Aug 12, 2023

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fs: allow writing files with ArrayBuffers #46490

fs: allow writing files with ArrayBuffers #46490

SRHerzog commented Feb 3, 2023

aduh95 left a comment

addaleax commented Feb 4, 2023

SRHerzog commented Feb 6, 2023

jasnell commented Feb 11, 2023

LiviaMedeiros left a comment

LiviaMedeiros left a comment

LiviaMedeiros Mar 24, 2023

LiviaMedeiros Mar 24, 2023

LiviaMedeiros Mar 24, 2023

nodejs-github-bot commented Mar 24, 2023

nodejs-github-bot commented Mar 24, 2023

zbjornson Aug 12, 2023 •

edited

Loading

	<!-- YAML
	added: v10.0.0
	changes:
	- version: v14.0.0
	pr-url: https://github.com/nodejs/node/pull/31030
	description: The `buffer` parameter won't coerce unsupported input to
	buffers anymore.
	-->

	* `buffer` {Buffer\|TypedArray\|DataView\|ArrayBuffer}
	* `buffer` {Buffer\|TypedArray\|DataView\|ArrayBuffer\|SharedArrayBuffer}

	* @param {string \| Buffer \| TypedArray \| DataView \| ArrayBuffer} data
	* @param {string \| Buffer \| TypedArray \| DataView \| ArrayBuffer \| SharedArrayBuffer} data

	['string', 'Buffer', 'TypedArray', 'DataView', 'ArrayBuffer'],
	['string', 'Buffer', 'TypedArray', 'DataView', 'ArrayBuffer', 'SharedArrayBuffer'],

fs: allow writing files with ArrayBuffers #46490

Are you sure you want to change the base?

fs: allow writing files with ArrayBuffers #46490

Conversation

SRHerzog commented Feb 3, 2023

aduh95 left a comment

Choose a reason for hiding this comment

addaleax commented Feb 4, 2023

SRHerzog commented Feb 6, 2023

jasnell commented Feb 11, 2023

LiviaMedeiros left a comment

Choose a reason for hiding this comment

LiviaMedeiros left a comment

Choose a reason for hiding this comment

LiviaMedeiros Mar 24, 2023

Choose a reason for hiding this comment

LiviaMedeiros Mar 24, 2023

Choose a reason for hiding this comment

LiviaMedeiros Mar 24, 2023

Choose a reason for hiding this comment

nodejs-github-bot commented Mar 24, 2023

nodejs-github-bot commented Mar 24, 2023

zbjornson Aug 12, 2023 • edited Loading

Choose a reason for hiding this comment

zbjornson Aug 12, 2023 •

edited

Loading