From 77e8ba74a72911da73ffa29e677697278384dc8a Mon Sep 17 00:00:00 2001 From: Henry Oswald Date: Thu, 14 May 2020 13:09:57 +0100 Subject: [PATCH] add pipeUrlToFileWithRetry function to retry file downloads 3 times --- services/clsi/app/js/UrlCache.js | 2 +- services/clsi/app/js/UrlFetcher.js | 9 +- .../test/acceptance/js/UrlCachingTests.js | 1 - services/clsi/test/unit/js/UrlCacheTests.js | 6 +- services/clsi/test/unit/js/UrlFetcherTests.js | 244 ++++++++++-------- 5 files changed, 153 insertions(+), 109 deletions(-) diff --git a/services/clsi/app/js/UrlCache.js b/services/clsi/app/js/UrlCache.js index 9b982e5e63..df9c175a63 100644 --- a/services/clsi/app/js/UrlCache.js +++ b/services/clsi/app/js/UrlCache.js @@ -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 => { diff --git a/services/clsi/app/js/UrlFetcher.js b/services/clsi/app/js/UrlFetcher.js index 19c681ca1f..6c7d83af0e 100644 --- a/services/clsi/app/js/UrlFetcher.js +++ b/services/clsi/app/js/UrlFetcher.js @@ -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) {} diff --git a/services/clsi/test/acceptance/js/UrlCachingTests.js b/services/clsi/test/acceptance/js/UrlCachingTests.js index 4d6249784c..b86541b681 100644 --- a/services/clsi/test/acceptance/js/UrlCachingTests.js +++ b/services/clsi/test/acceptance/js/UrlCachingTests.js @@ -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') diff --git a/services/clsi/test/unit/js/UrlCacheTests.js b/services/clsi/test/unit/js/UrlCacheTests.js index f056a6eb22..f5c0f3e20d 100644 --- a/services/clsi/test/unit/js/UrlCacheTests.js +++ b/services/clsi/test/unit/js/UrlCacheTests.js @@ -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() { diff --git a/services/clsi/test/unit/js/UrlFetcherTests.js b/services/clsi/test/unit/js/UrlFetcherTests.js index c435f450dd..247920979f 100644 --- a/services/clsi/test/unit/js/UrlFetcherTests.js +++ b/services/clsi/test/unit/js/UrlFetcherTests.js @@ -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) + }) }) }) })