From 40ac59090bfba92d95ec55e03f983b3c6bd39514 Mon Sep 17 00:00:00 2001 From: Brian Gough Date: Wed, 7 Nov 2018 16:22:09 +0000 Subject: [PATCH 1/2] make backends consistent by deleting temporary files --- .../app/coffee/FSPersistorManager.coffee | 20 +++++++++++++++---- .../app/coffee/S3PersistorManager.coffee | 10 ++++++---- 2 files changed, 22 insertions(+), 8 deletions(-) diff --git a/services/filestore/app/coffee/FSPersistorManager.coffee b/services/filestore/app/coffee/FSPersistorManager.coffee index 0868216a15..733202e4cd 100644 --- a/services/filestore/app/coffee/FSPersistorManager.coffee +++ b/services/filestore/app/coffee/FSPersistorManager.coffee @@ -13,10 +13,19 @@ module.exports = sendFile: ( location, target, source, callback = (err)->) -> filteredTarget = filterName target logger.log location:location, target:filteredTarget, source:source, "sending file" - fs.rename source, "#{location}/#{filteredTarget}", (err) -> - if err!=null + done = _.once (err) -> + if err? logger.err err:err, location:location, target:filteredTarget, source:source, "Error on put of file" - callback err + callback(err) + # actually copy the file (instead of moving it) to maintain consistent behaviour + # between the different implementations + sourceStream = fs.createReadStream source + sourceStream.on 'error', done + targetStream = fs.createWriteStream "#{location}/#{filteredTarget}" + targetStream.on 'error', done + targetStream.on 'finish', () -> + done() + sourceStream.pipe targetStream sendStream: ( location, target, sourceStream, callback = (err)->) -> logger.log location:location, target:target, "sending file stream" @@ -26,7 +35,10 @@ module.exports = if err? logger.err location:location, target:target, fsPath:fsPath, err:err, "something went wrong writing stream to disk" return callback err - @sendFile location, target, fsPath, callback + @sendFile location, target, fsPath, (err) -> + # delete the temporary file created above and return the original error + LocalFileWriter.deleteFile fsPath, () -> + callback(err) # opts may be {start: Number, end: Number} getFileStream: (location, name, opts, _callback = (err, res)->) -> diff --git a/services/filestore/app/coffee/S3PersistorManager.coffee b/services/filestore/app/coffee/S3PersistorManager.coffee index 2bd6eb0e9b..3055810131 100644 --- a/services/filestore/app/coffee/S3PersistorManager.coffee +++ b/services/filestore/app/coffee/S3PersistorManager.coffee @@ -42,9 +42,8 @@ module.exports = if res.statusCode != 200 logger.err bucketName:bucketName, key:key, fsPath:fsPath, "non 200 response from s3 putting file" return callback("non 200 response from s3 on put file") - LocalFileWriter.deleteFile fsPath, (err)-> - logger.log res:res, bucketName:bucketName, key:key, fsPath:fsPath,"file uploaded to s3" - callback(err) + logger.log res:res, bucketName:bucketName, key:key, fsPath:fsPath,"file uploaded to s3" + callback(err) putEventEmiter.on "error", (err)-> logger.err err:err, bucketName:bucketName, key:key, fsPath:fsPath, "error emmited on put of file" callback err @@ -57,7 +56,10 @@ module.exports = if err? logger.err bucketName:bucketName, key:key, fsPath:fsPath, err:err, "something went wrong writing stream to disk" return callback(err) - @sendFile bucketName, key, fsPath, callback + @sendFile bucketName, key, fsPath, (err) -> + # delete the temporary file created above and return the original error + LocalFileWriter.deleteFile fsPath, () -> + callback(err) # opts may be {start: Number, end: Number} getFileStream: (bucketName, key, opts, callback = (err, res)->)-> From 78ccf453dbb308ab2ad636c1e9e4582415b01460 Mon Sep 17 00:00:00 2001 From: Brian Gough Date: Wed, 7 Nov 2018 16:51:06 +0000 Subject: [PATCH 2/2] update unit tests --- .../coffee/FSPersistorManagerTests.coffee | 30 +++++++++++++++++-- 1 file changed, 27 insertions(+), 3 deletions(-) diff --git a/services/filestore/test/unit/coffee/FSPersistorManagerTests.coffee b/services/filestore/test/unit/coffee/FSPersistorManagerTests.coffee index 46def22a05..f762b7a89f 100644 --- a/services/filestore/test/unit/coffee/FSPersistorManagerTests.coffee +++ b/services/filestore/test/unit/coffee/FSPersistorManagerTests.coffee @@ -25,6 +25,7 @@ describe "FSPersistorManagerTests", -> @Rimraf = sinon.stub() @LocalFileWriter = writeStream: sinon.stub() + deleteFile: sinon.stub() @requires = "./LocalFileWriter":@LocalFileWriter "fs":@Fs @@ -43,10 +44,32 @@ describe "FSPersistorManagerTests", -> @FSPersistorManager = SandboxedModule.require modulePath, requires: @requires describe "sendFile", -> - it "should put the file", (done) -> - @Fs.rename.callsArgWith(2,@error) + beforeEach -> + @Fs.createReadStream = sinon.stub().returns({ + on: -> + pipe: -> + }) + + it "should copy the file", (done) -> + @Fs.createWriteStream =sinon.stub().returns({ + on: (event, handler) -> + process.nextTick(handler) if event is 'finish' + }) @FSPersistorManager.sendFile @location, @name1, @name2, (err)=> - @Fs.rename.calledWith( @name2, "#{@location}/#{@name1Filtered}" ).should.equal true + @Fs.createReadStream.calledWith(@name2).should.equal true + @Fs.createWriteStream.calledWith("#{@location}/#{@name1Filtered}" ).should.equal true + done() + + it "should return an error if the file cannot be stored", (done) -> + @Fs.createWriteStream =sinon.stub().returns({ + on: (event, handler) => + if event is 'error' + process.nextTick () => + handler(@error) + }) + @FSPersistorManager.sendFile @location, @name1, @name2, (err)=> + @Fs.createReadStream.calledWith(@name2).should.equal true + @Fs.createWriteStream.calledWith("#{@location}/#{@name1Filtered}" ).should.equal true err.should.equal @error done() @@ -54,6 +77,7 @@ describe "FSPersistorManagerTests", -> beforeEach -> @FSPersistorManager.sendFile = sinon.stub().callsArgWith(3) @LocalFileWriter.writeStream.callsArgWith(2, null, @name1) + @LocalFileWriter.deleteFile.callsArg(1) @SourceStream = on:->