Skip to content
This repository has been archived by the owner on Apr 22, 2023. It is now read-only.

Fix fs can't handle large file on 64bit platform #1199

Closed
wants to merge 3 commits into from
Closed

Fix fs can't handle large file on 64bit platform #1199

wants to merge 3 commits into from

Conversation

koichik
Copy link

@koichik koichik commented Jun 18, 2011

fs.read() and fs.write() can't handle more than 2GB files on 64bit platform.
Also fs.truncate() can't handle more than 4GB files.

Node already has the capability to handle large files.
This patch changes only a parameter check (read/write) and
type conversion from JS's number to C types (truncate).

The testcase only works on 64bit platform, then I put it into test/disabled.

koichik and others added 3 commits June 18, 2011 16:25
fs.read() and fs.write() can't handle more than 2GB files on 64bit platform.
Also fs.truncate() can't handle more than 4GB files.

Node already has the capability to handle large files.
This patch changes only a parameter check (read/write) and
type conversion from JS's number to C types (truncate).

The testcase only works on 64bit platform, then I put it into test/disabled.
Thanks bnoordhuis (Ben Noordhuis)
@tshinnic
Copy link

Just a note for a future reader...

If you want to return a large integer value from .cc to JS you can't use the commonly-seen method of returning integer values,

    return scope.Close(Integer::New(offs_result));

This will only handle signed 32 bit integer values. Even the added Integer::NewFromUnsigned() will only make unsigned full 32 bit values.

You must switch to using Number::New() to create integer values more than 32 bits. I don't know how large the values can be before truncation occurs, but this is working for me now up past 43 bits:

    return scope.Close(Number::New(offs_result));

@koichik
Copy link
Author

koichik commented Jun 23, 2011

Hi, tshinnic.

Because the Write()/Read()'s length is singed int32, they does not return a number bigger than signed int32.
https://github.com/joyent/node/blob/feb26d6c742980b660efc988133640cf14fc67d3/src/node_file.cc#L698
https://github.com/joyent/node/blob/feb26d6c742980b660efc988133640cf14fc67d3/src/node_file.cc#L762

Also the Buffer size is up to signed int32, may be.
https://github.com/joyent/node/blob/5e409c2f1aa94b76f42e58001f7fcd0959a958aa/src/node_buffer.cc#L178

Then I think using Number::New is not a must in this issue.
I agree that we will be necessary when Node supported large Buffer in the future.

@tshinnic
Copy link

Agreed, not needed here

I thought to add the note here because I was using your code to update another module for position value input arguments:
baudehlo/node-fs-ext@3ac09b0
and because there I had to also output position values. If people want to know how to do both, here are both answers! :-)

@japj
Copy link

japj commented Jul 7, 2011

can this be closed?

@koichik
Copy link
Author

koichik commented Jul 8, 2011

I am going to push this myself :-)
Because v0.5 was released, I merged it into the master (ab68f39).
Please review.

@koichik koichik closed this in a3e3ad4 Jul 13, 2011
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants