Merge pull request #1116 from solid/feature/lock-patch

Lock file during PATCH

Author: kjetilk

lib/handlers/patch.js

@@ -10,6 +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')
 
 // Patch parsers by request body content type
 const PATCH_PARSERS = {
@@ -23,6 +24,7 @@ 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
@@ -50,22 +52,24 @@ async function patchHandler (req, res, next) {
       throw error(415, `Unsupported patch content type: ${patch.contentType}`)
     }
 
-    // Parse the target graph and the patch document,
-    // and verify permission for performing this specific patch
-    const [graph, patchObject] = await Promise.all([
-      readGraph(resource),
-      parsePatch(url, patch.uri, patch.text)
-        .then(patchObject => checkPermission(req, patchObject))
-    ])
+    // Parse the patch document and verify permissions
+    const patchObject = await parsePatch(url, patch.uri, patch.text)
+    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)
 
     // Send the result to the client
     res.send(result)
   } catch (err) {
     return next(err)
+  } finally {
+    if (releaseLock) {
+      await releaseLock()
+    }
   }
   return next()
 }

lib/ldp.js

@@ -1,4 +1,4 @@
-const path = require('path')
+const { join, dirname } = require('path')
 const url = require('url')
 const fs = require('fs')
 const $rdf = require('rdflib')
@@ -17,6 +17,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 RDF_MIME_TYPES = new Set([
   'text/turtle',            // .ttl
@@ -75,20 +76,18 @@ class LDP {
     })
   }
 
-  createReadStream (filename, start, end) {
-    if (start && end) {
-      return fs.createReadStream(filename, {'start': start, 'end': end})
-    } else {
-      return fs.createReadStream(filename)
-    }
-  }
-
   async readResource (url) {
+    let releaseLock
     try {
       const { path } = await this.resourceMapper.mapUrlToFile({ url })
-      return await promisify(fs.readFile)(path, {'encoding': 'utf8'})
+      releaseLock = await lock(path, { retries: 10 })
+      return await promisify(fs.readFile)(path, {encoding: 'utf8'})
     } catch (err) {
       throw error(err.status, err.message)
+    } finally {
+      if (releaseLock) {
+        await releaseLock()
+      }
     }
   }
 
@@ -99,7 +98,7 @@ class LDP {
     return this.readResource(url + this.suffixMeta)
   }
 
-  async listContainer (filename, reqUri, containerData, hostname) {
+  async listContainer (container, reqUri, containerData, hostname) {
     const resourceGraph = $rdf.graph()
 
     try {
@@ -111,14 +110,14 @@ class LDP {
 
     try {
       // add container stats
-      await ldpContainer.addContainerStats(this, reqUri, filename, resourceGraph)
+      await ldpContainer.addContainerStats(this, reqUri, container, resourceGraph)
       // read directory
-      const files = await ldpContainer.readdir(filename)
+      const files = await ldpContainer.readdir(container)
       // iterate through all the files
       await Promise.all(files.map(async file => {
         const { url: fileUri } = await this.resourceMapper.mapFileToUrl(
-          { path: path.join(filename, file), hostname })
-        return await ldpContainer.addFile(this, resourceGraph, reqUri, fileUri, filename, file)
+          { path: join(container, file), hostname })
+        return await ldpContainer.addFile(this, resourceGraph, reqUri, fileUri, container, file)
       }))
     } catch (err) {
       throw error(500, "Can't list container")
@@ -154,7 +153,7 @@ class LDP {
     let originalPath = resourcePath
     if (container) {
       // Create directory by an LDP PUT to the container's .meta resource
-      resourcePath = path.join(originalPath, ldp.suffixMeta)
+      resourcePath = join(originalPath, ldp.suffixMeta)
       if (originalPath && !originalPath.endsWith('/')) {
         originalPath += '/'
       }
@@ -226,8 +225,8 @@ class LDP {
     }
 
     // Second, create the enclosing directory, if necessary
-    const { path: filePath } = await this.resourceMapper.mapUrlToFile({ url, contentType, createIfNotExists: true })
-    const dirName = path.dirname(filePath)
+    const { path } = await this.resourceMapper.mapUrlToFile({ url, contentType, createIfNotExists: true })
+    const dirName = dirname(path)
     try {
       await promisify(mkdirp)(dirName)
     } catch (err) {
@@ -237,16 +236,24 @@ class LDP {
     }
 
     // Directory created, now write the file
-    return await new Promise((resolve, reject) => {
-      const file = stream.pipe(fs.createWriteStream(filePath))
-      file.on('error', function () {
-        reject(error(500, 'Error writing data'))
-      })
-      file.on('finish', function () {
-        debug.handlers('PUT -- Wrote data to: ' + filePath)
-        resolve()
+    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()
+        })
       })
-    })
+    } finally {
+      if (releaseLock) {
+        await releaseLock()
+      }
+    }
   }
 
   async exists (hostname, path, searchIndex = true) {
@@ -320,82 +327,86 @@ class LDP {
   }
 
   async get (options, searchIndex = true) {
-    let filename, contentType, stats
+    let path, contentType, stats
     try {
-      ({ path: filename, contentType } = await this.resourceMapper.mapUrlToFile({ url: options, searchIndex }))
-      stats = await this.stat(filename)
+      ({ path, contentType } = await this.resourceMapper.mapUrlToFile({ url: options, searchIndex }))
+      stats = await this.stat(path)
     } catch (err) {
       throw error(404, 'Can\'t find file requested: ' + options)
     }
 
     // Just return, since resource exists
     if (!options.includeBody) {
-      return { 'stream': stats, 'contentType': contentType, 'container': stats.isDirectory() }
+      return { stream: stats, contentType, container: stats.isDirectory() }
     }
 
     // Found a container
     if (stats.isDirectory()) {
       const { url: absContainerUri } = await this.resourceMapper
-        .mapFileToUrl({ path: filename, hostname: options.hostname })
+        .mapFileToUrl({ path, hostname: options.hostname })
       const metaFile = await this.readContainerMeta(absContainerUri)
         .catch(() => '') // Default to an empty meta file if it is missing
       let data
       try {
-        data = await this.listContainer(filename, absContainerUri, metaFile, options.hostname)
+        data = await this.listContainer(path, absContainerUri, metaFile, options.hostname)
       } catch (err) {
         debug.handlers('GET container -- Read error:' + err.message)
         throw err
       }
       const stream = stringToStream(data)
       // TODO 'text/turtle' is fixed, should be contentType instead
       // This forces one more translation turtle -> desired
-      return { 'stream': stream, 'contentType': 'text/turtle', 'container': true }
+      return { stream, contentType: 'text/turtle', container: true }
     } else {
-      let stream
-      let chunksize
-      let contentRange
+      let chunksize, contentRange, start, end
       if (options.range) {
-        const total = fs.statSync(filename).size
+        const total = fs.statSync(path).size
         const parts = options.range.replace(/bytes=/, '').split('-')
         const partialstart = parts[0]
         const partialend = parts[1]
-        const start = parseInt(partialstart, 10)
-        const end = partialend ? parseInt(partialend, 10) : total - 1
+        start = parseInt(partialstart, 10)
+        end = partialend ? parseInt(partialend, 10) : total - 1
         chunksize = (end - start) + 1
         contentRange = 'bytes ' + start + '-' + end + '/' + total
-        stream = this.createReadStream(filename, start, end)
-      } else {
-        stream = this.createReadStream(filename)
       }
-      return new Promise((resolve, reject) => {
-        stream
-          .on('error', function (err) {
-            debug.handlers(`GET -- error reading ${filename}: ${err.message}`)
-            return reject(error(err, "Can't read file " + err))
-          })
-          .on('open', function () {
-            debug.handlers(`GET -- Reading ${filename}`)
-            return resolve({ stream, contentType, container: false, contentRange, chunksize })
-          })
-      })
+      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()
+        }
+      }
     }
   }
 
   async delete (url) {
     // First check if the path points to a valid file
-    let filePath, stats
+    let path, stats
     try {
-      ({ path: filePath } = await this.resourceMapper.mapUrlToFile({ url }))
-      stats = await this.stat(filePath)
+      ({ path } = await this.resourceMapper.mapUrlToFile({ url }))
+      stats = await this.stat(path)
     } catch (err) {
       throw error(404, "Can't find " + err)
     }
 
     // If so, delete the directory or file
     if (stats.isDirectory()) {
-      return this.deleteContainer(filePath)
+      return this.deleteContainer(path)
     } else {
-      return this.deleteResource(filePath)
+      return this.deleteResource(path)
     }
   }
 
@@ -425,28 +436,34 @@ class LDP {
     }
   }
 
-  async deleteResource (filename) {
+  async deleteResource (path) {
+    let releaseLock
     try {
-      return promisify(fs.unlink)(filename)
+      releaseLock = await lock(path, { retries: 10 })
+      return await promisify(fs.unlink)(path)
     } catch (err) {
       debug.container('DELETE -- unlink() error: ' + err)
       throw error(err, 'Failed to delete resource')
+    } finally {
+      if (releaseLock) {
+        await releaseLock()
+      }
     }
   }
 
   getAvailablePath (host, containerURI, { slug = uuid.v1(), extension }) {
-    const filename = slug + extension
+    const path = slug + extension
     function ensureNotExists (self, newPath) {
       // Verify whether the new path already exists
       return self.exists(host, newPath).then(
         // If it does, generate another one
         () => ensureNotExists(self, URI.joinPaths(containerURI,
-                `${uuid.v1().split('-')[0]}-${filename}`).toString()),
+                `${uuid.v1().split('-')[0]}-${path}`).toString()),
         // If not, we found an appropriate path
         () => newPath
       )
     }
-    return ensureNotExists(this, URI.joinPaths(containerURI, filename).toString())
+    return ensureNotExists(this, URI.joinPaths(containerURI, path).toString())
   }
 
   getTrustedOrigins (req) {