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 :smile:

[raky291]

+1

don't exist any solution yet?

[ghost]

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.

[ghost]

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.

[ghost]

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.

[arendjr]

I have developed an alternative patch for this problem: #438

Please let me know your opinions.

[Hanruis]

@arendjr need adds

 req.on('abort', function () {
     pendingWrites.decrement()
     abortWithCode('CLIENT_ABORTED')
 })

I read node doc and find that 'aborted' means the request aborted by the server, while 'abort' means aborted by the client.

[arendjr]

@Hanruis: I agree the documentation seems confusing, but I've tested it with Node v6.9.1 and aborted is the one that's actually received.

~~But note how the docs say this (emphasis mine):~~

~~> Event: 'abort'#~~ ~~>~~ ~~> Added in: v1.4.1~~ ~~> Emitted when the request has been aborted by the client. This event is only emitted on the first call to abort().~~ ~~>~~ ~~> Event: 'aborted'#~~ ~~> ~~ ~~> Added in: v0.3.8~~ ~~> Emitted when the request has been aborted by the server and the network socket has closed.~~

~~But if the client aborts the request, there is no call to abort() at all. Whereas calling abort() makes sense if the server wants to abort the request. So I suspect someone mixed something up there...~~

~~Edit: I've reported this with the Node project as well: https://github.com/nodejs/node/issues/10787~~

Another update: So the confusion was actually that we're dealing with an IncomingMessage object, not an HttpClientRequest object. On IncomingMessage the event for client abortion is actually called aborted, and it has no abort event.

[Hanruis]

@arendjr thx~~

[sant123]

Any solve to this?

[nite1984]

Is there any progress on this issue? What's the current practice on removing files from canceled uploads ? Is removing the most recent file on the close event of the request the only solution yet ?

[drewwiens]

Seconding @Nite01 -- is there a standard way to prevent accumulating partially-uploaded files?

[sant123]

Since there's no fix from this issue and the alpha release is far away, I decided to move to Total.js. By the moment it has everything to work including proper file upload.

[yuwanlin]

I hava solved this problem absolutely by modify the souce code in make-middleware.js, the code like this, and I will explain what it does(my opinion, maybe not true). win10 multer1.3.0

// make-middleware.js
busboy.on('file', function (fieldname, fileStream, filename, encoding, mimetype) {
  req.on('close', function() {
    busboy.emit('finish');
  })
  // original code ..........
}

I add 3 line code solved the issue.

// make-middleware.js when a request is comming, busboy.on('file', cb) will be triggered. then req.pipe(busboy)(the last line of function multerMiddleware) will be trigged, too. Busboy is a Writable stream. when you cancle the request(refresh page or xhr.abort()), at the time the file upload some, not total. the 'close' event of req is triggered, but the 'finish' event of busboy will not be triggered(I guess). the busboy stream still exist. so the file is holded. so you can't delete it.

the 'finish' of busboy event will invoke some function.

1.busboy.on('finish', 
2.indicateDone()
3.done()
4.req.unpipe(busboy)

the step 4 is relative to the issue. because req.pipe(busboy) to hold the file, req.unpipe(busboy) free the file. so you can remove it from your folder.

---

I solve another problem - big file upload segment by segment. For example, the big file is 2GB, I will slice the big file to 2048 1MB file, the upload. If so, Multer will does these steps:

1.generate 1M's segment file
2.I will get 2048 1M's segment file

then I create a new file(2G), and append the 2048 segment. It's inefficient. so I add something.

1.If upload a (big) file segment by segment
2.create a big file 
3.write the segment to the big file directly.
// disk.js
finalPath = path.join(destination, targetFileName)
outStream = fs.createWriteStream(finalPath, {
  flags: 'r+',
  autoClose: true,
  start: start
})

It's more efficient.

May I pull request ?

[LinusU]

May I pull request ?

I'm not exactly sure how this approach will work? Doesn't it require the client to change as well?

---

re. the rest of the thread. I'm sorry that I haven't gotten the time to look at this properly. If it's of any interest, this should be fixed in the 2.0 alpha (see #399). The approach there is to create the file on disk, and then immediately unlink it while keeping the handle open. That will make the OS cleanup the file when the handle is closed, which will happen even if the entire application dies unexpectedly.

[hezedu]

@Ealhad You can get its name by storage. And req has Event aborted:

var multer = require('multer');
var tmpDir = require('os').tmpdir();
var path = require('path');

var storage = multer.diskStorage({
  destination: function (req, file, cb) {
    cb(null, tmpDir);
  },
  filename: function (req, file, cb) {
    req._originalname = file.originalname;
    cb(null, req._originalname);
  }
})

var upload = multer({storage}).single('file');

router.put('*',  function(req, res, next){

  req.on('aborted', () => {
    console.error('req aborted by client', path.join(tmpDir, req._originalname))
  })

  upload(req, res, function(err){
    if(err){
      return next(err);
    }
    res.end('ok');
  })
});

[ouzhou]

i see, but is there any handler to fix request aborted? @LinusU

[ouzhou]

if i aborted a request, multer will release the res?

[simonwep]

Well, got the same problem. Another solution would be adding a secondary array to the Request object:

const formdata = multer({
    storage: multer.diskStorage({
        destination: (req, file, cb) => cb(null, "/my/path"),
        filename(req, file, cb) {
            const name = generateName();
            req.fileNames.push(name);
            cb(null, name);
        }
    })
}).any();

module.exports = (req, res, next) => {

    // Holds custom-filenames
    req.fileNames = [];

    // Listen if request has been aborted
    req.on('aborted', () => {
        if (req.fileNames) {
            for (const fileName of req.fileNames) {
                // Delete files
            }
        }
    });

    formdata(req, res, next);
};

[hmmdeif]

Came across this issue today, nice to see that a 4 year old issue like this is unresolved. I think it's poor form to have 'fixed' it in the next version when the next version is many years in the making.

[Shinao]

I'm using @yuwanlin solution for now. Even though busboy is throwing me an error it's still better than having memory increase until the disk is full... If anyone else has any other method let me know.