Skip to content

Commit cea3a95

Browse files
committed
Add ref to buffer during fs.write and fs.read
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
1 parent cf05257 commit cea3a95

File tree

2 files changed

+61
-0
lines changed

2 files changed

+61
-0
lines changed

src/node_file.cc

+11
Original file line numberDiff line numberDiff line change
@@ -31,6 +31,7 @@ using namespace v8;
3131
ThrowException(Exception::TypeError(String::New("Bad argument")))
3232
static Persistent<String> encoding_symbol;
3333
static Persistent<String> errno_symbol;
34+
static Persistent<String> buf_symbol;
3435

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

640641
if (cb->IsFunction()) {
642+
// Grab a reference to buffer so it isn't GCed
643+
Local<Object> cb_obj = cb->ToObject();
644+
cb_obj->Set(buf_symbol, buffer_obj);
645+
641646
ASYNC_CALL(write, cb, fd, buf, len, pos)
642647
} else {
643648
ssize_t written = pos < 0 ? write(fd, buf, len) : pwrite(fd, buf, len, pos);
@@ -702,6 +707,11 @@ static Handle<Value> Read(const Arguments& args) {
702707
cb = args[5];
703708

704709
if (cb->IsFunction()) {
710+
// Grab a reference to buffer so it isn't GCed
711+
// TODO: need test coverage
712+
Local<Object> cb_obj = cb->ToObject();
713+
cb_obj->Set(buf_symbol, buffer_obj);
714+
705715
ASYNC_CALL(read, cb, fd, buf, len, pos);
706716
} else {
707717
// SYNC
@@ -792,6 +802,7 @@ void File::Initialize(Handle<Object> target) {
792802

793803
errno_symbol = NODE_PSYMBOL("errno");
794804
encoding_symbol = NODE_PSYMBOL("node:encoding");
805+
buf_symbol = NODE_PSYMBOL("__buf");
795806
}
796807

797808
void InitFs(Handle<Object> target) {
+50
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,50 @@
1+
var common = require('../common');
2+
var fs = require("fs");
3+
var assert = require("assert");
4+
var join = require('path').join;
5+
6+
var filename = join(common.tmpDir, 'out.txt');
7+
8+
try {
9+
fs.unlinkSync(filename);
10+
} catch (e) {
11+
// might not exist, that's okay.
12+
}
13+
14+
var fd = fs.openSync(filename, "w");
15+
16+
var line = "aaaaaaaaaaaaaaaaaaaaaaaaaaaa\n";
17+
18+
var N = 10240, complete = 0;
19+
for (var i = 0; i < N; i ++) {
20+
// Create a new buffer for each write. Before the write is actually
21+
// executed by the thread pool, the buffer will be collected.
22+
var buffer = new Buffer(line);
23+
fs.write(fd, buffer, 0, buffer.length, null, function (er, written) {
24+
complete++;
25+
if (complete === N) {
26+
fs.closeSync(fd);
27+
var s = fs.createReadStream(filename);
28+
s.on("data", testBuffer);
29+
}
30+
});
31+
}
32+
33+
var bytesChecked = 0;
34+
35+
function testBuffer (b) {
36+
for (var i = 0; i < b.length; i++) {
37+
bytesChecked++;
38+
if (b[i] !== 'a'.charCodeAt(0) && b[i] !== '\n'.charCodeAt(0)) {
39+
throw new Error("invalid char "+i+","+b[i]);
40+
}
41+
}
42+
}
43+
44+
process.on('exit', function () {
45+
// Probably some of the writes are going to overlap, so we can't assume
46+
// that we get (N * line.length). Let's just make sure we've checked a
47+
// few...
48+
assert.ok(bytesChecked > 1000);
49+
});
50+

0 commit comments

Comments
 (0)