proto.js may try and set headers when they have already been sent #767

Closed
eric-wieser opened this Issue Mar 23, 2013 · 9 comments

Comments

Projects
None yet
5 participants

This line and this line are incompatible:

if (!layer || res.headerSent) {
    // ...
    res.setHeader(...)
}

Since res.setHeader will error with the message

Error: Can't set headers after they are sent.

if res.headerSent is non-empty

I'm not entirely sure why I'm hitting this case, but either way, the code should not even attempt to send headers if it knows the header has already been sent. (right?)

Member

tj commented Mar 23, 2013

I've never encountered this without it being a user error, which is unfortunately pretty easy with node

I think I know what causes this to be triggered. If a piece of middleware calls next() twice, then the content can end up being sent twice. I'd be inclined to fix this by either:

  • Making invoking next twice from the same middleware trigger an error
  • Adding an if (res.headerSent) return req.socket.destroy(); like on this line
  • Doing both of the above

Yep, this is being caused by another library calling my handler twice (danwrong/restler#112), which in turn calls next() twice. Preventing double calls of next would at least make this easier to debug. Here's the terrible implementation I threw together to debug my problem:

var intNext = next;
var safeNext = function() {
  intNext.apply(this, arguments);
  var stack = new Error().stack
  intNext = function() {
    throw Error("Called twice", {first_at: stack});
  };
}

debug('%s', layer.handle.name || 'anonymous');
var arity = layer.handle.length;
if (err) {
  if (arity === 4) {
    layer.handle(err, req, res, safeNext);
  } else {
    next(err);
  }
} else if (arity < 4) {
  layer.handle(req, res, safeNext);
} else {
  next();
}
Member

tj commented Apr 1, 2013

closing since it's a user error

tj closed this Apr 1, 2013

While this is a user error, it is one that is very hard to track down. It would be better if this threw an error when invoked twice.

100% agree with Eric. While this obviously isn't Connect's fault, is there any harm in at least making this easier to debug?

+1 to Eric this should throw error for debug reason.

Contributor

jonathanong commented Apr 27, 2013

the only way to do this reliably is to overwrite res.write and res.end as well as intercepting all the next calls, which i don't think is worth it just so debugging is easier. eric's case only handles the times people call next twice, but a lot of times people are calling res.write or res.end as well. For example:

function (req, res, next) {
  async(function (err) {
    if (err) next(err);
    res.end('done')
  })
}

may issue be resolved by middleware or monkey patching connect?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment