file is not removed if upload is canceled

[thewilli]

Consider this minimal working example (Multer v1.1.0 on Windows 7, OS X 10.11 and Ubuntu 14.04 with node v4.2.1):

var express = require('express')
var multer  = require('multer')
var upload = multer({ dest: 'uploads/' })
var app = express();

app.post("/upload", upload.single("file"), function(req, res, next){
    res.end("uploaded to: " + req.file.filename);
});
app.listen(80);

and the following test script

# create large test file
$ dd if=/dev/urandom bs=2G count=1 of=upload
# upload
$ curl -F "file=@upload" http://localhost/upload
# upload again, but cancel before upload is finished
timeout -s SIGKILL 2s curl -F "file=@upload" http://localhost/upload

Expected behavior:
Only the first file is persisted, the second is removed after the upload got canceled.

Actual behaviour:
Both files are stored in the uploads folder

[futurliberta]

Hi,

Did anyone find a solution ?

I've done an express middleware that removes remaining files but when I cancel a xhr request, req.files looks like that:

[ { fieldname: 'files[]',
    originalname: 'xxxxxxxxxxxxxxxxxxx',
    encoding: '7bit',
    mimetype: 'application/octet-stream',
    destination: 'xxxxxxxxxxxxx/uploads/',
    filename: '35226762ecf04fb57923fce15dfca8d4',
    path: 'xxxxxxxxxxxxxxxxxxxx\\35226762ecf04fb57923fce15dfca8d4',
    size: 1877316 },
  { fieldname: 'files[]' } ]

And one file is remaining because I can't get its path.

[sant123]

Has been fixed this issue? I mean, just asking 😄

[raky291]

+1

don't exist any solution yet?

I thought perhaps the error handling code from the README was the solution, but it seems the callback is not called if the request is aborted.

This post might help.

EDIT :
This is what I came up with :

const upload = multer({dest: __dirname + '/../../resource/'}).single('file')
app.post('/upload', (req, res) => {

    req.on('close', () => {
        console.error('req aborted by client')
        // delete most recent file ?
    })

    upload(req, res, () => {
        // do normal stuff
    })
})

We know when if the upload gets canceled. However the partially uploaded file still can't be deleted, since only multer knows its name ; deleting the most recent file could be a temporary solution.

[sant123]

@Ealhad But in the request "close" event, multer could notice the request has been canceled and take the path from the object {dest: __dirname + '/../../resource/'} parameter in order for deleting the file.

I see this implementation is really simple, but... ¿am I missing something in order to fix this issue? I mean, add this fix in the next version of multer.

Well @sant123 I tried messing with the code a bit, I couldn't find the name of the file. End of lib/make-middleware.js :

// that's what I added
req.on('close', function() {
    console.err('req aborted')
})

busboy.on('error', function (err) { abortWithError(err) })
busboy.on('partsLimit', function () { abortWithCode('LIMIT_PART_COUNT') })
busboy.on('filesLimit', function () { abortWithCode('LIMIT_FILE_COUNT') })
//...

However from there, I don't know what to do. All I can find is an empty uploadFiles array.

I tried to abortWithError as well, since it handles file deletion according to the code, but in that particular case it didn't do anything.

[sant123]

Now I see your point @Ealhad.... Mmmm something it'd help is storing the full path in the req object before processing the file, then in a case of an exception the req object will have our path and then delete the file.

Well, I really can't figure how to do it. I am unable to get a filename until the call to storage._handleFile (make-middleware.js), although the file does exist in the system.

[adskjohn]

Any progress on this issue?

@LinusU Any best practice on removing files from canceled uploads? It seems the storage callbacks never fire in this case as the stream never has an error or finish event fired.

[adskjohn]

I opened a PR #429 that fixes the issue for me, but it would be great if someone more versed in multer could give opinions on whether the solution needs to be more elegant etc.

[sant123]

@adskjohn you were close, really close!. What I did is only modify the "disk.js" file adding these lines after the "finish" stream event:

outStream.on('finish', function () {
  cb(null, {
    destination: destination,
    filename: filename,
    path: finalPath,
    size: outStream.bytesWritten
  })
})
// The new code....
req.on('close', function() {
  outStream.close();
  fs.unlink(finalPath, cb);
})

This should solve the issue 😄

[adskjohn]

@sant123 I started there, just using the storage removeFile function instead, but it still triggers the outStream.on('finish'...). I wasn't sure how clean it was to allow it to report success back out to the rest of the middleware, if it doesn't have any potential issues then great.

In either case, it would be great to get a solution into the package so it can be used in a deployed production environment. If you want to PR your solution in I'd be happy with either of them making it up.

[sant123]

@adskjohn we need to know what exactly does "cb" inside the "finish" event. The tests I made tells me there's no problem when this "cb" gets called. Now that you have a PR just modify it, there's no reason to create another one if the solution is the same 😄. You can confirm the file is just removed when the request is canceled.

[adskjohn]

Yeah, the issue I had with it is that the finish cb is called and passes back invalid file information, because the file no longer exists when the cb is called. In my case the termination of the steam in the disk writing just closes the stream and returns the information about an existing (though probably corrupt/incomplete) file. Another potentially more proper approach would be to use your approach, but have the finish event check if the file exists and throw an error if not.

Having _handleFile return successfully when no file ends up on the disk seems like it would be an unintended result.