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.ReadFile throws asynchronously with encoding on large enough input #2767

Closed
timoxley opened this issue Sep 9, 2015 · 13 comments
Closed
Labels
fs Issues and PRs related to the fs subsystem / file system.

Comments

@timoxley
Copy link
Contributor

timoxley commented Sep 9, 2015

Given 'utf8' encoding is specified, and a large enough input file, fs.ReadFile will throw an asynchronous error.

Example program

var fs = require('fs');
fs.readFile('data.txt', 'utf8', function(err, data) {
  console.log(err, data && data.length)
})

Input: 268 megabytes of data

Success.

> dd if=/dev/zero of=data.txt  bs=1000000  count=268
> node -e "fs = require('fs'); fs.readFile('data.txt', 'utf8', (err, data) => console.log(err, data && data.length))"
null 268000000

Input: 269 megabytes of data

No Success.
Worse, the error isn't forwarded to the callback – the process is throwing asynchronously.

> dd if=/dev/zero of=data.txt  bs=1000000  count=269
> node -e "fs = require('fs'); fs.readFile('data.txt', 'utf8', (err, data) => console.log(err, data && data.length))"
buffer.js:359
    throw new Error('toString failed');
    ^

Error: toString failed
    at Buffer.toString (buffer.js:359:11)
    at FSReqWrap.readFileAfterClose [as oncomplete] (fs.js:378:21)

The error is not something specific to fs.readFile though, the same error is produced if we toString on the buffer directly, without the 'utf8' parameter, unsurprisingly. The problem is the uncatchable throw.

> node -e "fs = require('fs'); fs.readFile('data.txt', (err, data) => console.log(err, String(data).length))"

While perhaps the throw in the Buffer.prototype.toString makes sense:

Buffer.prototype.toString = function() {
  if (arguments.length === 0) {
    var result = this.utf8Slice(0, this.length);
  } else {
    var result = slowToString.apply(this, arguments);
  }
  if (result === undefined)
    throw new Error('toString failed');
  return result;
};

node/lib/buffer.js

Lines 352 to 361 in f8152df

Buffer.prototype.toString = function() {
if (arguments.length === 0) {
var result = this.utf8Slice(0, this.length);
} else {
var result = slowToString.apply(this, arguments);
}
if (result === undefined)
throw new Error('toString failed');
return result;
};

It does seem like poor behaviour to have uncatchable errors being thrown in core APIs where the user has explicitly attached an errback.

Would a try…catch around the buffer = buffer.toString(context.encoding); in fs.ReadFile be appropriate?

function readFileAfterClose(err) {
  var context = this.context;
  var buffer = null;
  var callback = context.callback;

  if (context.err)
    return callback(context.err);

  if (context.size === 0)
    buffer = Buffer.concat(context.buffers, context.pos);
  else if (context.pos < context.size)
    buffer = context.buffer.slice(0, context.pos);
  else
    buffer = context.buffer;

  // maybe ?
  if (context.encoding) {
    try {
      buffer = buffer.toString(context.encoding); // currently this line is not wrapped in a try/catch
    } catch (e) {
      if (!err) err = e; // which error gets priority?
    }
  }

  callback(err, buffer);
}

node/lib/fs.js

Lines 377 to 378 in f8152df

if (context.encoding)
buffer = buffer.toString(context.encoding);

fs.readFile is understandably hot code, I haven't done benchmarking on the impact of a try…catch here.

Given that there appears to be an upper limit to what can be stringified safely, perhaps the max length could be tested for before attempting the stringification, and a 'this is too big' Error would ideally pop out of the callback.

Suggest at a minimum a more helpful error message "our buffering is good but have you tried the streams?"

@timoxley timoxley changed the title fs.ReadFile throws asynchronously with utf8 encoding on large enough input fs.ReadFile throws asynchronously with encoding on large enough input Sep 9, 2015
@vkurchatkin vkurchatkin added the fs Issues and PRs related to the fs subsystem / file system. label Sep 9, 2015
@targos
Copy link
Member

targos commented Sep 9, 2015

AFAIK V8 can optimize functions with try...catch blocks now. I'm just not sure if it is in 4.5

@YurySolovyov
Copy link

@targos looks like it should

p.s. Sorry it is just ‘for-of’, ‘class’, ‘with’ and computed property names., not try/catch

@bnoordhuis
Copy link
Member

Sorry it is just ‘for-of’, ‘class’, ‘with’ and computed property names., not try/catch

And only with TF (--turbo) but that's disabled by default.

@targos
Copy link
Member

targos commented Sep 9, 2015

@bnoordhuis are you sure?

from node --v8-options:

  --turbo_shipping (enable TurboFan compiler on subset)
        type: bool  default: true
  --turbo_try_catch (enable try-catch support in TurboFan)
        type: bool  default: true

@targos
Copy link
Member

targos commented Sep 9, 2015

depends what is the effect of the --turbo_shipping flag

@bnoordhuis
Copy link
Member

Nothing until you specify --turbo. :-)

(Strictly speaking, --turbo_filter='*' accomplishes the same thing.)

You can check for yourself with --trace_opt --trace_deopt.

@alecrajeev
Copy link

I have this problem too

evanlucas added a commit to evanlucas/node that referenced this issue Oct 23, 2015
In fs.readFile, if an encoding is specified and toString fails, do not
throw an error. Instead, pass the error to the callback.

Fixes: nodejs#2767
PR-URL: nodejs#3485
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Trevor Norris <[email protected]>
@evanlucas
Copy link
Contributor

Fixed in b620790

evanlucas added a commit that referenced this issue Oct 26, 2015
In fs.readFile, if an encoding is specified and toString fails, do not
throw an error. Instead, pass the error to the callback.

Fixes: #2767
PR-URL: #3485
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Trevor Norris <[email protected]>
@ta3pks
Copy link

ta3pks commented Dec 3, 2015

on node 5x get the same error while trying ti use handlebars with readfile

@evanlucas
Copy link
Contributor

@NikosEfthias Can you tell me the specific version and platform you are using?

@ta3pks
Copy link

ta3pks commented Dec 3, 2015

@evanlucas solved the issue the problem was that handlebars couldnt turn buffer into string when i call data.toString() no problem

@girishp15
Copy link

Hi,

I am trying to read codiegniter cache file, In this file data stored in serialize, When i try to read it retrun proper data but some time break serialize string data and throw error. Please check below error :

I have try below code :

fs.watch('../application/cache', function (event, filename) {
    if (filename){
        fs.readFile('../application/cache/'+filename, 'utf8', function (err, data) {
            if (err) throw err;
                        if(typeof data != 'undefined'){
                            console.log("Final Data ============= ",data);
                            if(data && PHPUnserialize.unserialize(data)){
                                var result = PHPUnserialize.unserialize(data);
                                for(var k in result)
                                {
                                    var temp = result[k];
                                    console.log('Result', temp.match_games);
                                    if(typeof temp.match_games != 'undefined')
                                    {
                                        broadCastLiveScore(temp.match_games, temp);
                                    }
                                }
                            }
                        }
        });
    }
    // console.log('filename', filename);
    // Prints: <Buffer ...>
});

and i got below errors :

throw new that.window[type](msg, filename, line);
                           ^

TypeError: Cannot read property 'Error' of undefined

Can you provide me solution, Thanks in Advance

Error Data

@bnoordhuis
Copy link
Member

@girishp15 That looks neither related nor an issue with node.js core. Please don't hijack issues.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
fs Issues and PRs related to the fs subsystem / file system.
Projects
None yet
Development

No branches or pull requests

9 participants