Multer: Multer doesn't seem to be forwarding error to Express

[atsepkov]

I'm using multer version 1.4.2. According to https://github.com/expressjs/multer#error-handling, multer should automatically forward the error to Express. I implemented a default error handler in express like this:

    app.use((err, req, res, next) => {
        let error = err.message;
        if (err instanceof BadRequest) {
            res.status(400);
        } else if (err instanceof NotFound) {
            res.status(404);
        } else if (err instanceof NotAuthorized) {
            res.status(401);
        } else {
            res.status(500);
            console.error(err.stack);
            error = `Server-side error`;
        }
        res.send({
            error: error
        })
    })

This handler is correctly invoked by my own logic when I throw an error. I initialized multer as follows:

const storage = multer.diskStorage({
    destination: (req, file, cb) => {
        cb(null, 'uploads')
    },
    filename: (req, file, cb) => {
        cb(null, generateFilename(req.query));
    }
});

My method generateFilename throws an error when query is not in proper format. Instead of being forwarded to my default error handler in Express, however, the error causes the entire server to crash from uncaught exception.

[jonchurch]

The filename function communicates its errors using the cb:

filename: (req, file, cb) => {
        try {
            cb(null, generateFilename(req.query));
        } catch(err) {
            cb(err)
        }
    }

If you're throwing an exception and not catching it somewhere, then crashing the app is expected. I know that is likely frustrating, because Express attempts to catch exceptions for you so it feels a bit out of step, but Multer doesn't attempt to catch any thrown exceptions. I'm not exactly sure that multer should try and catch exceptions for you, but it does seem strange.

Come to think of it, I'm not sure exactly why the exception wouldn't bubble up to Express, since this should be happening in a middleware. Actually, I think it's because this logic plays out in an event listener.

@LinusU Do you have an opinion on this? We could add a try/catch and abortWithError from busboy's file event handler to mediate this.

[jonchurch]

cc/ @LinusU bc I'm not sure that edits actually ping someone ☝️

[atsepkov]

Thanks, the catch block worked like a charm. So in the README section I referenced, where it says When encountering an error, Multer will delegate the error to Express, I misunderstood that to mean that like in Express, any error thrown within multer will propagate to the default error handler. Should that README section be clarified (perhaps with the above example in addition to the one already there showing intercepting multer errors from express)?

[jonchurch]

Im not sure this example is the best way to communicate the point, and I dont recall reports of this issue before so Im not certain how common it is. I interpret an error to be different from an exception, but the docs could always use improvement.

I think this is very interesting though, and we should consider adding a try/catch internally so thrown errors are passed off to Express. Im on the fence essentially about whether this is a bug or not, and should be addressed in the docs or in the source.

I think if you expect your code might throw, you should make sure you’re prepared to catch it. But since Express middleware sets up the contract that it will catch for you, its logical to expect multer to conform to that.