fix: remove partial disk file on aborted uploads

lib/make-middleware.js

@@ -35,6 +35,8 @@ function makeMiddleware (setup) {
     var errorOccured = false
     var pendingWrites = new Counter()
     var uploadedFiles = []
+    var filesInProgress = []
+    var filesToCleanup = []
 
     function done (err) {
       var called = false
@@ -73,16 +75,28 @@ function makeMiddleware (setup) {
       if (readFinished && pendingWrites.isZero() && !errorOccured) done()
     }
 
+    function removePendingFile (file) {
+      var fileIndex = filesInProgress.indexOf(file)
+      if (~fileIndex) {
+        filesInProgress.splice(fileIndex, 1)
+      }
+    }
+
     function abortWithError (uploadError, skipPendingWait) {
       if (errorOccured) return
       errorOccured = true
+      filesToCleanup = uploadedFiles.concat(filesInProgress)
 
       function finishAbort () {
+        var files = filesToCleanup.concat()
+        uploadedFiles = []
+        filesInProgress = []
+
         function remove (file, cb) {
           storage._removeFile(req, file, cb)
         }
 
-        removeUploadedFiles(uploadedFiles, remove, function (err, storageErrors) {
+        removeUploadedFiles(files, remove, function (err, storageErrors) {
           if (err) return done(err)
 
           uploadError.storageErrors = storageErrors
@@ -199,6 +213,7 @@ function makeMiddleware (setup) {
         var aborting = false
         pendingWritesIncremented = true
         pendingWrites.increment()
+        filesInProgress.push(file)
 
         Object.defineProperty(file, 'stream', {
           configurable: true,
@@ -212,6 +227,13 @@ function makeMiddleware (setup) {
         })
 
         storage._handleFile(req, file, function (err, info) {
+          removePendingFile(file)
+
+          if (errorOccured) {
+            appender.removePlaceholder(placeholder)
+            return pendingWrites.decrement()
+          }
+
           if (aborting) {
             appender.removePlaceholder(placeholder)
             uploadedFiles.push({ ...file, ...info })
@@ -220,6 +242,7 @@ function makeMiddleware (setup) {
 
           if (err) {
             appender.removePlaceholder(placeholder)
+            uploadedFiles.push(file)
             pendingWrites.decrement()
             return abortWithError(err)
           }

storage/disk.js

@@ -34,6 +34,9 @@ DiskStorage.prototype._handleFile = function _handleFile (req, file, cb) {
       if (err) return cb(err)
 
       var finalPath = path.join(destination, filename)
+      file.destination = destination
+      file.filename = filename
+      file.path = finalPath
       var outStream = fs.createWriteStream(finalPath)
 
       file.stream.pipe(outStream)

test/_util.js

@@ -2,14 +2,22 @@ var fs = require('fs')
 var path = require('path')
 var stream = require('stream')
 
+function filePath (name) {
+  return path.join(__dirname, 'files', name)
+}
+
 exports.file = function file (name) {
-  return fs.createReadStream(path.join(__dirname, 'files', name))
+  return fs.createReadStream(filePath(name))
 }
 
 exports.fileSize = function fileSize (path) {
   return fs.statSync(path).size
 }
 
+exports.fixtureSize = function fixtureSize (name) {
+  return exports.fileSize(filePath(name))
+}
+
 exports.submitForm = function submitForm (multer, form, cb) {
   form.getLength(function (err, length) {
     if (err) return cb(err)

test/disk-storage.js

@@ -10,6 +10,11 @@ var multer = require('../')
 var temp = require('fs-temp')
 var rimraf = require('rimraf')
 var FormData = require('form-data')
+var small0Size = util.fixtureSize('small0.dat')
+var small1Size = util.fixtureSize('small1.dat')
+var tiny0Size = util.fixtureSize('tiny0.dat')
+var mediumSize = util.fixtureSize('medium.dat')
+var largeSize = util.fixtureSize('large.jpg')
 
 describe('Disk Storage', function () {
   var uploadDir, upload
@@ -42,8 +47,8 @@ describe('Disk Storage', function () {
 
       assert.strictEqual(req.file.fieldname, 'small0')
       assert.strictEqual(req.file.originalname, 'small0.dat')
-      assert.strictEqual(req.file.size, 1778)
-      assert.strictEqual(util.fileSize(req.file.path), 1778)
+      assert.strictEqual(req.file.size, small0Size)
+      assert.strictEqual(util.fileSize(req.file.path), small0Size)
 
       done()
     })
@@ -116,8 +121,8 @@ describe('Disk Storage', function () {
 
       assert.strictEqual(req.files.tiny0[0].fieldname, 'tiny0')
       assert.strictEqual(req.files.tiny0[0].originalname, 'tiny0.dat')
-      assert.strictEqual(req.files.tiny0[0].size, 122)
-      assert.strictEqual(util.fileSize(req.files.tiny0[0].path), 122)
+      assert.strictEqual(req.files.tiny0[0].size, tiny0Size)
+      assert.strictEqual(util.fileSize(req.files.tiny0[0].path), tiny0Size)
 
       assert.strictEqual(req.files.tiny1[0].fieldname, 'tiny1')
       assert.strictEqual(req.files.tiny1[0].originalname, 'tiny1.dat')
@@ -126,23 +131,23 @@ describe('Disk Storage', function () {
 
       assert.strictEqual(req.files.small0[0].fieldname, 'small0')
       assert.strictEqual(req.files.small0[0].originalname, 'small0.dat')
-      assert.strictEqual(req.files.small0[0].size, 1778)
-      assert.strictEqual(util.fileSize(req.files.small0[0].path), 1778)
+      assert.strictEqual(req.files.small0[0].size, small0Size)
+      assert.strictEqual(util.fileSize(req.files.small0[0].path), small0Size)
 
       assert.strictEqual(req.files.small1[0].fieldname, 'small1')
       assert.strictEqual(req.files.small1[0].originalname, 'small1.dat')
-      assert.strictEqual(req.files.small1[0].size, 315)
-      assert.strictEqual(util.fileSize(req.files.small1[0].path), 315)
+      assert.strictEqual(req.files.small1[0].size, small1Size)
+      assert.strictEqual(util.fileSize(req.files.small1[0].path), small1Size)
 
       assert.strictEqual(req.files.medium[0].fieldname, 'medium')
       assert.strictEqual(req.files.medium[0].originalname, 'medium.dat')
-      assert.strictEqual(req.files.medium[0].size, 13196)
-      assert.strictEqual(util.fileSize(req.files.medium[0].path), 13196)
+      assert.strictEqual(req.files.medium[0].size, mediumSize)
+      assert.strictEqual(util.fileSize(req.files.medium[0].path), mediumSize)
 
       assert.strictEqual(req.files.large[0].fieldname, 'large')
       assert.strictEqual(req.files.large[0].originalname, 'large.jpg')
-      assert.strictEqual(req.files.large[0].size, 2413677)
-      assert.strictEqual(util.fileSize(req.files.large[0].path), 2413677)
+      assert.strictEqual(req.files.large[0].size, largeSize)
+      assert.strictEqual(util.fileSize(req.files.large[0].path), largeSize)
 
       done()
     })

test/error-handling.js

@@ -2,7 +2,10 @@
 
 var assert = require('assert')
 
+var fs = require('fs')
 var os = require('os')
+var temp = require('fs-temp')
+var rimraf = require('rimraf')
 var util = require('./_util')
 var multer = require('../')
 var removeUploadedFiles = require('../lib/remove-uploaded-files')
@@ -460,3 +463,85 @@ describe('Error Handling', function () {
     })
   })
 })
+
+it('should remove uploaded files if request is aborted before completion', function (done) {
+  this.timeout(5000)
+
+  temp.mkdir(function (err, uploadDir) {
+    if (err) return done(err)
+
+    var upload = multer({ storage: multer.diskStorage({ destination: uploadDir }) }).single('file')
+    var server = http.createServer(function (req, res) {
+      upload(req, res, function (err) {
+        assert(err)
+        var files = fs.readdirSync(uploadDir)
+        assert.strictEqual(files.length, 0)
+        clearTimeout(timer)
+        server.close(function () {
+          rimraf(uploadDir, function (cleanupErr) {
+            done(cleanupErr)
+          })
+        })
+      })
+    })
+
+    var timer = setTimeout(function () {
+      server.close(function () {
+        rimraf(uploadDir, function (cleanupErr) {
+          done(cleanupErr || new Error('middleware did not complete after client abort'))
+        })
+      })
+    }, 2000)
+
+    server.listen(0, function () {
+      var port = server.address().port
+      var boundary = 'Abort' + Date.now()
+      var preamble = [
+        '--' + boundary,
+        'Content-Disposition: form-data; name="file"; filename="test.bin"',
+        'Content-Type: application/octet-stream',
+        '',
+        ''
+      ].join('\r\n')
+      var footer = '\r\n--' + boundary + '--\r\n'
+      var chunk = Buffer.alloc(32 * 1024, 97)
+      var totalChunks = 5
+      var contentLength = Buffer.byteLength(preamble) +
+          (chunk.length * totalChunks) +
+          Buffer.byteLength(footer)
+
+      var sock = new net.Socket()
+      var sentChunks = 0
+      function writeChunk () {
+        if (sentChunks >= 3) {
+          sock.destroy()
+          return
+        }
+
+        sentChunks += 1
+        var canContinue = sock.write(chunk)
+        if (canContinue) {
+          setTimeout(writeChunk, 2)
+        } else {
+          sock.once('drain', function () {
+            setTimeout(writeChunk, 2)
+          })
+        }
+      }
+
+      sock.connect(port, '127.0.0.1', function () {
+        sock.write(
+          'POST / HTTP/1.1\r\n' +
+            'Host: localhost\r\n' +
+            'Connection: close\r\n' +
+            'Content-Type: multipart/form-data; boundary=' + boundary + '\r\n' +
+            'Content-Length: ' + contentLength + '\r\n\r\n'
+        )
+        sock.write(preamble)
+        writeChunk()
+      })
+
+      sock.on('error', function () {})
+    })
+  })
+})

test/functionality.js

@@ -1,6 +1,7 @@
 /* eslint-env mocha */
 
 var assert = require('assert')
+var path = require('path')
 
 var util = require('./_util')
 var multer = require('../')
@@ -16,6 +17,8 @@ function startsWith (str, start) {
   return (str.substring(0, start.length) === start)
 }
 
+var small0Size = util.fixtureSize('small0.dat')
+
 describe('Functionality', function () {
   var cleanup = []
 
@@ -52,7 +55,7 @@ describe('Functionality', function () {
       util.submitForm(parser, env.form, function (err, req) {
         assert.ifError(err)
         assert.ok(startsWith(req.file.path, env.uploadDir))
-        assert.strictEqual(util.fileSize(req.file.path), 1778)
+        assert.strictEqual(util.fileSize(req.file.path), small0Size)
         done()
       })
     })
@@ -129,8 +132,8 @@ describe('Functionality', function () {
     util.submitForm(parser, form, function (err, req) {
       assert.ifError(err)
       assert.strictEqual(req.files.length, 2)
-      assert.ok(req.files[0].path.indexOf('/testforme-') >= 0)
-      assert.ok(req.files[1].path.indexOf('/testforme-') >= 0)
+      assert.strictEqual(path.basename(path.dirname(req.files[0].path)).startsWith('testforme-'), true)
+      assert.strictEqual(path.basename(path.dirname(req.files[1].path)).startsWith('testforme-'), true)
       done()
     })
   })

test/memory-storage.js

@@ -6,6 +6,11 @@ var deepEqual = require('deep-equal')
 var util = require('./_util')
 var multer = require('../')
 var FormData = require('form-data')
+var tiny0Size = util.fixtureSize('tiny0.dat')
+var small0Size = util.fixtureSize('small0.dat')
+var small1Size = util.fixtureSize('small1.dat')
+var mediumSize = util.fixtureSize('medium.dat')
+var largeSize = util.fixtureSize('large.jpg')
 
 describe('Memory Storage', function () {
   var upload
@@ -29,8 +34,8 @@ describe('Memory Storage', function () {
 
       assert.strictEqual(req.file.fieldname, 'small0')
       assert.strictEqual(req.file.originalname, 'small0.dat')
-      assert.strictEqual(req.file.size, 1778)
-      assert.strictEqual(req.file.buffer.length, 1778)
+      assert.strictEqual(req.file.size, small0Size)
+      assert.strictEqual(req.file.buffer.length, small0Size)
 
       done()
     })
@@ -104,8 +109,8 @@ describe('Memory Storage', function () {
 
       assert.strictEqual(req.files.tiny0[0].fieldname, 'tiny0')
       assert.strictEqual(req.files.tiny0[0].originalname, 'tiny0.dat')
-      assert.strictEqual(req.files.tiny0[0].size, 122)
-      assert.strictEqual(req.files.tiny0[0].buffer.length, 122)
+      assert.strictEqual(req.files.tiny0[0].size, tiny0Size)
+      assert.strictEqual(req.files.tiny0[0].buffer.length, tiny0Size)
 
       assert.strictEqual(req.files.tiny1[0].fieldname, 'tiny1')
       assert.strictEqual(req.files.tiny1[0].originalname, 'tiny1.dat')
@@ -114,23 +119,23 @@ describe('Memory Storage', function () {
 
       assert.strictEqual(req.files.small0[0].fieldname, 'small0')
       assert.strictEqual(req.files.small0[0].originalname, 'small0.dat')
-      assert.strictEqual(req.files.small0[0].size, 1778)
-      assert.strictEqual(req.files.small0[0].buffer.length, 1778)
+      assert.strictEqual(req.files.small0[0].size, small0Size)
+      assert.strictEqual(req.files.small0[0].buffer.length, small0Size)
 
       assert.strictEqual(req.files.small1[0].fieldname, 'small1')
       assert.strictEqual(req.files.small1[0].originalname, 'small1.dat')
-      assert.strictEqual(req.files.small1[0].size, 315)
-      assert.strictEqual(req.files.small1[0].buffer.length, 315)
+      assert.strictEqual(req.files.small1[0].size, small1Size)
+      assert.strictEqual(req.files.small1[0].buffer.length, small1Size)
 
       assert.strictEqual(req.files.medium[0].fieldname, 'medium')
       assert.strictEqual(req.files.medium[0].originalname, 'medium.dat')
-      assert.strictEqual(req.files.medium[0].size, 13196)
-      assert.strictEqual(req.files.medium[0].buffer.length, 13196)
+      assert.strictEqual(req.files.medium[0].size, mediumSize)
+      assert.strictEqual(req.files.medium[0].buffer.length, mediumSize)
 
       assert.strictEqual(req.files.large[0].fieldname, 'large')
       assert.strictEqual(req.files.large[0].originalname, 'large.jpg')
-      assert.strictEqual(req.files.large[0].size, 2413677)
-      assert.strictEqual(req.files.large[0].buffer.length, 2413677)
+      assert.strictEqual(req.files.large[0].size, largeSize)
+      assert.strictEqual(req.files.large[0].buffer.length, largeSize)
 
       done()
     })

test/reuse-middleware.js

@@ -5,6 +5,7 @@ var assert = require('assert')
 var util = require('./_util')
 var multer = require('../')
 var FormData = require('form-data')
+var small0Size = util.fixtureSize('small0.dat')
 
 describe('Reuse Middleware', function () {
   var parser
@@ -37,8 +38,8 @@ describe('Reuse Middleware', function () {
         req.files.forEach(function (file) {
           assert.strictEqual(file.fieldname, 'them-files')
           assert.strictEqual(file.originalname, 'small0.dat')
-          assert.strictEqual(file.size, 1778)
-          assert.strictEqual(file.buffer.length, 1778)
+          assert.strictEqual(file.size, small0Size)
+          assert.strictEqual(file.buffer.length, small0Size)
         })
 
         if (--pending === 0) done()