serve file from disk to avoid read-after-write inconsistency

This commit is contained in:
Brian Gough 2018-11-07 16:23:55 +00:00
parent 47bd6edbc9
commit 8744b3aa4e
4 changed files with 57 additions and 4 deletions

View file

@ -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)->

View file

@ -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()

View file

@ -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", ->

View file

@ -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)->