fs: read whole file only in regular files #1074
Conversation
santigimeno
commented
Mar 5, 2015
santigimeno
commented
- Trying to fix bug fs.readFile[Sync]("/dev/stdin") does not always read the entire file #1070. Tests still pass in my machine but I haven't been able to reproduce the original bug so I don't know it this fixes anything at all.
|
Finally I could reproduce it in a FreeBSD machine. |
|
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? |
|
I think this is what we usually do in such cases. |
|
Updated. Now the file is created inside the tests |
|
rebased to latest v1.x and reworded the commit log |
|
@santigimeno |
|
@piscisaureus That test was relying on |
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); |
There was a problem hiding this comment.
this should be st = fs.fstatSync(fd);
- Using st_size to read other kind of files can lead to not reading all the data.
|
@piscisaureus You're right. Originally I was using PR updated to use fstatSync and a fix to |
PR-URL: #1074 Reviewed-By: Bert Belder <[email protected]>
Using st_size to read non-regular files can lead to not reading all the data. PR-URL: #1074 Reviewed-By: Bert Belder <[email protected]>
PR-URL: #1074 Reviewed-By: Bert Belder <[email protected]>
|
Thanks @santigimeno. Landed in iojs:e2c9040...iojs:5ecdc03 |
SmartOS issue: TritonDataCenter/smartos-live#753 Node.js issue: nodejs/node#1074 and nodejs/node-v0.x-archive#7412 Patch from: nodejs/node-v0.x-archive@a6af709
