add pipeUrlToFileWithRetry function to retry file downloads 3 times

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 => {

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

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

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

test/unit/js/UrlFetcherTests.js

@@ -33,72 +33,64 @@ describe('UrlFetcher', function() {
       }
     }))
   })
-
+  describe('pipeUrlToFileWithRetry', function() {
-  it('should turn off the cookie jar in request', function() {
+    this.beforeEach(function() {
-    return this.defaults.calledWith({ jar: false }).should.equal(true)
+      this.UrlFetcher.pipeUrlToFile = sinon.stub()
-  })
-
-  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) {
+    it('should call pipeUrlToFile', function(done) {
-      this.UrlFetcher.pipeUrlToFile(this.url, this.path, () => {
+      this.UrlFetcher.pipeUrlToFile.callsArgWith(2)
-        this.request.get.args[0][0].url.should.equal(this.url)
+      this.UrlFetcher.pipeUrlToFileWithRetry(this.url, this.path, err => {
-        return done()
+        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) {
+    it('should call pipeUrlToFile multiple times on error', function(done) {
-      this.settings.filestoreDomainOveride = '192.11.11.11'
+      error = new Error("couldn't download file")
-      this.UrlFetcher.pipeUrlToFile(this.url, this.path, () => {
+      this.UrlFetcher.pipeUrlToFile.callsArgWith(2, error)
-        this.request.get.args[0][0].url.should.equal(
+      this.UrlFetcher.pipeUrlToFileWithRetry(this.url, this.path, err => {
-          '192.11.11.11/file/here?query=string'
+        expect(err).to.equal(error)
-        )
+        this.UrlFetcher.pipeUrlToFile.callCount.should.equal(3)
-        return done()
+        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() {
+  describe('pipeUrlToFile', function() {
-    beforeEach(function(done) {
+    it('should turn off the cookie jar in request', function() {
-      this.path = '/path/to/file/on/disk'
+      return this.defaults.calledWith({ jar: false }).should.equal(true)
-      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() {
+    describe('rewrite url domain if filestoreDomainOveride is set', function() {
-      beforeEach(function(done) {
+      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 it('should use override domain when filestoreDomainOveride is set', function(done) {
-        return this.request.get
+        this.settings.filestoreDomainOveride = '192.11.11.11'
-          .calledWith(sinon.match({ url: this.url }))
+        this.UrlFetcher.pipeUrlToFile(this.url, this.path, () => {
-          .should.equal(true)
+          this.request.get.args[0][0].url.should.equal(
-      })
+            '192.11.11.11/file/here?query=string'
-
+          )
-      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 done()
         })
-        this.res = { statusCode: 404 }
+        this.res = { statusCode: 200 }
         this.urlStream.emit('response', this.res)
-        return this.urlStream.emit('end')
+        this.urlStream.emit('end')
-      })
+        return this.fileStream.emit('finish')
-
-      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 describe('with error', function() {
+    return describe('pipeUrlToFile', function() {
       beforeEach(function(done) {
-        this.UrlFetcher.pipeUrlToFile(this.url, this.path, err => {
+        this.path = '/path/to/file/on/disk'
-          this.callback(err)
+        this.request.get = sinon
-          return done()
+          .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() {
+      describe('with non success status code', function() {
-        return this.callback.calledWith(this.error).should.equal(true)
+        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() {
+      return describe('with error', function() {
-        this.urlStream.emit('end')
+        beforeEach(function(done) {
-        return this.callback.calledOnce.should.equal(true)
+          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)
+        })
       })
     })
   })