Merge branch 'master' into bg-add-compile-groups

app.js

@@ -228,7 +228,7 @@ app.get('/smoke_test_force', (req, res) => smokeTest.sendNewResult(res))
 
 app.use(function(error, req, res, next) {
   if (error instanceof Errors.NotFoundError) {
-    logger.warn({ err: error, url: req.url }, 'not found error')
+    logger.log({ err: error, url: req.url }, 'not found error')
     return res.sendStatus(404)
   } else if (error.code === 'EPIPE') {
     // inspect container returns EPIPE when shutting down

app/js/CompileManager.js

@@ -431,15 +431,7 @@ module.exports = CompileManager = {
     const compileDir = getCompileDir(project_id, user_id)
     const synctex_path = `${base_dir}/output.pdf`
     const command = ['code', synctex_path, file_path, line, column]
-    return fse.ensureDir(compileDir, function(error) {
+    CompileManager._runSynctex(project_id, user_id, command, function(
-      if (error != null) {
-        logger.err(
-          { error, project_id, user_id, file_name },
-          'error ensuring dir for sync from code'
-        )
-        return callback(error)
-      }
-      return CompileManager._runSynctex(project_id, user_id, command, function(
       error,
       stdout
     ) {
@@ -450,11 +442,7 @@ module.exports = CompileManager = {
         { project_id, user_id, file_name, line, column, command, stdout },
         'synctex code output'
       )
-        return callback(
+      return callback(null, CompileManager._parseSynctexFromCodeOutput(stdout))
-          null,
-          CompileManager._parseSynctexFromCodeOutput(stdout)
-        )
-      })
     })
   },
 
@@ -467,15 +455,7 @@ module.exports = CompileManager = {
     const base_dir = Settings.path.synctexBaseDir(compileName)
     const synctex_path = `${base_dir}/output.pdf`
     const command = ['pdf', synctex_path, page, h, v]
-    return fse.ensureDir(compileDir, function(error) {
+    CompileManager._runSynctex(project_id, user_id, command, function(
-      if (error != null) {
-        logger.err(
-          { error, project_id, user_id, file_name },
-          'error ensuring dir for sync to code'
-        )
-        return callback(error)
-      }
-      return CompileManager._runSynctex(project_id, user_id, command, function(
       error,
       stdout
     ) {
@@ -491,29 +471,23 @@ module.exports = CompileManager = {
         CompileManager._parseSynctexFromPdfOutput(stdout, base_dir)
       )
     })
-    })
   },
 
-  _checkFileExists(path, callback) {
+  _checkFileExists(dir, filename, callback) {
     if (callback == null) {
       callback = function(error) {}
     }
-    const synctexDir = Path.dirname(path)
+    const file = Path.join(dir, filename)
-    const synctexFile = Path.join(synctexDir, 'output.synctex.gz')
+    return fs.stat(dir, function(error, stats) {
-    return fs.stat(synctexDir, function(error, stats) {
       if ((error != null ? error.code : undefined) === 'ENOENT') {
-        return callback(
+        return callback(new Errors.NotFoundError('no output directory'))
-          new Errors.NotFoundError('called synctex with no output directory')
-        )
       }
       if (error != null) {
         return callback(error)
       }
-      return fs.stat(synctexFile, function(error, stats) {
+      return fs.stat(file, function(error, stats) {
         if ((error != null ? error.code : undefined) === 'ENOENT') {
-          return callback(
+          return callback(new Errors.NotFoundError('no output file'))
-            new Errors.NotFoundError('called synctex with no output file')
-          )
         }
         if (error != null) {
           return callback(error)
@@ -538,6 +512,10 @@ module.exports = CompileManager = {
     const timeout = 60 * 1000 // increased to allow for large projects
     const compileName = getCompileName(project_id, user_id)
     const compileGroup = 'synctex'
+    CompileManager._checkFileExists(directory, 'output.synctex.gz', error => {
+      if (error) {
+        return callback(error)
+      }
       return CommandRunner.run(
         compileName,
         command,
@@ -557,6 +535,7 @@ module.exports = CompileManager = {
           return callback(null, output.stdout)
         }
       )
+    })
   },
 
   _parseSynctexFromCodeOutput(output) {

app/js/DockerRunner.js

@@ -26,7 +26,6 @@ const LockManager = require('./DockerLockManager')
 const fs = require('fs')
 const Path = require('path')
 const _ = require('lodash')
-const metrics = require('metrics-sharelatex')
 
 logger.info('using docker runner')
 
@@ -446,18 +445,10 @@ module.exports = DockerRunner = {
           })
         }
       )
-    var inspectContainer = isRetry =>
+    return container.inspect(function(error, stats) {
-      container.inspect(function(error, stats) {
       if ((error != null ? error.statusCode : undefined) === 404) {
         return createAndStartContainer()
       } else if (error != null) {
-          if (error.message.match(/EPIPE/)) {
-            if (!isRetry) {
-              metrics.inc('container-inspect-epipe-retry')
-              return inspectContainer(true)
-            }
-            metrics.inc('container-inspect-epipe-error')
-          }
         logger.err(
           { container_name: name, error },
           'unable to inspect container to start'
@@ -467,7 +458,6 @@ module.exports = DockerRunner = {
         return startExistingContainer()
       }
     })
-    inspectContainer(false)
   },
 
   attachToContainer(containerId, attachStreamHandler, attachStartCallback) {

test/acceptance/js/SynctexTests.js

@@ -69,7 +69,7 @@ Hello world
     })
   })
 
-  return describe('from pdf to code', function() {
+  describe('from pdf to code', function() {
     return it('should return the correct location', function(done) {
       return Client.syncFromPdf(
         this.project_id,
@@ -88,4 +88,104 @@ Hello world
       )
     })
   })
+
+  describe('when the project directory is not available', function() {
+    before(function() {
+      this.other_project_id = Client.randomId()
+    })
+    describe('from code to pdf', function() {
+      it('should return a 404 response', function(done) {
+        return Client.syncFromCode(
+          this.other_project_id,
+          'main.tex',
+          3,
+          5,
+          (error, body) => {
+            if (error != null) {
+              throw error
+            }
+            expect(body).to.equal('Not Found')
+            return done()
+          }
+        )
+      })
+    })
+    describe('from pdf to code', function() {
+      it('should return a 404 response', function(done) {
+        return Client.syncFromPdf(
+          this.other_project_id,
+          1,
+          100,
+          200,
+          (error, body) => {
+            if (error != null) {
+              throw error
+            }
+            expect(body).to.equal('Not Found')
+            return done()
+          }
+        )
+      })
+    })
+  })
+
+  describe('when the synctex file is not available', function() {
+    before(function(done) {
+      this.broken_project_id = Client.randomId()
+      const content = 'this is not valid tex' // not a valid tex file
+      this.request = {
+        resources: [
+          {
+            path: 'main.tex',
+            content
+          }
+        ]
+      }
+      Client.compile(
+        this.broken_project_id,
+        this.request,
+        (error, res, body) => {
+          this.error = error
+          this.res = res
+          this.body = body
+          return done()
+        }
+      )
+    })
+
+    describe('from code to pdf', function() {
+      it('should return a 404 response', function(done) {
+        return Client.syncFromCode(
+          this.broken_project_id,
+          'main.tex',
+          3,
+          5,
+          (error, body) => {
+            if (error != null) {
+              throw error
+            }
+            expect(body).to.equal('Not Found')
+            return done()
+          }
+        )
+      })
+    })
+    describe('from pdf to code', function() {
+      it('should return a 404 response', function(done) {
+        return Client.syncFromPdf(
+          this.broken_project_id,
+          1,
+          100,
+          200,
+          (error, body) => {
+            if (error != null) {
+              throw error
+            }
+            expect(body).to.equal('Not Found')
+            return done()
+          }
+        )
+      })
+    })
+  })
 })

test/acceptance/js/helpers/Client.js

@@ -81,13 +81,14 @@ module.exports = Client = {
           file,
           line,
           column
-        }
+        },
+        json: true
       },
       (error, response, body) => {
         if (error != null) {
           return callback(error)
         }
-        return callback(null, JSON.parse(body))
+        return callback(null, body)
       }
     )
   },
@@ -103,13 +104,14 @@ module.exports = Client = {
           page,
           h,
           v
-        }
+        },
+        json: true
       },
       (error, response, body) => {
         if (error != null) {
           return callback(error)
         }
-        return callback(null, JSON.parse(body))
+        return callback(null, body)
       }
     )
   },

test/unit/js/DockerRunnerTests.js

@@ -36,7 +36,6 @@ describe('DockerRunner', function() {
         'logger-sharelatex': (this.logger = {
           log: sinon.stub(),
           error: sinon.stub(),
-          err: sinon.stub(),
           info: sinon.stub(),
           warn: sinon.stub()
         }),
@@ -453,63 +452,6 @@ describe('DockerRunner', function() {
       })
     })
 
-    describe('when inspect always fails with EPIPE error', function() {
-      beforeEach(function() {
-        this.error = new Error('write EPIPE')
-        this.container.inspect = sinon.stub().yields(this.error)
-        this.container.start = sinon.stub().yields()
-
-        this.DockerRunner.startContainer(
-          this.options,
-          this.volumes,
-          () => {},
-          this.callback
-        )
-      })
-
-      it('should retry once', function() {
-        sinon.assert.callOrder(
-          this.container.inspect,
-          this.container.inspect,
-          this.callback
-        )
-      })
-
-      it('should call back with error', function() {
-        sinon.assert.calledWith(this.callback, this.error)
-      })
-    })
-
-    describe('when inspect fails once with EPIPE error', function() {
-      beforeEach(function() {
-        this.container.inspect = sinon.stub()
-        this.container.inspect.onFirstCall().yields(new Error('write EPIPE'))
-        this.container.inspect.onSecondCall().yields()
-        this.container.start = sinon.stub().yields()
-
-        this.DockerRunner.startContainer(
-          this.options,
-          this.volumes,
-          () => {},
-          this.callback
-        )
-      })
-
-      it('should retry once and start container', function() {
-        sinon.assert.callOrder(
-          this.container.inspect,
-          this.container.inspect,
-          this.DockerRunner.attachToContainer,
-          this.container.start,
-          this.callback
-        )
-      })
-
-      it('should call back without error', function() {
-        sinon.assert.calledWith(this.callback, null)
-      })
-    })
-
     describe('when the container does not exist', function() {
       beforeEach(function() {
         const exists = false