multer icon indicating copy to clipboard operation
multer copied to clipboard

Close write stream on connection error (FD leak on incomplete uploads)

Open Khez opened this issue 4 years ago • 1 comments

We have seen a large number of open FD's when using express with multer.

After investing, we have found the issue to be aborted uploads.

It seems the upload FD is not being closed if the client aborts the request mid-way.

How To Reproduce

From the readme (adapted to a single field):

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

var app = express();
var cpUpload = upload.fields([{
    name: 'avatar',
    maxCount: 1
}]);

app.post('/upload', cpUpload, function (req, res, next) {
    res.send('ok');
});

app.listen(8081, () => {
    console.log('Listening');
});

Using whichever client, upload a file and abort it during transit:

# make sure the file is somewhat large
# and don't forget to abort before getting the response
curl -vvv 'http://localhost:8081/upload' -F 'avatar=@file' --limit-rate 1M;

Finally using lsof on the node process, you should see a FD leaked to the uploads directory

Related(?)

  • https://github.com/nodejs/node/issues/7732
    • Really old bug, closed without being solved, seems like this is is the "core" issue
  • https://github.com/fastify/fastify-static/issues/116#issuecomment-655529420
    • Similar reproduction steps, but on downloading files

Khez avatar Jan 14 '21 11:01 Khez

I'm saddened to see this ignored. This was a very visible issue and with a clear solution.

We attempted a similar fix in production, but we still had some FD Leaks, uncertain why. It looked something like:

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

var app = express();
var cpUpload = upload.fields([{
    name: 'avatar',
    maxCount: 1
}]);


app.post('/upload', function (req, res, next) {
    // Userland fix attempt for https://github.com/expressjs/multer/pull/971
    (req.connection || req.socket).on('error', (error) => {
        console.log('Connection Error Detected', error.message || error);
        req.emit('end');
    });

    cpUpload(req, res, function(err) {
        console.log(err || 'Upload Done');
        res.send('ok');
    });
});

app.listen(8081, () => {
    console.log('Listening');
});

Due to this PR gathering dust and the fact that we still had FD leaks, we ended up moving to formidable. Haven't had any leaks since then.

Khez avatar Mar 16 '21 11:03 Khez