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.

Sign up for GitHub

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

Jump to bottom

fs: allow writing files with ArrayBuffers #46490

Open
wants to merge 6 commits into
base: main
Choose a base branch
from
Open

fs: allow writing files with ArrayBuffers #46490

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
5802e95
fs: allow writing files with ArrayBuffers
SRHerzog Feb 3, 2023
ad09466
implement writev for arraybuffers, update tests and docs
SRHerzog Feb 8, 2023
d9bd41f
update docs and tests
SRHerzog Feb 9, 2023
7b5fe63
minor fixes
SRHerzog Feb 17, 2023
7f63710
fix buffer offset in test
SRHerzog Feb 22, 2023
fdad5c2
allow SharedArrayBuffer as well as ArrayBuffer
SRHerzog Mar 13, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
50 changes: 26 additions & 24 deletions doc/api/fs.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -661,7 +661,7 @@ changes:
buffers anymore.
-->

* `buffer` {Buffer|TypedArray|DataView}
* `buffer` {Buffer|TypedArray|DataView|ArrayBuffer}
Copy link
Contributor

Choose a reason for hiding this comment

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

Support of SharedArrayBuffer should be documented

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

* `offset` {integer} The start position from within `buffer` where the data
to write begins.
* `length` {integer} The number of bytes from `buffer` to write. **Default:**
Expand All @@ -677,7 +677,7 @@ Write `buffer` to the file.
The promise is resolved with an object containing two properties:

* `bytesWritten` {integer} the number of bytes written
* `buffer` {Buffer|TypedArray|DataView} a reference to the
* `buffer` {Buffer|TypedArray|DataView|ArrayBuffer} a reference to the
`buffer` written.

It is unsafe to use `filehandle.write()` multiple times on the same file
Expand All @@ -696,7 +696,7 @@ added:
- v16.17.0
-->

* `buffer` {Buffer|TypedArray|DataView}
* `buffer` {Buffer|TypedArray|DataView|ArrayBuffer}
* `options` {Object}
* `offset` {integer} **Default:** `0`
* `length` {integer} **Default:** `buffer.byteLength - offset`
Expand Down Expand Up @@ -760,7 +760,8 @@ changes:
strings anymore.
-->

* `data` {string|Buffer|TypedArray|DataView|AsyncIterable|Iterable|Stream}
* `data` {string|Buffer|TypedArray|DataView|ArrayBuffer|AsyncIterable|
Iterable|Stream}
* `options` {Object|string}
* `encoding` {string|null} The expected character encoding when `data` is a
string. **Default:** `'utf8'`
Expand Down Expand Up @@ -788,19 +789,19 @@ beginning of the file.
added: v12.9.0
-->

* `buffers` {Buffer\[]|TypedArray\[]|DataView\[]}
* `buffers` {Buffer\[]|TypedArray\[]|DataView\[]|ArrayBuffer\[]}
* `position` {integer|null} The offset from the beginning of the file where the
data from `buffers` should be written. If `position` is not a `number`,
the data will be written at the current position. **Default:** `null`
* Returns: {Promise}

Write an array of {ArrayBufferView}s to the file.
Write an array of {ArrayBufferView}s or {ArrayBuffer}s to the file.

The promise is resolved with an object containing a two properties:

* `bytesWritten` {integer} the number of bytes written
* `buffers` {Buffer\[]|TypedArray\[]|DataView\[]} a reference to the `buffers`
input.
* `buffers` {Buffer\[]|TypedArray\[]|DataView\[]|ArrayBuffer\[]} a reference
to the `buffers` input.

It is unsafe to call `writev()` multiple times on the same file without waiting
for the promise to be resolved (or rejected).
Expand Down Expand Up @@ -856,15 +857,15 @@ added: v10.0.0
-->

* `path` {string|Buffer|URL|FileHandle} filename or {FileHandle}
* `data` {string|Buffer}
* `data` {string|Buffer|ArrayBuffer}
* `options` {Object|string}
* `encoding` {string|null} **Default:** `'utf8'`
* `mode` {integer} **Default:** `0o666`
* `flag` {string} See [support of file system `flags`][]. **Default:** `'a'`.
* Returns: {Promise} Fulfills with `undefined` upon success.

Asynchronously append data to a file, creating the file if it does not yet
exist. `data` can be a string or a {Buffer}.
exist. `data` can be a string, a {Buffer}, or an {ArrayBuffer}.

If `options` is a string, then it specifies the `encoding`.

Expand Down Expand Up @@ -1693,7 +1694,8 @@ changes:
-->

* `file` {string|Buffer|URL|FileHandle} filename or `FileHandle`
* `data` {string|Buffer|TypedArray|DataView|AsyncIterable|Iterable|Stream}
* `data` {string|Buffer|TypedArray|DataView|ArrayBuffer|AsyncIterable
|Iterable|Stream}
* `options` {Object|string}
* `encoding` {string|null} **Default:** `'utf8'`
* `mode` {integer} **Default:** `0o666`
Expand Down Expand Up @@ -1989,7 +1991,7 @@ changes:
-->

* `path` {string|Buffer|URL|number} filename or file descriptor
* `data` {string|Buffer}
* `data` {string|Buffer|ArrayBuffer}
* `options` {Object|string}
* `encoding` {string|null} **Default:** `'utf8'`
* `mode` {integer} **Default:** `0o666`
Expand All @@ -1998,7 +2000,7 @@ changes:
* `err` {Error}

Asynchronously append data to a file, creating the file if it does not yet
exist. `data` can be a string or a {Buffer}.
exist. `data` can be a string, a {Buffer}, or an {ArrayBuffer}.

The `mode` option only affects the newly created file. See [`fs.open()`][]
for more details.
Expand Down Expand Up @@ -4594,7 +4596,7 @@ changes:
-->

* `fd` {integer}
* `buffer` {Buffer|TypedArray|DataView}
* `buffer` {Buffer|TypedArray|DataView|ArrayBuffer}
* `offset` {integer} **Default:** `0`
* `length` {integer} **Default:** `buffer.byteLength - offset`
* `position` {integer|null} **Default:** `null`
Expand Down Expand Up @@ -4780,7 +4782,7 @@ changes:
-->

* `file` {string|Buffer|URL|integer} filename or file descriptor
* `data` {string|Buffer|TypedArray|DataView}
* `data` {string|Buffer|TypedArray|DataView|ArrayBuffer}
* `options` {Object|string}
* `encoding` {string|null} **Default:** `'utf8'`
* `mode` {integer} **Default:** `0o666`
Expand Down Expand Up @@ -4891,15 +4893,15 @@ changes:
-->

* `fd` {integer}
* `buffers` {ArrayBufferView\[]}
* `buffers` {ArrayBufferView\[]|ArrayBuffer\[]}
* `position` {integer|null} **Default:** `null`
* `callback` {Function}
* `err` {Error}
* `bytesWritten` {integer}
* `buffers` {ArrayBufferView\[]}

Write an array of `ArrayBufferView`s to the file specified by `fd` using
`writev()`.
Write an array of `ArrayBufferView`s or `ArrayBuffer`s to the file specified
by `fd` using `writev()`.

`position` is the offset from the beginning of the file where this data
should be written. If `typeof position !== 'number'`, the data will be written
Expand Down Expand Up @@ -4973,14 +4975,14 @@ changes:
-->

* `path` {string|Buffer|URL|number} filename or file descriptor
* `data` {string|Buffer}
* `data` {string|Buffer|ArrayBuffer}
* `options` {Object|string}
* `encoding` {string|null} **Default:** `'utf8'`
* `mode` {integer} **Default:** `0o666`
* `flag` {string} See [support of file system `flags`][]. **Default:** `'a'`.

Synchronously append data to a file, creating the file if it does not yet
exist. `data` can be a string or a {Buffer}.
exist. `data` can be a string, a {Buffer}, or an {ArrayBuffer}.

The `mode` option only affects the newly created file. See [`fs.open()`][]
for more details.
Expand Down Expand Up @@ -6019,7 +6021,7 @@ changes:
-->

* `file` {string|Buffer|URL|integer} filename or file descriptor
* `data` {string|Buffer|TypedArray|DataView}
* `data` {string|Buffer|TypedArray|DataView|ArrayBuffer}
* `options` {Object|string}
* `encoding` {string|null} **Default:** `'utf8'`
* `mode` {integer} **Default:** `0o666`
Expand Down Expand Up @@ -6055,7 +6057,7 @@ changes:
-->

* `fd` {integer}
* `buffer` {Buffer|TypedArray|DataView}
* `buffer` {Buffer|TypedArray|DataView|ArrayBuffer}
* `offset` {integer} **Default:** `0`
* `length` {integer} **Default:** `buffer.byteLength - offset`
* `position` {integer|null} **Default:** `null`
Expand All @@ -6073,7 +6075,7 @@ added:
-->

* `fd` {integer}
* `buffer` {Buffer|TypedArray|DataView}
* `buffer` {Buffer|TypedArray|DataView|ArrayBuffer}
* `options` {Object}
* `offset` {integer} **Default:** `0`
* `length` {integer} **Default:** `buffer.byteLength - offset`
Expand Down Expand Up @@ -6113,7 +6115,7 @@ added: v12.9.0
-->

* `fd` {integer}
* `buffers` {ArrayBufferView\[]}
* `buffers` {ArrayBufferView\[]|ArrayBuffer\[]}
* `position` {integer|null} **Default:** `null`
* Returns: {number} The number of bytes written.

Expand Down
32 changes: 23 additions & 9 deletions lib/fs.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -38,6 +38,7 @@ const {
StringPrototypeCharCodeAt,
StringPrototypeIndexOf,
StringPrototypeSlice,
Uint8Array,
} = primordials;

const { fs: constants } = internalBinding('constants');
Expand All @@ -56,7 +57,7 @@ const {
} = constants;

const pathModule = require('path');
const { isArrayBufferView } = require('internal/util/types');
const { isArrayBufferView, isArrayBuffer, isSharedArrayBuffer } = require('internal/util/types');

// We need to get the statValues from the binding at the callsite since
// it's re-initialized after deserialization.
Expand Down Expand Up @@ -108,6 +109,7 @@ const {
stringToFlags,
stringToSymlinkType,
toUnixTimestamp,
validateAndNormalizeBufferArray,
validateBufferArray,
validateCpOptions,
validateOffsetLengthRead,
Expand Down Expand Up @@ -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;
Copy link
Contributor

@zbjornson zbjornson Aug 12, 2023 •
edited

Choose a reason for hiding this comment

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

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

if (isArrayBufferView(bufferToWrite)) {
callback = maybeCallback(callback || position || length || offset);

if (typeof offset === 'object') {
Expand Down Expand Up @@ -888,6 +891,9 @@ function writeSync(fd, buffer, offsetOrOptions, length, position) {
let result;

let offset = offsetOrOptions;
if (isArrayBuffer(buffer) || isSharedArrayBuffer(buffer)) {
buffer = new Uint8Array(buffer);
}
if (isArrayBufferView(buffer)) {
if (typeof offset === 'object') {
({
Expand Down Expand Up @@ -940,7 +946,7 @@ function writev(fd, buffers, position, callback) {
}

fd = getValidatedFd(fd);
validateBufferArray(buffers);
const buffersToWrite = validateAndNormalizeBufferArray(buffers);
callback = maybeCallback(callback || position);

if (buffers.length === 0) {
Expand All @@ -954,7 +960,7 @@ function writev(fd, buffers, position, callback) {
if (typeof position !== 'number')
position = null;

return binding.writeBuffers(fd, buffers, position, req);
return binding.writeBuffers(fd, buffersToWrite, position, req);
}

ObjectDefineProperty(writev, kCustomPromisifyArgsSymbol, {
Expand All @@ -973,7 +979,7 @@ ObjectDefineProperty(writev, kCustomPromisifyArgsSymbol, {
*/
function writevSync(fd, buffers, position) {
fd = getValidatedFd(fd);
validateBufferArray(buffers);
buffers = validateAndNormalizeBufferArray(buffers);

if (buffers.length === 0) {
return 0;
Expand Down Expand Up @@ -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
Copy link
Contributor

Choose a reason for hiding this comment

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

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

* @param {{
* encoding?: string | null;
* mode?: number;
Expand All @@ -2178,6 +2184,10 @@ function writeFile(path, data, options, callback) {
options = getOptions(options, { encoding: 'utf8', mode: 0o666, flag: 'w' });
const flag = options.flag || 'w';

if (isArrayBuffer(data) || isSharedArrayBuffer(data)) {
data = new Uint8Array(data);
}

if (!isArrayBufferView(data)) {
validateStringAfterArrayBufferView(data, 'data');
data = Buffer.from(data, options.encoding || 'utf8');
Expand Down Expand Up @@ -2207,7 +2217,7 @@ function writeFile(path, data, options, callback) {
/**
* Synchronously writes data to the file.
* @param {string | Buffer | URL | number} path
* @param {string | Buffer | TypedArray | DataView} data
* @param {string | Buffer | TypedArray | DataView | ArrayBuffer} data
* @param {{
* encoding?: string | null;
* mode?: number;
Expand All @@ -2218,6 +2228,10 @@ function writeFile(path, data, options, callback) {
function writeFileSync(path, data, options) {
options = getOptions(options, { encoding: 'utf8', mode: 0o666, flag: 'w' });

if (isArrayBuffer(data) || isSharedArrayBuffer(data)) {
data = new Uint8Array(data);
}

if (!isArrayBufferView(data)) {
validateStringAfterArrayBufferView(data, 'data');
data = Buffer.from(data, options.encoding || 'utf8');
Expand All @@ -2244,7 +2258,7 @@ function writeFileSync(path, data, options) {
/**
* Asynchronously appends data to a file.
* @param {string | Buffer | URL | number} path
* @param {string | Buffer} data
* @param {string | Buffer | ArrayBuffer} data
* @param {{
* encoding?: string | null;
* mode?: number;
Expand All @@ -2270,7 +2284,7 @@ function appendFile(path, data, options, callback) {
/**
* Synchronously appends data to a file.
* @param {string | Buffer | URL | number} path
* @param {string | Buffer} data
* @param {string | Buffer | ArrayBuffer} data
* @param {{
* encoding?: string | null;
* mode?: number;
Expand Down