No simple way to tell the client if an error occured about multer HOT 18 CLOSED

hi @qqilihq, I'd like to share my solution. please take a look:

app.use('/route', multer({
  dest: './upload/',
  limits: {
    fileSize: 1024*1024, // at most 1MB
  },
  // setting attributes of `file` is useful for passing info to the
  // next middleware
  onFileSizeLimit: function (file) {
    fs.unlink('./' + file.path); // delete the partially written file
    file.failed = true;
  }
}));

After the middleware above, we can do the trick as below:

app.use('/route', function(req, res, next){
  // take the 1st file for example, of course you can do a for-loop instead
  var files = req.files.yourfilename; // yourfilename is the field name defined in front-end
  var file = files[0];
  if (!file.failed) {
    // gotcha!
  }
}

I think this might help you. One more thing might be helpful: onFileUploadComplete does not mean success, any single file will fire this event one time, even if it runs over filesize limit.

from multer.

There isn't any onFileSizeLimit option any more so that will never log. You other code should work thought, however; I think that you are hitting bug #168.

from multer.

Why has onFileSizeLimit been removed? Is there an alternative to it that we could use?

from multer.

Could pass the request and response objects to the error handler?

router.use('/uploads', multer({
    dest: './uploads',
    limits: { files: 2 },
    onFilesLimit: function (req, res, next) {
      res.json(400, { err: 'Hit files limit' })
   }
  });

from multer.

@domharrington I don't thinkg you can access res and next, at least with current implementation of multer, but I don't see any reasons why it shouldn't pass those guys to the callback.

from multer.

From what I can see it should not pass next, as far as I know this should
only be called once for the middleware and should be/is currently handled
after all downloads are complete. res is meant to respond to the client and
most of the functions in it should also be only called once instead of
next, at least from what I know.

I see no reason to expose with of these, all that is needed is some access
to req so that in later middle ware we can tell what failed if anything.
On 6 Nov 2014 21:34, "Vitalij Kudresov" [email protected] wrote:

@domharrington https://github.com/domharrington I don't thinkg you can
access res and next, at least with current implementation of multer, but
I don't see any reasons why it shouldn't pass those guys to the callback.

—
Reply to this email directly or view it on GitHub
#72 (comment).

from multer.

I'm having the same issue; after the files limit has been reached, I cannot find any way to send back an error response, instead the request just times out.

Did you solve that problem? If there is anything I might have overlooked, please let me know.

from multer.

@jasson15: that was clever.

from multer.

Ok, I've had a chance to look at this issue a little.... What if we add an error parameter to onFinish and then go through the code and add places where processing seems to hang b/c there's no callback? A rewrite of some of those places would look like....

      busboy.on('partsLimit', function() {
        if (options.onPartsLimit) { options.onPartsLimit(); }
        onFinish(new Error('partsLimit reached: cannot upload more parts than specified in options.limits.parts'));
      });

      busboy.on('filesLimit', function() {
        if (options.onFilesLimit) { options.onFilesLimit(); } 
        onFinish(new Error('filesLimit reached: cannot upload more files than specified in options.limits.files'));
      });

      busboy.on('fieldsLimit', function() {
        if (options.onFieldsLimit) { options.onFieldsLimit(); }
        onFinish(new Error('fieldsLimit reached: cannot upload more fields than specified in options.limits.fields'));
      });

The onFinish would look like this:

      var onFinish = function (err) {
        if ( err ) {
          req.files = {}
          req.body = {}
          // onParseEnd is never reached b/c there's an error, so let's next the err
          return next(err);
        } else {

Or maybe instead of re-initializing req.files and req.body, we keep those as-is?

There's some other places in the code we might want to add next as a param, such as changeDest or rename. It would need to be tested that when next is called the underlying process from busboy doesn't continue to receive and read data.

from multer.

@jasson15 Thanks for the suggestion, I actually went for a similar solution finally. Btw., there is a pre-existing property truncated which you can use to check whether the transfer for a specific file was cancelled.

@jpfluger That looks like a good idea. Having that changes in a future release would be great. Thank you so far for your great work!

from multer.

@qqilihq wow, I'll give truncated a try, thanks!
@jpfluger It looks great! But here comes another issue: maybe there are several err passed to onFinish when more than 1 file reach the limits. Once onFinish gets an err, if we just let it pass the control to the next middleware, we might lose other err or other succesfully uploaded files.

This reminds me of how package q is designed in combination: it gives developers 2 choices: a) return the results only when everything is done well (Q.all), or b) return all of the good and bad results (Q.allSettled).

So, the onFinish might become:

var onFinish = function (err) {
    if (!options.allSettled) { // the developer only cares when everything done right
        if ( err ) {
          req.files = {}
          req.body = {}
          // onParseEnd is never reached b/c there's an error, so let's next the err
          return next(err);
        } else {
            ...
        }
    } else { // no matter what the results are, the developer wants all the results
        ...
    }
}

from multer.

@jasson15 I was having the same issue unable to send client error, and your solution worked for me, thanks!

from multer.

Error handling is rewritten to be much more straight forward in the newly released version 1.0.0. Please reopen if you feel that there is still room for improvements.

from multer.

@LinusU: I still don't get it to work. That's my implementation:

var upload = multer({
  dest: './uploads/',
  limits: {
    fileSize: 1024 * 1024 * 1
  },
  onFileSizeLimit: function(file) {
    console.log('does not get reported');
  }
});

app.post('/file', upload.single('file'), function(req, res, next) {
  // never touched when sending file larger than 1 MB
  res.status(200).send('OK');
});

app.use(function(err, req, res, next) {
  // throws sometimes once, but with 2nd request app hangs somewhere and does not respond anymore
  console.error(err.stack);
  res.status(500).json({error: err.message});
});

Any idea?

from multer.

Yes, standard express error handling. See Error handling in the readme...

from multer.

I read the readme, and the standard error handling is fine but has to be implemented in every route individually - the events allowed for a shared instance of multer to be used in multiple routes.

from multer.

Would you be open to a PR that re-implemented the events?

from multer.

It doesn't have to be implemented for each route:

app.use(function (err, req, res, next) {
  // all errors, from all routes, will be sent here \o/
})

from multer.