From e7604b1ea71a761c43bb2a757ae5015e00303349 Mon Sep 17 00:00:00 2001 From: Jorge Chamorro Bieling Date: Wed, 23 Mar 2011 12:29:49 +0100 Subject: [PATCH] Retain buffers in fs.read/write() Closes GH-814. Closes GH-827. --- lib/fs.js | 14 +++++- src/node_file.cc | 7 --- test/common.js | 4 ++ test/pummel/test-regress-GH-814.js | 71 ++++++++++++++++++++++++++++++ test/pummel/test-regress-GH-814_2.js | 85 ++++++++++++++++++++++++++++++++++++ 5 files changed, 172 insertions(+), 9 deletions(-) create mode 100644 test/pummel/test-regress-GH-814.js create mode 100644 test/pummel/test-regress-GH-814_2.js diff --git a/lib/fs.js b/lib/fs.js index 8d42c45..5329290 100644 --- 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 --- a/src/node_file.cc +++ b/src/node_file.cc @@ -707,9 +707,6 @@ static Handle Write(const Arguments& args) { Local cb = args[5]; if (cb->IsFunction()) { - // Grab a reference to buffer so it isn't GCed - Local 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 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 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 --- 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 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 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; +} -- 2.7.4