Merge pull request #35 from sharelatex/bg-serve-converted-file-from-disk

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

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

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

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

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