From 2da01cc611dc3b1d1f06fd378c9e2349c4c52012 Mon Sep 17 00:00:00 2001 From: Issac Goldstand Date: Thu, 9 May 2013 13:09:59 +0300 Subject: [PATCH 1/2] Fixes bug introduced in github issue #132 where renaming a file to itself can cause an unhandled error --- lib/streams/DateRollingFileStream.js | 11 +++++++++-- 1 file changed, 9 insertions(+), 2 deletions(-) diff --git a/lib/streams/DateRollingFileStream.js b/lib/streams/DateRollingFileStream.js index b121f6a..8632f66 100644 --- a/lib/streams/DateRollingFileStream.js +++ b/lib/streams/DateRollingFileStream.js @@ -23,10 +23,12 @@ function DateRollingFileStream(filename, pattern, options, now) { this.pattern = pattern || '.yyyy-MM-dd'; this.now = now || Date.now; this.lastTimeWeWroteSomething = format.asString(this.pattern, new Date(this.now())); + this.alwaysIncludePattern = false; this.baseFilename = filename; if (options) { if (options.alwaysIncludePattern) { + this.alwaysIncludePattern = true; filename = filename + this.lastTimeWeWroteSomething; } delete options.alwaysIncludePattern; @@ -75,8 +77,13 @@ DateRollingFileStream.prototype.roll = function(filename, callback) { } function renameTheCurrentFile(cb) { - debug("Renaming the " + filename + " -> " + newFilename); - fs.rename(filename, newFilename, cb); + if (this.alwaysIncludePattern) { + // no-op + cb(); + } else { + debug("Renaming the " + filename + " -> " + newFilename); + fs.rename(filename, newFilename, cb); + } } }; From dc632f47059988782a65358b4b738f485f8553bc Mon Sep 17 00:00:00 2001 From: Issac Goldstand Date: Sat, 11 May 2013 23:01:28 +0300 Subject: [PATCH 2/2] Fixes bug introduced in github issue #132 where file rolling needs to be handled differently for alwaysIncludePattern streams --- lib/streams/DateRollingFileStream.js | 39 ++++++++------- test/streams/DateRollingFileStream-test.js | 57 ++++++++++++++++++++++ 2 files changed, 78 insertions(+), 18 deletions(-) diff --git a/lib/streams/DateRollingFileStream.js b/lib/streams/DateRollingFileStream.js index 8632f66..69d6668 100644 --- a/lib/streams/DateRollingFileStream.js +++ b/lib/streams/DateRollingFileStream.js @@ -23,13 +23,13 @@ function DateRollingFileStream(filename, pattern, options, now) { this.pattern = pattern || '.yyyy-MM-dd'; this.now = now || Date.now; this.lastTimeWeWroteSomething = format.asString(this.pattern, new Date(this.now())); - this.alwaysIncludePattern = false; this.baseFilename = filename; + this.alwaysIncludePattern = false; if (options) { if (options.alwaysIncludePattern) { this.alwaysIncludePattern = true; - filename = filename + this.lastTimeWeWroteSomething; + filename = this.baseFilename + this.lastTimeWeWroteSomething; } delete options.alwaysIncludePattern; if (Object.keys(options).length === 0) { @@ -55,18 +55,26 @@ DateRollingFileStream.prototype.shouldRoll = function() { }; DateRollingFileStream.prototype.roll = function(filename, callback) { - var that = this, - newFilename = this.baseFilename + this.previousTime; + var that = this; debug("Starting roll"); - async.series([ - this.closeTheStream.bind(this), - deleteAnyExistingFile, - renameTheCurrentFile, - this.openTheStream.bind(this) - ], callback); - + if (this.alwaysIncludePattern) { + this.filename = this.baseFilename + this.lastTimeWeWroteSomething; + async.series([ + this.closeTheStream.bind(this), + this.openTheStream.bind(this) + ], callback); + } else { + var newFilename = this.baseFilename + this.previousTime; + async.series([ + this.closeTheStream.bind(this), + deleteAnyExistingFile, + renameTheCurrentFile, + this.openTheStream.bind(this) + ], callback); + } + function deleteAnyExistingFile(cb) { //on windows, you can get a EEXIST error if you rename a file to an existing file //so, we'll try to delete the file we're renaming to first @@ -77,13 +85,8 @@ DateRollingFileStream.prototype.roll = function(filename, callback) { } function renameTheCurrentFile(cb) { - if (this.alwaysIncludePattern) { - // no-op - cb(); - } else { - debug("Renaming the " + filename + " -> " + newFilename); - fs.rename(filename, newFilename, cb); - } + debug("Renaming the " + filename + " -> " + newFilename); + fs.rename(filename, newFilename, cb); } }; diff --git a/test/streams/DateRollingFileStream-test.js b/test/streams/DateRollingFileStream-test.js index 69e9ba8..a314170 100644 --- a/test/streams/DateRollingFileStream-test.js +++ b/test/streams/DateRollingFileStream-test.js @@ -125,6 +125,63 @@ vows.describe('DateRollingFileStream').addBatch({ } } } + }, + + 'with alwaysIncludePattern': { + topic: function() { + var that = this, + testTime = new Date(2012, 8, 12, 0, 10, 12), + stream = new DateRollingFileStream(__dirname + '/test-date-rolling-file-stream-pattern', '.yyyy-MM-dd', {alwaysIncludePattern: true}, now); + stream.write("First message\n", 'utf8', function() { + that.callback(null, stream); + }); + }, + teardown: cleanUp(__dirname + '/test-date-rolling-file-stream-pattern.2012-09-12'), + + 'should create a file with the pattern set': { + topic: function(stream) { + fs.readFile(__dirname + '/test-date-rolling-file-stream-pattern.2012-09-12', this.callback); + }, + 'file should contain first message': function(result) { + assert.equal(result.toString(), "First message\n"); + } + }, + + 'when the day changes': { + topic: function(stream) { + testTime = new Date(2012, 8, 13, 0, 10, 12); + stream.write("Second message\n", 'utf8', this.callback); + }, + teardown: cleanUp(__dirname + '/test-date-rolling-file-stream-pattern.2012-09-13'), + + + 'the number of files': { + topic: function() { + fs.readdir(__dirname, this.callback); + }, + 'should be two': function(files) { + assert.equal(files.filter(function(file) { return file.indexOf('test-date-rolling-file-stream-pattern') > -1; }).length, 2); + } + }, + + 'the file with the later date': { + topic: function() { + fs.readFile(__dirname + '/test-date-rolling-file-stream-pattern.2012-09-13', this.callback); + }, + 'should contain the second message': function(contents) { + assert.equal(contents.toString(), "Second message\n"); + } + }, + + 'the file with the date': { + topic: function() { + fs.readFile(__dirname + '/test-date-rolling-file-stream-pattern.2012-09-12', this.callback); + }, + 'should contain the first message': function(contents) { + assert.equal(contents.toString(), "First message\n"); + } + } + } } }).exportTo(module);