`fs` request: allow bigint values in read() and write() calls

[ZaneHannanAU]

• Version: v10.7.0
• Platform: Linux zeen3.localdomain 4.17.7-100.fc27.x86_64 deps: update openssl to 1.0.1j #1 SMP Tue Jul 17 16:29:47 UTC 2018 x86_64 x86_64 x86_64 GNU/Linux
• Subsystem: fs

Currently working on an application that is likely to read & write from values greater than the 32-bit (technically 32 bit signed int) limit in fs.(read|write) and fs.promises.(read|write). Wanted to request that these functions be able to accept bigint values for reading/writing files >= 4GiB.

$ node
> let fh = await fs.promises.open('k.txt', 'r')
(node:6387) ExperimentalWarning: The fs.promises API is experimental
undefined
> await fh.read(Buffer.allocUnsafe(32), 0n, 32n, 32n)
await fh.read(Buffer.allocUnsafe(32), 0n, 32n, 32n)
^^^^^

SyntaxError: await is only valid in async function

> fh.read(Buffer.allocUnsafe(32), 0n, 32n, 32n)
Promise {
  <rejected> TypeError: Cannot mix BigInt and other types, use explicit conversions
    at read (internal/fs/promises.js:205:3)
    at FileHandle.read (internal/fs/promises.js:77:12)
    at repl:1:4
    at Script.runInThisContext (vm.js:91:20)
    at REPLServer.defaultEval (repl.js:321:29)
    at bound (domain.js:396:14)
    at REPLServer.runBound [as eval] (domain.js:409:12)
    at REPLServer.onLine (repl.js:619:10)
    at REPLServer.emit (events.js:187:15)
    at REPLServer.EventEmitter.emit (domain.js:442:20),
  domain:
   Domain {
     domain: null,
     _events:
      { removeListener: [Function: updateExceptionCapture],
        newListener: [Function: updateExceptionCapture],
        error: [Function: debugDomainError] },
     _eventsCount: 3,
     _maxListeners: undefined,
     members: [] } }
> (node:6387) UnhandledPromiseRejectionWarning: TypeError: Cannot mix BigInt and other types, use explicit conversions
    at read (internal/fs/promises.js:205:3)
    at FileHandle.read (internal/fs/promises.js:77:12)
    at repl:1:4
    at Script.runInThisContext (vm.js:91:20)
    at REPLServer.defaultEval (repl.js:321:29)
    at bound (domain.js:396:14)
    at REPLServer.runBound [as eval] (domain.js:409:12)
    at REPLServer.onLine (repl.js:619:10)
    at REPLServer.emit (events.js:187:15)
    at REPLServer.EventEmitter.emit (domain.js:442:20)
(node:6387) UnhandledPromiseRejectionWarning: Unhandled promise rejection. This error originated either by throwing inside of an async function without a catch block, or by rejecting a promise which was not handled with .catch(). (rejection id: 1)
(node:6387) [DEP0018] DeprecationWarning: Unhandled promise rejections are deprecated. In the future, promise rejections that are not handled will terminate the Node.js process with a non-zero exit code.

> $ type node
node is aliased to `node --experimental-repl-await --icu-data-dir=`echo -ne "$HOME"`/node_modules/full-icu'

Other than that; I'd request the allowing of bigint values in eg utimes(), lchmod(), flags, truncate(), mode, interval (on watchFile), highWaterMark (mostly just for consistency) and so on. It'd be a massive patch though; plus would likely make some parts incompatible (eg constants that are not pulling from fs.constants or process.binding('fs').constants).

Of course they shouldn't be using them but... people are human.

[XadillaX]

But Buffer's length cannot be bigint.

[ZaneHannanAU]

Hrmm… then probably only have position as bigint?
If you coerce length to a number maybe; but technically offset in a buffer is fine within js since it's coerced to a string index regardless.

Hrmm…

[ZaneHannanAU]

Though I'm not knowledgeable on the internals of it; sorry.

[XadillaX]

@ZaneHannanAU offset means offset in Buffer and length needs to be smaller than Buffer.length - offset.

So these parameters should be number but not BigInt.

[targos]

The position parameter already accepts values up to Number.MAX_SAFE_INTEGER. It means that fs.read supports files up to 8192 TB.
I'm not strictly opposed to supporting BigInts here, but is it really needed?

[ZaneHannanAU]

Ah. Always get offset and position mixed up.

Number.MAX_SAFE_INTEGER is up to 2^53-1 before losing precision. Iirc the implementation of read performs position |= 0 so it's only 2^31 or 2^32; but internally displays as -1. Mostly asking for full support of 64 and 128 bit file systems at this point.

$ node
> let v = Number.MAX_SAFE_INTEGER
undefined
> v |= 0
-1
> v >>> 0
4294967295
>

[ZaneHannanAU]

Not sure if that's only for fs.promises or fs.(read|write) tho

[targos]

AFAICT position fully supports numbers up to 2^53-1 (both in fs and fs.promises).

See how it's passed to the C++ layer:

node/lib/internal/fs/promises.js

Lines 219 to 223 in afc5636

	if (!Number.isSafeInteger(position))
	position = -1;
	const bytesRead = (await binding.read(handle.fd, buffer, offset, length,
	position, kUsePromises)) || 0;

Then in C++ the number is read as a 64 bit value:

node/src/node_file.cc

Lines 1627 to 1628 in afc5636

	CHECK(args[4]->IsNumber());
	const int64_t pos = args[4].As<Integer>()->Value();

[targos]

We can probably easily extend the API to support bigints up to 2^64-1.

To go beyond that, we would need libuv to somehow support bigger offsets.

[targos]

/cc @nodejs/fs

[ZaneHannanAU]

Ah; I see now.

Basically cast the postion to a bigint (eg BigInt.asUintN(64, BigInt(position))) and update the binding to use isBigint() alike?

Other than that you could run a check that position === BigInt.asUintN(64, BigInt(position)) for preflight as well.

If it's that simple then even I could write up a PR. I mostly just needed to know it I guess.

Yeah; if libuv supported bigger offsets it'd help.

Thank you very much!

[targos]

I think it would be better to handle this separately from regular numbers.
Basically:

• In JS, if position is a bigint, check that the value is smaller than 2n ** 64n.
  • If it's not, throw an out of range error
  • If it is, let it be passed to the binding
• In the C++ binding, if the value is a bigint, extract it to an int64_t, otherwise do the same as before.

Feel free to create a PR and mention me there :)