1.x #358

Closed
wants to merge 1 commit into
from

Projects

None yet

3 participants

@zidadi
zidadi commented Sep 7, 2011

Handle the case where middleware wrongly call next() after sending out response and the case where all middleware call next() and no one ends the request.

@zidadi zidadi handle the case where middleware wrongly call next after sending out …
…response and the case where all middleware call next and no one ends the request.
882db89
@Raynos
Raynos commented on 882db89 Sep 7, 2011

No. You shouldn't hold hands for people who call next after sending a response. This leads to the framework "correcting your mistakes for you". This is always wrong.

@tj
Member
tj commented Sep 7, 2011

yeah I have to agree with @Raynos, I do think we can improve the somewhat useless errror message(s) you get from Node when this happens, but I agree that regardless this is a hard error that should be fixed in the application

@zidadi
zidadi commented Sep 7, 2011

@Raynos: Sorry, I don't make my message very clear. The problem is when the middleware call next() after sending the response will crash the connect. Because connect will try to send 404 to the client which connect should not do, node throws an error to report the response has already ben sent. So basically a wrong middleware may crash the whole connect which makes connect a bit subtle to tolerant wrong middleware.

It is the first time I use connect. Maybe I misunderstand some design concepts of connect.

@tj
Member
tj commented Sep 7, 2011

well the bottom line is people are still writing middleware incorrectly when that's the case. if you respond there is no need to next() as that would imply that the subsequent middleware may respond as well

@Raynos
Raynos commented Sep 7, 2011

@zidadi and this is an issue that should be fixed in broken middleware.

Your not going to run broken / buggy middleware in production are you? That's just silly. We don't need to aggressively patch connect to deal with these issues. That's called swallowing errors.

And yes a wrong middleware can crash the entire server, you should handle it like any other uncaught exception, mark it a bug, log it and reboot the entire server.

@tj
Member
tj commented Sep 7, 2011

I think the main problem is that people get confused by Node's error messages of "cant set headers after sent" or whatever it is, no one seems to interpret that as "oh I'm responding several times", so I think error messages can be improved a bit

@zidadi
zidadi commented Sep 7, 2011

Yes, this makes sense to me now. But we should probably need to throw an error to the middleware instead of sending out 404 to the client if the response has already been sent?

Something like this:

if (!layer || res.headerSent) {
      // but wait! we have a parent
      if (out) return out(err);

      // error
      if (err) {
        var msg = 'production' == env
          ? 'Internal Server Error'
          : err.stack || err.toString();

        // output to stderr in a non-test env
        if ('test' != env) console.error(err.stack || err.toString());

        // unable to respond
        if (res.headerSent) return res.socket.destroy();

        res.statusCode = 500;
        res.setHeader('Content-Type', 'text/plain');
        res.end(msg);
      } else if(!res.headerSent){
        res.statusCode = 404;
        res.setHeader('Content-Type', 'text/plain');
        res.end('Cannot ' + req.method + ' ' + req.url);
      } else{
        throw new Error("next called after sending messages");
      }
      return;
    }
@zidadi
zidadi commented Sep 7, 2011

@visionmedia: yes, the error message was a bit confusing to new comers like me ;p

@Raynos
Raynos commented Sep 7, 2011

@zidadi I agree that throwing an error is a suitable thing to do however you should still not be silently destroying that socket in the if (err) { ... } block

@zidadi
zidadi commented Sep 7, 2011

@Raynos: that was originally in the 1.x code. ;p I will post a modified patch tonight after working based on what we discussed. Any other suggestion? ;)

@zidadi zidadi closed this Sep 7, 2011
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment