Merge pull request #1163 from solid/fix/lock-timeout

Make stale locks result in 500 instead of server crash

Author: jaxoncreed

lib/handlers/patch.js

@@ -10,7 +10,7 @@ const $rdf = require('rdflib')
 const crypto = require('crypto')
 const overQuota = require('../utils').overQuota
 const getContentType = require('../utils').getContentType
-const { lock } = require('proper-lockfile')
+const withLock = require('../lock')
 
 // Patch parsers by request body content type
 const PATCH_PARSERS = {
@@ -24,7 +24,6 @@ const DEFAULT_FOR_NEW_CONTENT_TYPE = 'text/turtle'
 async function patchHandler (req, res, next) {
   debug(`PATCH -- ${req.originalUrl}`)
   res.header('MS-Author-Via', 'SPARQL')
-  let releaseLock
   try {
     // Obtain details of the target resource
     const ldp = req.app.locals.ldp
@@ -57,19 +56,16 @@ async function patchHandler (req, res, next) {
     await checkPermission(req, patchObject)
 
     // Patch the graph and write it back to the file
-    releaseLock = await lock(path, { retries: 10, realpath: false })
-    const graph = await readGraph(resource)
-    await applyPatch(patchObject, graph, url)
-    const result = await writeGraph(graph, resource, ldp.resourceMapper.rootPath, ldp.serverUri)
+    const result = await withLock(path, { mustExist: false }, async () => {
+      const graph = await readGraph(resource)
+      await applyPatch(patchObject, graph, url)
+      return writeGraph(graph, resource, ldp.resourceMapper.rootPath, ldp.serverUri)
+    })
 
     // Send the result to the client
     res.send(result)
   } catch (err) {
     return next(err)
-  } finally {
-    if (releaseLock) {
-      await releaseLock()
-    }
   }
   return next()
 }

lib/ldp.js

@@ -18,7 +18,7 @@ const parse = require('./utils').parse
 const fetch = require('node-fetch')
 const { promisify } = require('util')
 const URI = require('urijs')
-const { lock } = require('proper-lockfile')
+const withLock = require('./lock')
 
 const RDF_MIME_TYPES = new Set([
   'text/turtle',            // .ttl
@@ -78,17 +78,11 @@ class LDP {
   }
 
   async readResource (url) {
-    let releaseLock
     try {
       const { path } = await this.resourceMapper.mapUrlToFile({ url })
-      releaseLock = await lock(path, { retries: 10 })
-      return await promisify(fs.readFile)(path, {encoding: 'utf8'})
+      return await withLock(path, () => promisify(fs.readFile)(path, {encoding: 'utf8'}))
     } catch (err) {
       throw error(err.status, err.message)
-    } finally {
-      if (releaseLock) {
-        await releaseLock()
-      }
     }
   }
 
@@ -244,24 +238,16 @@ class LDP {
     }
 
     // Directory created, now write the file
-    let releaseLock
-    try {
-      releaseLock = await lock(path, { retries: 10, realpath: false })
-      return await new Promise((resolve, reject) => {
-        const file = stream.pipe(fs.createWriteStream(path))
-        file.on('error', function () {
-          reject(error(500, 'Error writing data'))
-        })
-        file.on('finish', function () {
-          debug.handlers('PUT -- Wrote data to: ' + path)
-          resolve()
-        })
+    return withLock(path, { mustExist: false }, () => new Promise((resolve, reject) => {
+      const file = stream.pipe(fs.createWriteStream(path))
+      file.on('error', function () {
+        reject(error(500, 'Error writing data'))
       })
-    } finally {
-      if (releaseLock) {
-        await releaseLock()
-      }
-    }
+      file.on('finish', function () {
+        debug.handlers('PUT -- Wrote data to: ' + path)
+        resolve()
+      })
+    }))
   }
 
   async exists (hostname, path, searchIndex = true) {
@@ -379,26 +365,18 @@ class LDP {
         chunksize = (end - start) + 1
         contentRange = 'bytes ' + start + '-' + end + '/' + total
       }
-      let releaseLock
-      try {
-        releaseLock = await lock(path, { retries: 10 })
-        return await new Promise((resolve, reject) => {
-          const stream = fs.createReadStream(path, start && end ? {start, end} : {})
-          stream
-            .on('error', function (err) {
-              debug.handlers(`GET -- error reading ${path}: ${err.message}`)
-              return reject(error(err, "Can't read file " + err))
-            })
-            .on('open', function () {
-              debug.handlers(`GET -- Reading ${path}`)
-              return resolve({ stream, contentType, container: false, contentRange, chunksize })
-            })
-        })
-      } finally {
-        if (releaseLock) {
-          await releaseLock()
-        }
-      }
+      return withLock(path, () => new Promise((resolve, reject) => {
+        const stream = fs.createReadStream(path, start && end ? {start, end} : {})
+        stream
+          .on('error', function (err) {
+            debug.handlers(`GET -- error reading ${path}: ${err.message}`)
+            return reject(error(err, "Can't read file " + err))
+          })
+          .on('open', function () {
+            debug.handlers(`GET -- Reading ${path}`)
+            return resolve({ stream, contentType, container: false, contentRange, chunksize })
+          })
+      }))
     }
   }
 
@@ -447,17 +425,11 @@ class LDP {
   }
 
   async deleteResource (path) {
-    let releaseLock
     try {
-      releaseLock = await lock(path, { retries: 10 })
-      return await promisify(fs.unlink)(path)
+      return await withLock(path, { mustExist: false }, () => promisify(fs.unlink)(path))
     } catch (err) {
       debug.container('DELETE -- unlink() error: ' + err)
       throw error(err, 'Failed to delete resource')
-    } finally {
-      if (releaseLock) {
-        await releaseLock()
-      }
     }
   }
 

lib/lock.js

@@ -0,0 +1,31 @@
+const { lock } = require('proper-lockfile')
+
+const staleSeconds = 30
+
+// Obtains a lock on the path, and maintains it until the callback finishes
+function withLock (path, options = {}, callback = options) {
+  return new Promise(async (resolve, reject) => {
+    let releaseLock, result
+    try {
+      // Obtain the lock
+      releaseLock = await lock(path, {
+        retries: 10,
+        update: 1000,
+        stale: staleSeconds * 1000,
+        realpath: !!options.mustExist,
+        onCompromised: () =>
+          reject(new Error(`The file at ${path} was not updated within ${staleSeconds}s.`))
+      })
+      // Hold on to the lock until the callback's returned promise resolves
+      result = await callback()
+    } catch (error) {
+      reject(error)
+    // Ensure the lock is always released
+    } finally {
+      await releaseLock()
+    }
+    resolve(result)
+  })
+}
+
+module.exports = withLock