Can't set headers after they are sent : multiple middleware executions concurency

Question

In my main express.js config file, I use two custom error middleware functions:

const error = require('../middlewares/error');

// catch 404 and forward to error handler
app.use(error.notFound);

// if error is not an instanceOf APIError, convert it.
app.use(error.converter);

I use boom to unify error messages. this is my error middleware:

module.exports = {
  /**
   * Error responder. Send stacktrace only during development
   * @public
   */
  responder: (err, req, res, next) => {
    res.status(err.output.payload.statusCode);
    res.json(err.output.payload);
    res.end();
  },

  /**
   * If error is not a Boom error, convert it.
   * @public
   */
  converter: (err, req, res, next) => {
    if (env !== 'development') {
      delete err.stack;
    }
    if (err.isBoom) {
      return module.exports.responder(err, req, res);
    }
    if (err.name === 'MongoError' && err.code === 11000) {
      const boomedError = boom.conflict('This email already exists');
      boomedError.output.payload.stack = err ? err.stack : undefined;
      return module.exports.responder(boomedError, req, res);
    }
    const boomedError = boom.boomify(err, { statusCode: 422 });
    return module.exports.responder(boomedError, req, res);
  },

  /**
   * Catch 404 and forward to error responder
   * @public
   */
  notFound: (req, res) => {
    const err = boom.notFound('Not Found');
    return module.exports.responder(err, req, res);
  },
};

My problem is, when I make a "register" action with an existing email, the responder() is executed three times. One for my boom.conflict error, but then also one for "not found". (even though I've done res.end().

This is the register logic:

exports.register = async (req, res, next) => {
  try {
    validationResult(req).throw();
    const user = new User(req.body);
    const token = generateTokenResponse(user, user.token());
    const userTransformed = user.transform();
    user.tokens.push({ kind: 'jwt', token });
    user.activationId = uuidv1();
    await user.save();
    res.status(httpStatus.CREATED);
    sendValidationEmail(user.activationId);
    return res.json({ user: userTransformed });
  } catch (err) {
    return next(converter(err, req, res, next));
  }
};

user.save() triggers this by the way:

userSchema.pre('save', async function save(next) {
  try {
    if (!this.isModified('password')) return next();

    const rounds = env === 'test' ? 1 : 10;

    const hash = await bcrypt.hash(this.password, rounds);
    this.password = hash;

    return next();
  } catch (err) {
    return next(converter(err));
  }
});

Answer 1

Calling res.end() just tells the response stream that you're done sending data. You don't need that in this case because calling res.json() will do it for you.

However, that isn't the same as telling Express that you're done with handling the request. Just because you've sent a response doesn't necessarily mean you've no work left to do.

The way you tell Express that you've finished is by not calling next(). Express assumes you've finished by default and will only continue executing the middleware/routing chain if you call next().

So this line:

return next(converter(err, req, res, next));

should just be:

converter(err, req, res, next);

Likewise your other call to converter() shouldn't be calling next() either.