fs: allow writing files with ArrayBuffers#46490
fs: allow writing files with ArrayBuffers#46490SRHerzog wants to merge 6 commits intonodejs:mainfrom SRHerzog:write-arraybuffer
Conversation
aduh95
left a comment
There was a problem hiding this comment.
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.
I would imagine both of these cases to come out roughly the same, tbh. Creating an |
|
I've just tested passing an ArrayBuffer directly to the C++ binding (changed the validation in |
|
Should this also allow for |
LiviaMedeiros
left a comment
There was a problem hiding this comment.
This would be a great thing to have, thank you!
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
LiviaMedeiros
left a comment
There was a problem hiding this comment.
The code LGTM!
If there are no objections, it seems to be a
semver-minor
Support of ArrayBuffer and SharedArrayBuffer should be added as changelog entries in documentation, e.g. here:
Lines 655 to 662 in fdad5c2
| --> | ||
|
|
||
| * `buffer` {Buffer|TypedArray|DataView} | ||
| * `buffer` {Buffer|TypedArray|DataView|ArrayBuffer} |
There was a problem hiding this comment.
Support of SharedArrayBuffer should be documented
| * `buffer` {Buffer|TypedArray|DataView|ArrayBuffer} | |
| * `buffer` {Buffer|TypedArray|DataView|ArrayBuffer|SharedArrayBuffer} |
| * 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 |
There was a problem hiding this comment.
| * @param {string | Buffer | TypedArray | DataView | ArrayBuffer} data | |
| * @param {string | Buffer | TypedArray | DataView | ArrayBuffer | SharedArrayBuffer} data |
| throw new ERR_INVALID_ARG_TYPE( | ||
| name, | ||
| ['string', 'Buffer', 'TypedArray', 'DataView'], | ||
| ['string', 'Buffer', 'TypedArray', 'DataView', 'ArrayBuffer'], |
There was a problem hiding this comment.
| ['string', 'Buffer', 'TypedArray', 'DataView', 'ArrayBuffer'], | |
| ['string', 'Buffer', 'TypedArray', 'DataView', 'ArrayBuffer', 'SharedArrayBuffer'], |
|
|
||
| let offset = offsetOrOptions; | ||
| if (isArrayBufferView(buffer)) { | ||
| const bufferToWrite = isArrayBuffer(buffer) || isSharedArrayBuffer(buffer) ? new Uint8Array(buffer) : buffer; |
There was a problem hiding this comment.
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
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