github/overleaf
1
0
Fork 0
mirror of https://github.com/overleaf/overleaf.git synced 2024-11-21 20:47:08 -05:00
Code Issues Projects Releases Packages Wiki Activity

Merge pull request #165 from overleaf/ho-retry-url-downloads

Browse source 
add pipeUrlToFileWithRetry for file downloads
This commit is contained in:
Henry Oswald 2020-05-19 10:25:19 +01:00 committed by GitHub
commit 457b7d6657
5 changed files with 153 additions and 109 deletions
Show stats Download patch file Download diff file Expand all files Collapse all files

2
services/clsi/app/js/UrlCache.js
View file

@ -95,7 +95,7 @@ module.exports = UrlCache = {
}
if (needsDownloading) {
logger.log({ url, lastModified }, 'downloading URL')
return UrlFetcher.pipeUrlToFile(
return UrlFetcher.pipeUrlToFileWithRetry(
url,
UrlCache._cacheFilePathForUrl(project_id, url),
error => {

9
services/clsi/app/js/UrlFetcher.js
View file

@ -12,16 +12,23 @@
* DS207: Consider shorter variations of null checks
* Full docs: https://github.com/decaffeinate/decaffeinate/blob/master/docs/suggestions.md
*/
let UrlFetcher
const request = require('request').defaults({ jar: false })
const fs = require('fs')
const logger = require('logger-sharelatex')
const settings = require('settings-sharelatex')
const URL = require('url')
const async = require('async')
const oneMinute = 60 * 1000
module.exports = UrlFetcher = {
pipeUrlToFileWithRetry(url, filePath, callback) {
const doDownload = function(cb) {
UrlFetcher.pipeUrlToFile(url, filePath, cb)
}
async.retry(3, doDownload, callback)
},
pipeUrlToFile(url, filePath, _callback) {
if (_callback == null) {
_callback = function(error) {}

1
services/clsi/test/acceptance/js/UrlCachingTests.js
View file

@ -11,7 +11,6 @@
* Full docs: https://github.com/decaffeinate/decaffeinate/blob/master/docs/suggestions.md
*/
const Client = require('./helpers/Client')
const request = require('request')
require('chai').should()
const sinon = require('sinon')
const ClsiApp = require('./helpers/ClsiApp')

6
services/clsi/test/unit/js/UrlCacheTests.js
View file

@ -160,7 +160,7 @@ describe('UrlCache', function() {
describe('_ensureUrlIsInCache', function() {
beforeEach(function() {
this.UrlFetcher.pipeUrlToFile = sinon.stub().callsArg(2)
this.UrlFetcher.pipeUrlToFileWithRetry = sinon.stub().callsArg(2)
return (this.UrlCache._updateOrCreateUrlDetails = sinon
.stub()
.callsArg(3))
@ -190,7 +190,7 @@ describe('UrlCache', function() {
})
it('should download the URL to the cache file', function() {
return this.UrlFetcher.pipeUrlToFile
return this.UrlFetcher.pipeUrlToFileWithRetry
.calledWith(
this.url,
this.UrlCache._cacheFilePathForUrl(this.project_id, this.url)
@ -232,7 +232,7 @@ describe('UrlCache', function() {
})
it('should not download the URL to the cache file', function() {
return this.UrlFetcher.pipeUrlToFile.called.should.equal(false)
return this.UrlFetcher.pipeUrlToFileWithRetry.called.should.equal(false)
})
return it('should return the callback with the cache file path', function() {

244
services/clsi/test/unit/js/UrlFetcherTests.js
View file

@ -33,72 +33,64 @@ describe('UrlFetcher', function() {
}
}))
})
it('should turn off the cookie jar in request', function() {
return this.defaults.calledWith({ jar: false }).should.equal(true)
})
describe('rewrite url domain if filestoreDomainOveride is set', function() {
beforeEach(function() {
this.path = '/path/to/file/on/disk'
this.request.get = sinon
.stub()
.returns((this.urlStream = new EventEmitter()))
this.urlStream.pipe = sinon.stub()
this.urlStream.pause = sinon.stub()
this.urlStream.resume = sinon.stub()
this.fs.createWriteStream = sinon
.stub()
.returns((this.fileStream = new EventEmitter()))
return (this.fs.unlink = (file, callback) => callback())
describe('pipeUrlToFileWithRetry', function() {
this.beforeEach(function() {
this.UrlFetcher.pipeUrlToFile = sinon.stub()
})
it('should use the normal domain when override not set', function(done) {
this.UrlFetcher.pipeUrlToFile(this.url, this.path, () => {
this.request.get.args[0][0].url.should.equal(this.url)
return done()
it('should call pipeUrlToFile', function(done) {
this.UrlFetcher.pipeUrlToFile.callsArgWith(2)
this.UrlFetcher.pipeUrlToFileWithRetry(this.url, this.path, err => {
expect(err).to.equal(undefined)
this.UrlFetcher.pipeUrlToFile.called.should.equal(true)
done()
})
this.res = { statusCode: 200 }
this.urlStream.emit('response', this.res)
this.urlStream.emit('end')
return this.fileStream.emit('finish')
})
return it('should use override domain when filestoreDomainOveride is set', function(done) {
this.settings.filestoreDomainOveride = '192.11.11.11'
this.UrlFetcher.pipeUrlToFile(this.url, this.path, () => {
this.request.get.args[0][0].url.should.equal(
'192.11.11.11/file/here?query=string'
)
return done()
it('should call pipeUrlToFile multiple times on error', function(done) {
error = new Error("couldn't download file")
this.UrlFetcher.pipeUrlToFile.callsArgWith(2, error)
this.UrlFetcher.pipeUrlToFileWithRetry(this.url, this.path, err => {
expect(err).to.equal(error)
this.UrlFetcher.pipeUrlToFile.callCount.should.equal(3)
done()
})
})
it('should call pipeUrlToFile twice if only 1 error', function(done) {
this.UrlFetcher.pipeUrlToFile.onCall(0).callsArgWith(2, 'error')
this.UrlFetcher.pipeUrlToFile.onCall(1).callsArgWith(2)
this.UrlFetcher.pipeUrlToFileWithRetry(this.url, this.path, err => {
expect(err).to.equal(undefined)
this.UrlFetcher.pipeUrlToFile.callCount.should.equal(2)
done()
})
this.res = { statusCode: 200 }
this.urlStream.emit('response', this.res)
this.urlStream.emit('end')
return this.fileStream.emit('finish')
})
})
return describe('pipeUrlToFile', function() {
beforeEach(function(done) {
this.path = '/path/to/file/on/disk'
this.request.get = sinon
.stub()
.returns((this.urlStream = new EventEmitter()))
this.urlStream.pipe = sinon.stub()
this.urlStream.pause = sinon.stub()
this.urlStream.resume = sinon.stub()
this.fs.createWriteStream = sinon
.stub()
.returns((this.fileStream = new EventEmitter()))
this.fs.unlink = (file, callback) => callback()
return done()
describe('pipeUrlToFile', function() {
it('should turn off the cookie jar in request', function() {
return this.defaults.calledWith({ jar: false }).should.equal(true)
})
describe('successfully', function() {
beforeEach(function(done) {
describe('rewrite url domain if filestoreDomainOveride is set', function() {
beforeEach(function() {
this.path = '/path/to/file/on/disk'
this.request.get = sinon
.stub()
.returns((this.urlStream = new EventEmitter()))
this.urlStream.pipe = sinon.stub()
this.urlStream.pause = sinon.stub()
this.urlStream.resume = sinon.stub()
this.fs.createWriteStream = sinon
.stub()
.returns((this.fileStream = new EventEmitter()))
return (this.fs.unlink = (file, callback) => callback())
})
it('should use the normal domain when override not set', function(done) {
this.UrlFetcher.pipeUrlToFile(this.url, this.path, () => {
this.callback()
this.request.get.args[0][0].url.should.equal(this.url)
return done()
})
this.res = { statusCode: 200 }
@ -107,67 +99,113 @@ describe('UrlFetcher', function() {
return this.fileStream.emit('finish')
})
it('should request the URL', function() {
return this.request.get
.calledWith(sinon.match({ url: this.url }))
.should.equal(true)
})
it('should open the file for writing', function() {
return this.fs.createWriteStream
.calledWith(this.path)
.should.equal(true)
})
it('should pipe the URL to the file', function() {
return this.urlStream.pipe
.calledWith(this.fileStream)
.should.equal(true)
})
return it('should call the callback', function() {
return this.callback.called.should.equal(true)
})
})
describe('with non success status code', function() {
beforeEach(function(done) {
this.UrlFetcher.pipeUrlToFile(this.url, this.path, err => {
this.callback(err)
return it('should use override domain when filestoreDomainOveride is set', function(done) {
this.settings.filestoreDomainOveride = '192.11.11.11'
this.UrlFetcher.pipeUrlToFile(this.url, this.path, () => {
this.request.get.args[0][0].url.should.equal(
'192.11.11.11/file/here?query=string'
)
return done()
})
this.res = { statusCode: 404 }
this.res = { statusCode: 200 }
this.urlStream.emit('response', this.res)
return this.urlStream.emit('end')
})
it('should call the callback with an error', function() {
this.callback.calledWith(sinon.match(Error)).should.equal(true)
const message = this.callback.args[0][0].message
expect(message).to.include('URL returned non-success status code: 404')
this.urlStream.emit('end')
return this.fileStream.emit('finish')
})
})
return describe('with error', function() {
return describe('pipeUrlToFile', function() {
beforeEach(function(done) {
this.UrlFetcher.pipeUrlToFile(this.url, this.path, err => {
this.callback(err)
return done()
this.path = '/path/to/file/on/disk'
this.request.get = sinon
.stub()
.returns((this.urlStream = new EventEmitter()))
this.urlStream.pipe = sinon.stub()
this.urlStream.pause = sinon.stub()
this.urlStream.resume = sinon.stub()
this.fs.createWriteStream = sinon
.stub()
.returns((this.fileStream = new EventEmitter()))
this.fs.unlink = (file, callback) => callback()
return done()
})
describe('successfully', function() {
beforeEach(function(done) {
this.UrlFetcher.pipeUrlToFile(this.url, this.path, () => {
this.callback()
return done()
})
this.res = { statusCode: 200 }
this.urlStream.emit('response', this.res)
this.urlStream.emit('end')
return this.fileStream.emit('finish')
})
it('should request the URL', function() {
return this.request.get
.calledWith(sinon.match({ url: this.url }))
.should.equal(true)
})
it('should open the file for writing', function() {
return this.fs.createWriteStream
.calledWith(this.path)
.should.equal(true)
})
it('should pipe the URL to the file', function() {
return this.urlStream.pipe
.calledWith(this.fileStream)
.should.equal(true)
})
return it('should call the callback', function() {
return this.callback.called.should.equal(true)
})
return this.urlStream.emit(
'error',
(this.error = new Error('something went wrong'))
)
})
it('should call the callback with the error', function() {
return this.callback.calledWith(this.error).should.equal(true)
describe('with non success status code', function() {
beforeEach(function(done) {
this.UrlFetcher.pipeUrlToFile(this.url, this.path, err => {
this.callback(err)
return done()
})
this.res = { statusCode: 404 }
this.urlStream.emit('response', this.res)
return this.urlStream.emit('end')
})
it('should call the callback with an error', function() {
this.callback.calledWith(sinon.match(Error)).should.equal(true)
const message = this.callback.args[0][0].message
expect(message).to.include(
'URL returned non-success status code: 404'
)
})
})
return it('should only call the callback once, even if end is called', function() {
this.urlStream.emit('end')
return this.callback.calledOnce.should.equal(true)
return describe('with error', function() {
beforeEach(function(done) {
this.UrlFetcher.pipeUrlToFile(this.url, this.path, err => {
this.callback(err)
return done()
})
return this.urlStream.emit(
'error',
(this.error = new Error('something went wrong'))
)
})
it('should call the callback with the error', function() {
return this.callback.calledWith(this.error).should.equal(true)
})
return it('should only call the callback once, even if end is called', function() {
this.urlStream.emit('end')
return this.callback.calledOnce.should.equal(true)
})
})
})
})