From 6baedf909ddefc1a0102abfc0471edd565497b2a Mon Sep 17 00:00:00 2001 From: Jim Turner Date: Fri, 17 Nov 2017 09:29:33 -0800 Subject: [PATCH] Fix #1478 - Project files are not being flushed to disk after being written (#1479) * Call fsync() before closing file * Fix race condition in tests due to incorrect stub. The startFlows() function wasn't really being stubbed, so it was still being called. But there was no corresponding call to stopFlows(). In later tests, the check in Flows.init() was throwing the "Cannot init without a stop" error. * Test coverage for fsync() calls For issue #1478 * Revert "Fix race condition in tests due to incorrect stub." This reverts commit 4f71d7851bfc7e948a037c43f4b070f80c10d65c. * Fix race condition in tests due to incorrect stub. The startFlows() function wasn't really being stubbed, so it was still being called. But there was no corresponding call to stopFlows(). In later tests, the check in Flows.init() was throwing the "Cannot init without a stop" error. * Fix intermittent test failure in Exec node. Occasionally, the error text on stderr will come in more than one piece. The test only worked correctly if a single message was received. --- red/runtime/locales/en-US/runtime.json | 3 +- red/runtime/storage/localfilesystem.js | 9 +++- test/nodes/core/core/75-exec_spec.js | 9 +++- test/red/runtime/nodes/index_spec.js | 4 +- .../runtime/storage/localfilesystem_spec.js | 41 +++++++++++++++++++ 5 files changed, 60 insertions(+), 6 deletions(-) diff --git a/red/runtime/locales/en-US/runtime.json b/red/runtime/locales/en-US/runtime.json index 37c82dc14..168e461ba 100644 --- a/red/runtime/locales/en-US/runtime.json +++ b/red/runtime/locales/en-US/runtime.json @@ -137,7 +137,8 @@ "empty": "Existing __type__ file is empty", "invalid": "Existing __type__ file is not valid json", "restore": "Restoring __type__ file backup : __path__", - "restore-fail": "Restoring __type__ file backup failed : __message__" + "restore-fail": "Restoring __type__ file backup failed : __message__", + "fsync-fail": "Flushing file __path__ to disk failed : __message__" } } } diff --git a/red/runtime/storage/localfilesystem.js b/red/runtime/storage/localfilesystem.js index 32c0415f8..ecb5b55ab 100644 --- a/red/runtime/storage/localfilesystem.js +++ b/red/runtime/storage/localfilesystem.js @@ -114,8 +114,13 @@ function writeFile(path,content) { return when.promise(function(resolve,reject) { var stream = fs.createWriteStream(path); stream.on('open',function(fd) { - stream.end(content,'utf8',function() { - fs.fsync(fd,resolve); + stream.write(content,'utf8',function() { + fs.fsync(fd,function(err) { + if (err) { + log.warn(log._("storage.localfilesystem.fsync-fail",{path: path, message: err.toString()})); + } + stream.end(resolve); + }); }); }); stream.on('error',function(err) { diff --git a/test/nodes/core/core/75-exec_spec.js b/test/nodes/core/core/75-exec_spec.js index c56647bee..28f6eb00f 100644 --- a/test/nodes/core/core/75-exec_spec.js +++ b/test/nodes/core/core/75-exec_spec.js @@ -544,6 +544,7 @@ describe('exec node', function() { it('should return an error for a failing command', function(done) { var flow; var expected; + var expectedFound = false; if (osType === "Windows_NT") { // Cannot use mkdir because Windows mkdir command automatically creates non-existent directories. flow = [{id:"n1",type:"exec",wires:[["n2"],["n3"],["n4"]],command:"ping /foo/bar/doo/dah", addpay:false, append:"", useSpawn:"true", oldrc:"false"}, @@ -563,12 +564,18 @@ describe('exec node', function() { try { msg.should.have.property("payload"); msg.payload.should.be.a.String(); - msg.payload.indexOf(expected).should.not.be.equal(-1); + if (msg.payload.indexOf(expected) >= 0) { + // The error text on the stderr stream might get sent in more than one piece. + // We only need to know that it occurred before the return code is sent, + // as checked below in node n4. + expectedFound = true; + } } catch(err) { done(err); } }); n4.on("input", function(msg) { try { + expectedFound.should.be.true; msg.should.have.property("payload"); msg.payload.should.have.property("code",1); done(); diff --git a/test/red/runtime/nodes/index_spec.js b/test/red/runtime/nodes/index_spec.js index a26474a24..b76378784 100644 --- a/test/red/runtime/nodes/index_spec.js +++ b/test/red/runtime/nodes/index_spec.js @@ -26,11 +26,11 @@ var registry = require("../../../../red/runtime/nodes/registry"); describe("red/nodes/index", function() { before(function() { - sinon.stub(flows,"startFlows"); + sinon.stub(index,"startFlows"); process.env.NODE_RED_HOME = path.resolve(path.join(__dirname,"..","..","..","..")) }); after(function() { - flows.startFlows.restore(); + index.startFlows.restore(); delete process.env.NODE_RED_HOME; }); diff --git a/test/red/runtime/storage/localfilesystem_spec.js b/test/red/runtime/storage/localfilesystem_spec.js index 646ade069..c7f68bbcf 100644 --- a/test/red/runtime/storage/localfilesystem_spec.js +++ b/test/red/runtime/storage/localfilesystem_spec.js @@ -17,8 +17,10 @@ var should = require("should"); var fs = require('fs-extra'); var path = require('path'); +var sinon = require('sinon'); var localfilesystem = require("../../../../red/runtime/storage/localfilesystem"); +var log = require("../../../../red/runtime/log"); describe('LocalFileSystem', function() { var userDir = path.join(__dirname,".testUserHome"); @@ -279,6 +281,45 @@ describe('LocalFileSystem', function() { }); }); + it('should fsync the flows file',function(done) { + var flowFile = 'test.json'; + var flowFilePath = path.join(userDir,flowFile); + localfilesystem.init({userDir:userDir, flowFile:flowFilePath}).then(function() { + sinon.spy(fs,"fsync"); + localfilesystem.saveFlows(testFlow).then(function() { + fs.fsync.callCount.should.eql(1); + fs.fsync.restore(); + done(); + }).otherwise(function(err) { + done(err); + }); + }).otherwise(function(err) { + done(err); + }); + }); + + it('should log fsync errors and continue',function(done) { + var flowFile = 'test.json'; + var flowFilePath = path.join(userDir,flowFile); + localfilesystem.init({userDir:userDir, flowFile:flowFilePath}).then(function() { + sinon.stub(fs,"fsync", function(fd, cb) { + cb(new Error()); + }); + sinon.spy(log,"warn"); + localfilesystem.saveFlows(testFlow).then(function() { + log.warn.callCount.should.eql(1); + log.warn.restore(); + fs.fsync.callCount.should.eql(1); + fs.fsync.restore(); + done(); + }).otherwise(function(err) { + done(err); + }); + }).otherwise(function(err) { + done(err); + }); + }); + it('should backup the flows file', function(done) { var defaultFlowFile = 'flows_'+require('os').hostname()+'.json'; var defaultFlowFilePath = path.join(userDir,defaultFlowFile);