GeezerVison is showing github.com — fs.readSync & fs.read position argument does not support BigInt · Issue #36185 · nodejs/node
fs.readSync & fs.read position argument does not support BigInt #36185Description
What steps will reproduce the bug?
readSync(reader, buffer, 0, 12, BigInt(position)) How often does it reproduce? Is there a required condition?
Everytime.
What is the expected behavior?
It should read from the position in the file, and it will if you use Number(position) on a BigInt position.
What do you see instead?
The call reads data into the buffer but it begins at position 0 in the file instead of at the position noted in the BigInt. Activitychanged the title [-]fs.readSync position argument does not support BigInt[/-] [+]fs.readSync & fs.read position argument does not support BigInt[/+] on Nov 20, 2020mikeal commented on Nov 20, 2020 ContributorAuthor More actions benjamingr commented on Nov 20, 2020 This is the same behaviour fs.read and fs.readSync have with anything that is not a Number or fails Number.isSafeInteger.
I think this is a reasonable improvement. Though it might be misleading since people may assume fs.read will work with offsets larger than integers which it doesn't.
I think a fix would be to:
A more elaborate fix would be to try and get reads to work with position > Int32, which would require:
- Going to node_file.cc
static void Read(const FunctionCallbackInfo<Value>& args)
- Remove the
IsSafeJsInt check in line 2039
- Instead of reading the value as Integer in line 2040, check
IsBigInt() and if so read it as a BigInt (an int64 is probably still fine for most cases since a 64 bit offset into a file is 9,223,372,036,854,775,807 bytes big which is still quite theoretical I believe).
- uv_fs_read is already being passed a 64 bit integer so the actual offset itself should work.
BridgeAR commented on Nov 20, 2020 Instead of coercing the value to a number, I'd in fact throw an error. That way there's just a single way to do this and the immediate feedback should be fine to let the user handle the coercing on their own or to use a number instead. Reacted by Momtchil Momtchev darvesh commented on Nov 20, 2020 mikeal commented on Nov 20, 2020 ContributorAuthor More actions
Though it might be misleading since people may assume fs.read will work with offsets larger than integers which it doesn't.
Why you gotta go and make my max file size 9 petabytes! 😭 This is so unreasonable 🤬
j/k Trott commented on Nov 23, 2020 (Removed good first issue because there are at least two open PRs that address this.) Reacted by Benjamin Gruenbaum jasnell commented on Nov 23, 2020 Given that it is perfectly fine to have bigints well within the acceptable range, I'd say that accepting a BigInt should be fine. We can throw a RangeError if too large of a bigint is provided. kaizhu256 commented on Nov 23, 2020 if bigint is allowed, would bigdecimal also be allowed if it hypothetically reaches stage 4?
MetadataMetadataDevelopmentNo branches or pull requestsIssue actions
|