Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fs: read whole file only in regular files #1074

Closed
wants to merge 3 commits into from
Closed

fs: read whole file only in regular files #1074

wants to merge 3 commits into from

Conversation

santigimeno
Copy link
Member

@santigimeno
Copy link
Member Author

Finally I could reproduce it in a FreeBSD machine.
Force pushed with fixes for readFile and readFileSync and a couple of tests

@Fishrock123
Copy link
Contributor

r=@piscisaureus?

@piscisaureus
Copy link
Contributor

I don't like landing a 100.000 line bogus text file.
Other than that, lgtm, although I'd prefer another one @iojs/collaborators to review this patch too.

@Fishrock123
Copy link
Contributor

I don't like landing a 100.000 line bogus text file.

Yeah, I didn't really like that either. Maybe we can just generate one in the tests?

@indutny
Copy link
Member

indutny commented Mar 6, 2015

I think this is what we usually do in such cases.

@santigimeno
Copy link
Member Author

Updated. Now the file is created inside the tests

@santigimeno
Copy link
Member Author

rebased to latest v1.x and reworded the commit log

@piscisaureus
Copy link
Contributor

@piscisaureus
Copy link
Contributor

@santigimeno
The PR looks good to me.
I have one more question though: why is that change to test-fs-sync-fd-leak necessary?

@santigimeno
Copy link
Member Author

@piscisaureus That test was relying on readFileSync to call fstatSync but after the changes it calls statSync

@piscisaureus
Copy link
Contributor

@santigimeno

That test was relying on readFileSync to call fstatSync but after the changes it calls statSync

I missed that, but I also do not agree. By changing to statSync() the operation is now racy, e.g. the file that gets stat()ed might be another file than the one that is opened. I don't think there is a good reason to switch to statSync, so preferrably just use fstatSync.

var size;
var threw = true;
try {
size = fs.fstatSync(fd).size;
st = fs.statSync(path);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this should be st = fs.fstatSync(fd);

@santigimeno
Copy link
Member Author

@piscisaureus You're right. Originally I was using fstatSync but I changed to statSync because readfile-zero-byte-liar test was failing and thought there was a problem with fstatSync.

PR updated to use fstatSync and a fix to readfile-zero-byte-liar test

piscisaureus pushed a commit that referenced this pull request Mar 12, 2015
PR-URL: #1074
Reviewed-By: Bert Belder <bertbelder@gmail.com>
piscisaureus pushed a commit that referenced this pull request Mar 12, 2015
Using st_size to read non-regular files can lead to not reading all the
data.

PR-URL: #1074
Reviewed-By: Bert Belder <bertbelder@gmail.com>
piscisaureus pushed a commit that referenced this pull request Mar 12, 2015
PR-URL: #1074
Reviewed-By: Bert Belder <bertbelder@gmail.com>
@piscisaureus
Copy link
Contributor

Thanks @santigimeno. Landed in iojs:e2c9040...iojs:5ecdc03

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants