Skip to content

Commit

Permalink
Add ref to buffer during fs.write and fs.read
Browse files Browse the repository at this point in the history
There was the possibility the buffer could be GCed while the eio_req was
pending.  Still needs test coverage for the fs.read() problem.

See:
http://groups.google.com/group/nodejs/browse_thread/thread/c11f8b683f37cef
  • Loading branch information
ry committed Nov 17, 2010
1 parent cf05257 commit cea3a95
Show file tree
Hide file tree
Showing 2 changed files with 61 additions and 0 deletions.
11 changes: 11 additions & 0 deletions src/node_file.cc
Original file line number Diff line number Diff line change
Expand Up @@ -31,6 +31,7 @@ using namespace v8;
ThrowException(Exception::TypeError(String::New("Bad argument")))
static Persistent<String> encoding_symbol;
static Persistent<String> errno_symbol;
static Persistent<String> buf_symbol;

// Buffer for readlink() and other misc callers; keep this scoped at
// file-level rather than method-level to avoid excess stack usage.
Expand Down Expand Up @@ -638,6 +639,10 @@ static Handle<Value> Write(const Arguments& args) {
Local<Value> cb = args[5];

if (cb->IsFunction()) {
// Grab a reference to buffer so it isn't GCed
Local<Object> cb_obj = cb->ToObject();
cb_obj->Set(buf_symbol, buffer_obj);

ASYNC_CALL(write, cb, fd, buf, len, pos)
} else {
ssize_t written = pos < 0 ? write(fd, buf, len) : pwrite(fd, buf, len, pos);
Expand Down Expand Up @@ -702,6 +707,11 @@ static Handle<Value> Read(const Arguments& args) {
cb = args[5];

if (cb->IsFunction()) {
// Grab a reference to buffer so it isn't GCed
// TODO: need test coverage
Local<Object> cb_obj = cb->ToObject();
cb_obj->Set(buf_symbol, buffer_obj);

ASYNC_CALL(read, cb, fd, buf, len, pos);
} else {
// SYNC
Expand Down Expand Up @@ -792,6 +802,7 @@ void File::Initialize(Handle<Object> target) {

errno_symbol = NODE_PSYMBOL("errno");
encoding_symbol = NODE_PSYMBOL("node:encoding");
buf_symbol = NODE_PSYMBOL("__buf");
}

void InitFs(Handle<Object> target) {
Expand Down
50 changes: 50 additions & 0 deletions test/simple/test-fs-sir-writes-alot.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,50 @@
var common = require('../common');
var fs = require("fs");
var assert = require("assert");
var join = require('path').join;

var filename = join(common.tmpDir, 'out.txt');

try {
fs.unlinkSync(filename);
} catch (e) {
// might not exist, that's okay.
}

var fd = fs.openSync(filename, "w");

var line = "aaaaaaaaaaaaaaaaaaaaaaaaaaaa\n";

var N = 10240, complete = 0;
for (var i = 0; i < N; i ++) {
// Create a new buffer for each write. Before the write is actually
// executed by the thread pool, the buffer will be collected.
var buffer = new Buffer(line);
fs.write(fd, buffer, 0, buffer.length, null, function (er, written) {
complete++;
if (complete === N) {
fs.closeSync(fd);
var s = fs.createReadStream(filename);
s.on("data", testBuffer);
}
});
}

var bytesChecked = 0;

function testBuffer (b) {
for (var i = 0; i < b.length; i++) {
bytesChecked++;
if (b[i] !== 'a'.charCodeAt(0) && b[i] !== '\n'.charCodeAt(0)) {
throw new Error("invalid char "+i+","+b[i]);
}
}
}

process.on('exit', function () {
// Probably some of the writes are going to overlap, so we can't assume
// that we get (N * line.length). Let's just make sure we've checked a
// few...
assert.ok(bytesChecked > 1000);
});

0 comments on commit cea3a95

Please sign in to comment.