Retain buffers in fs.read/write()

Closes GH-814.
Closes GH-827.

diff --git a/lib/fs.js b/lib/fs.js
index 8d42c45..5329290 100644 (file)
--- a/lib/fs.js
+++ b/lib/fs.js
@@ -240,7 +240,12 @@ fs.read = function(fd, buffer, offset, length, position, callback) {
     };
   }
 
-  binding.read(fd, buffer, offset, length, position, callback || noop);
+  function wrapper(err, bytesRead) {
+    // Retain a reference to buffer so that it can't be GC'ed too soon.
+    callback && callback(err, bytesRead || 0, buffer);
+  }
+
+  binding.read(fd, buffer, offset, length, position, wrapper);
 };
 
 fs.readSync = function(fd, buffer, offset, length, position) {
@@ -285,7 +290,12 @@ fs.write = function(fd, buffer, offset, length, position, callback) {
     return;
   }
 
-  binding.write(fd, buffer, offset, length, position, callback || noop);
+  function wrapper(err, written) {
+    // Retain a reference to buffer so that it can't be GC'ed too soon.
+    callback && callback(err, written || 0, buffer);
+  }
+
+  binding.write(fd, buffer, offset, length, position, wrapper);
 };
 
 fs.writeSync = function(fd, buffer, offset, length, position) {

diff --git a/src/node_file.cc b/src/node_file.cc
index 58ed023..a0d7849 100644 (file)
--- a/src/node_file.cc
+++ b/src/node_file.cc
@@ -707,9 +707,6 @@ 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 {
@@ -775,10 +772,6 @@ 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 {

diff --git a/test/common.js b/test/common.js
index bf0a43f..04cd6fe 100644 (file)
--- a/test/common.js
+++ b/test/common.js
@@ -60,6 +60,10 @@ process.on('exit', function() {
                       process,
                       global];
 
+  if (global.gc) {
+    knownGlobals.push(gc);
+  }
+
   if (global.DTRACE_HTTP_SERVER_RESPONSE) {
     knownGlobals.push(DTRACE_HTTP_SERVER_RESPONSE);
     knownGlobals.push(DTRACE_HTTP_SERVER_REQUEST);

diff --git a/test/pummel/test-regress-GH-814.js b/test/pummel/test-regress-GH-814.js
new file mode 100644 (file)
index 0000000..2018396
--- /dev/null
+++ b/test/pummel/test-regress-GH-814.js
@@ -0,0 +1,71 @@
+// Flags: --expose_gc
+
+function newBuffer(size, value) {
+  var buffer = new Buffer(size);
+  while (size--) {
+    buffer[size] = value;
+  }
+  //buffer[buffer.length-2]= 0x0d;
+  buffer[buffer.length - 1] = 0x0a;
+  return buffer;
+}
+
+
+var common = require('../common');
+var fs = require('fs');
+var testFileName = require('path').join(common.tmpDir, 'GH-814_testFile.txt');
+var testFileFD = fs.openSync(testFileName, 'w');
+console.log(testFileName);
+
+
+
+var kBufSize = 128 * 1024;
+var PASS = true;
+var neverWrittenBuffer = newBuffer(kBufSize, 0x2e); //0x2e === '.'
+var bufPool = [];
+
+
+
+var tail = require('child_process').spawn('tail', ['-f', testFileName]);
+tail.stdout.on('data', tailCB);
+
+function tailCB(data) {
+  PASS = data.toString().indexOf('.') < 0;
+}
+
+
+
+var timeToQuit = Date.now() + 8e3; //Test during no more than this seconds.
+(function main() {
+
+  if (PASS) {
+    fs.write(testFileFD, newBuffer(kBufSize, 0x61), 0, kBufSize, -1, cb);
+    gc();
+    var nuBuf = new Buffer(kBufSize);
+    neverWrittenBuffer.copy(nuBuf);
+    if (bufPool.push(nuBuf) > 100) {
+      bufPool.length = 0;
+    }
+  }
+  else {
+    throw Error("Buffer GC'ed test -> FAIL");
+  }
+
+  if (Date.now() < timeToQuit) {
+    process.nextTick(main);
+  }
+  else {
+    tail.kill();
+    console.log("Buffer GC'ed test -> PASS (OK)");
+  }
+
+})();
+
+
+function cb(err, written) {
+  if (err) {
+    throw err;
+  }
+}
+
+

diff --git a/test/pummel/test-regress-GH-814_2.js b/test/pummel/test-regress-GH-814_2.js
new file mode 100644 (file)
index 0000000..7443e4f
--- /dev/null
+++ b/test/pummel/test-regress-GH-814_2.js
@@ -0,0 +1,85 @@
+// Flags: --expose_gc
+
+var common = require('../common');
+
+var fs = require('fs');
+var testFileName = require('path').join(common.tmpDir, 'GH-814_test.txt');
+var testFD = fs.openSync(testFileName, 'w');
+console.error(testFileName + '\n');
+
+
+var tailProc = require('child_process').spawn('tail', ['-f', testFileName]);
+tailProc.stdout.on('data', tailCB);
+
+function tailCB(data) {
+  PASS = data.toString().indexOf('.') < 0;
+
+  if (PASS) {
+    //console.error('i');
+  } else {
+    console.error('[FAIL]\n DATA -> ');
+    console.error(data);
+    console.error('\n');
+    throw Error('Buffers GC test -> FAIL');
+  }
+}
+
+
+var PASS = true;
+var bufPool = [];
+var kBufSize = 16 * 1024 * 1024;
+var neverWrittenBuffer = newBuffer(kBufSize, 0x2e); //0x2e === '.'
+
+var timeToQuit = Date.now() + 5e3; //Test should last no more than this.
+writer();
+
+function writer() {
+
+  if (PASS) {
+    if (Date.now() > timeToQuit) {
+      setTimeout(function() {
+        process.kill(tailProc.pid);
+        console.error('\nBuffers GC test -> PASS (OK)\n');
+      }, 555);
+    } else {
+      fs.write(testFD, newBuffer(kBufSize, 0x61), 0, kBufSize, -1, writerCB);
+      gc();
+      gc();
+      gc();
+      gc();
+      gc();
+      gc();
+      var nuBuf = new Buffer(kBufSize);
+      neverWrittenBuffer.copy(nuBuf);
+      if (bufPool.push(nuBuf) > 100) {
+        bufPool.length = 0;
+      }
+      process.nextTick(writer);
+      //console.error('o');
+    }
+  }
+
+}
+
+function writerCB(err, written) {
+  //console.error('cb.');
+  if (err) {
+    throw err;
+  }
+}
+
+
+
+
+// ******************* UTILITIES
+
+
+function newBuffer(size, value) {
+  var buffer = new Buffer(size);
+  while (size--) {
+    buffer[size] = value;
+  }
+  buffer[buffer.length - 1] = 0x0d;
+  buffer[buffer.length - 1] = 0x0a;
+  return buffer;
+}