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

[ZaneHannanAU]

... trying to figure out if this'll work...

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

looking at the v8 documentation I don't believe it will but I have no clue.

Currently running ./configure ; make so waiting on that

[joyeecheung]

@ZaneHannanAU I believe what you need is args[4].As<v8::BigInt>()->Uint64Value(lossless) with lossless being a bool *

[ZaneHannanAU]

Thank you!

[ZaneHannanAU]

... getting errors

../src/node_file.cc: In function ‘void node::fs::FTruncate(const v8::FunctionCallbackInfo<v8::Value>&)’:
../src/node_file.cc:1047:66: error: expected primary-expression before ‘)’ token
   const int64_t len = args[1].As<v8::BigInt>()->Uint64Value(true*);
                                                                  ^
../src/node_file.cc: In function ‘void node::fs::WriteBuffer(const v8::FunctionCallbackInfo<v8::Value>&)’:
../src/node_file.cc:1433:66: error: expected primary-expression before ‘)’ token
   const int64_t pos = args[4].As<v8::BigInt>()->Uint64Value(true*);
                                                                  ^
../src/node_file.cc: In function ‘void node::fs::WriteBuffers(const v8::FunctionCallbackInfo<v8::Value>&)’:
../src/node_file.cc:1474:60: error: expected primary-expression before ‘)’ token
   int64_t pos = args[2].As<v8::BigInt>()->Uint64Value(true*);
                                                            ^
../src/node_file.cc: In function ‘void node::fs::WriteString(const v8::FunctionCallbackInfo<v8::Value>&)’:
../src/node_file.cc:1518:66: error: expected primary-expression before ‘)’ token
   const int64_t pos = args[2].As<v8::BigInt>()->Uint64Value(true*);
                                                                  ^
../src/node_file.cc: In function ‘void node::fs::Read(const v8::FunctionCallbackInfo<v8::Value>&)’:
../src/node_file.cc:1632:66: error: expected primary-expression before ‘)’ token
   const int64_t pos = args[4].As<v8::BigInt>()->Uint64Value(true*);

[ZaneHannanAU]

... I'm not sure if it's needed but I guess it might work without it?

[ZaneHannanAU]

went without and instantly crashing

$ ./node
> ./node[24219]: ../src/node_file.cc:1631:void node::fs::Read(const v8::FunctionCallbackInfo<v8::Value>&): Assertion `args[4]->IsBigInt()' failed.
 1: 0x8b9220 node::Abort() [./node]
 2: 0x8b92f5  [./node]
 3: 0x8fbb12  [./node]
 4: 0xb5d809  [./node]
 5: 0xb5e6a4 v8::internal::Builtin_HandleApiCall(int, v8::internal::Object**, v8::internal::Isolate*) [./node]
 6: 0x1a36122dc01d 
Aborted (core dumped)
$ 

[ZaneHannanAU]

... guessing I'll need to check everything huh

[ZaneHannanAU]

okay uh I can't figure out where it's from...

see @ZaneHannanAU/node I guess.

[ZaneHannanAU]

Testing in #22038 but needing help. Thank you.