From 8744b3aa4eefc2769be624e4d92029bb9080c0b4 Mon Sep 17 00:00:00 2001 From: Brian Gough Date: Wed, 7 Nov 2018 16:23:55 +0000 Subject: [PATCH] serve file from disk to avoid read-after-write inconsistency --- .../filestore/app/coffee/FileHandler.coffee | 19 ++++++++++++++++++- .../app/coffee/LocalFileWriter.coffee | 17 +++++++++++++++++ .../test/unit/coffee/FileHandlerTests.coffee | 10 +++++++--- .../unit/coffee/LocalFileWriterTests.coffee | 15 +++++++++++++++ 4 files changed, 57 insertions(+), 4 deletions(-) diff --git a/services/filestore/app/coffee/FileHandler.coffee b/services/filestore/app/coffee/FileHandler.coffee index 2eeb09bc74..94c6a7395c 100644 --- a/services/filestore/app/coffee/FileHandler.coffee +++ b/services/filestore/app/coffee/FileHandler.coffee @@ -64,7 +64,24 @@ module.exports = FileHandler = LocalFileWriter.deleteFile convertedFsPath, -> LocalFileWriter.deleteFile originalFsPath, -> return callback(err) - PersistorManager.getFileStream bucket, convertedKey, opts, callback + # Send back the converted file from the local copy to avoid problems + # with the file not being present in S3 yet. As described in the + # documentation below, we have already made a 'HEAD' request in + # checkIfFileExists so we only have "eventual consistency" if we try + # to stream it from S3 here. This was a cause of many 403 errors. + # + # "Amazon S3 provides read-after-write consistency for PUTS of new + # objects in your S3 bucket in all regions with one caveat. The + # caveat is that if you make a HEAD or GET request to the key name + # (to find if the object exists) before creating the object, Amazon + # S3 provides eventual consistency for read-after-write."" + # https://docs.aws.amazon.com/AmazonS3/latest/dev/Introduction.html#ConsistencyModel + LocalFileWriter.getStream convertedFsPath, (err, readStream) -> + return callback(err) if err? + readStream.on 'end', () -> + logger.log {convertedFsPath: convertedFsPath}, "deleting temporary file" + LocalFileWriter.deleteFile convertedFsPath, -> + callback(null, readStream) _convertFile: (bucket, originalKey, opts, callback)-> @_writeS3FileToDisk bucket, originalKey, opts, (err, originalFsPath)-> diff --git a/services/filestore/app/coffee/LocalFileWriter.coffee b/services/filestore/app/coffee/LocalFileWriter.coffee index 47b2b91e77..72422b7696 100644 --- a/services/filestore/app/coffee/LocalFileWriter.coffee +++ b/services/filestore/app/coffee/LocalFileWriter.coffee @@ -5,6 +5,7 @@ _ = require("underscore") logger = require("logger-sharelatex") metrics = require("metrics-sharelatex") Settings = require("settings-sharelatex") +Errors = require "./Errors" module.exports = @@ -26,6 +27,22 @@ module.exports = callback err stream.pipe writeStream + getStream: (fsPath, _callback = (err, res)->) -> + callback = _.once _callback + timer = new metrics.Timer("readingFile") + logger.log fsPath:fsPath, "reading file locally" + readStream = fs.createReadStream(fsPath) + readStream.on "end", -> + timer.done() + logger.log fsPath:fsPath, "finished reading file locally" + readStream.on "error", (err)-> + logger.err err:err, fsPath:fsPath, "problem reading file locally, with read stream" + if err.code == 'ENOENT' + callback new Errors.NotFoundError(err.message), null + else + callback err + callback null, readStream + deleteFile: (fsPath, callback)-> if !fsPath? or fsPath == "" return callback() diff --git a/services/filestore/test/unit/coffee/FileHandlerTests.coffee b/services/filestore/test/unit/coffee/FileHandlerTests.coffee index ab757b9360..50b8a17524 100644 --- a/services/filestore/test/unit/coffee/FileHandlerTests.coffee +++ b/services/filestore/test/unit/coffee/FileHandlerTests.coffee @@ -23,6 +23,7 @@ describe "FileHandler", -> directorySize: sinon.stub() @LocalFileWriter = writeStream: sinon.stub() + getStream: sinon.stub() deleteFile: sinon.stub() @FileConverter = convert: sinon.stub() @@ -152,17 +153,20 @@ describe "FileHandler", -> it "should _convertFile ", (done)-> @stubbedStream = {"something":"here"} + @localStream = { + on: -> + } @PersistorManager.sendFile = sinon.stub().callsArgWith(3) - @PersistorManager.getFileStream = sinon.stub().callsArgWith(3, null, @stubbedStream) + @LocalFileWriter.getStream = sinon.stub().callsArgWith(1, null, @localStream) @convetedKey = @key+"converted" @handler._convertFile = sinon.stub().callsArgWith(3, null, @stubbedPath) @ImageOptimiser.compressPng = sinon.stub().callsArgWith(1) @handler._getConvertedFileAndCache @bucket, @key, @convetedKey, {}, (err, fsStream)=> @handler._convertFile.called.should.equal true @PersistorManager.sendFile.calledWith(@bucket, @convetedKey, @stubbedPath).should.equal true - @PersistorManager.getFileStream.calledWith(@bucket, @convetedKey).should.equal true @ImageOptimiser.compressPng.calledWith(@stubbedPath).should.equal true - fsStream.should.equal @stubbedStream + @LocalFileWriter.getStream.calledWith(@stubbedPath).should.equal true + fsStream.should.equal @localStream done() describe "_convertFile", -> diff --git a/services/filestore/test/unit/coffee/LocalFileWriterTests.coffee b/services/filestore/test/unit/coffee/LocalFileWriterTests.coffee index 0b9eec035e..a6bc964e0f 100644 --- a/services/filestore/test/unit/coffee/LocalFileWriterTests.coffee +++ b/services/filestore/test/unit/coffee/LocalFileWriterTests.coffee @@ -15,8 +15,11 @@ describe "LocalFileWriter", -> on: (type, cb)-> if type == "finish" cb() + @readStream = + on: -> @fs = createWriteStream : sinon.stub().returns(@writeStream) + createReadStream: sinon.stub().returns(@readStream) unlink: sinon.stub() @settings = path: @@ -51,6 +54,18 @@ describe "LocalFileWriter", -> fsPath.should.equal @stubbedFsPath done() + describe "getStream", -> + + it "should read the stream from the file ", (done)-> + @writer.getStream @stubbedFsPath, (err, stream)=> + @fs.createReadStream.calledWith(@stubbedFsPath).should.equal true + done() + + it "should send the stream in the callback", (done)-> + @writer.getStream @stubbedFsPath, (err, readStream)=> + readStream.should.equal @readStream + done() + describe "delete file", -> it "should unlink the file", (done)->