Skip to content

HTTPS clone URL

Subversion checkout URL

You can clone with HTTPS or Subversion.

Download ZIP

Loading…

Add a way to detect if render has been called #1334

Closed
dominykas opened this Issue · 5 comments

3 participants

@dominykas

Practical use case: a 404 handler inside a wildcard route should only render the 404 if there was no response set earlier (i.e. res.headerSent is empty). In case of redirect()/send() - this works fine. In case of render(), depending on the templating system, the actual send() may happen in the next tick. This means that if (by accident) next() is called after render() - the wildcard handler will be called, however it will not have a way to detect if the request was actually handled (because the template might be read from the file system asynchronously).

Sample code:

var app = express();
app.set("view engine", "hbs");
app.set("views", __dirname);

app.get("/", function(req, res, next){
    console.log("/ handler called")
    res.render("core/content", {layout:"",content:"Content"});
    next();
});
app.get("/*", function(req, res){
    console.log("Wildcard handler called");
    assert(typeof(res.headerSent) === "undefined");
});

supertest(app)
    .get("/")
    .expect(200)
    .expect("Content")
    .end(function(e, res){
        console.log("Request completed");
        assert(e === null);
    });

For the time being I'm overriding the render() method and set a renderCalled = true inside, but it'd be nice to have this built in.

@eknkc

I was bitten by this too, had multiple calls to the callback function in diiferent situations. Forgetting return in a return next(); statement and stuff. That's hard to debug :)

I'd say; do not rely on a safeguard to catch it and recover.. Make it sure that no next is called "by accident". If it is, it's a bug and should be fixed, not hidden by such hacks. These will cause more problems to pass by unnoticed.

@dominykas

My initial line of thought was also "write software without bugs" and "don't make mistakes" - but as you say - it's hard to debug. I'm thinking of this less as a "safeguard to recover", but more of a "in case it happens - we can at least log that it did". The problem is that if you fail to check whether render() was called, at some point the app craps out with an exception "headers already sent" - and we don't want that...

As for making sure next() is not called "by accident" - in the situation I'm in, I actually want to have some "post route handler" things to be done (at app level, not route level) - and the only way to achieve it is next(). So, after lots of pondering, I decided that "always call next()" is a simpler rule to follow than "make sure to call either render or next, but never both".

@eknkc

For post route stuff you can use a middleware, push it to app stack somewhre before router, and register an event listener for "header" event. No need to call next, as every single request will trigger the event in app level.

responseTime middleware is a perfect example: http://www.senchalabs.org/connect/responseTime.html

@dominykas

If only it were that simple :)

I was also thinking about a way of deciding what to actually do about rendering at the "post-routing" stage. Use case: for all actions, load some data and set the view name. Post-routing: based on accepts headers either render(viewName) as HTML or json(publicData). Yes, this does mean that render() on it's own is not an issue for me if I go this route. Yes, I'm aware of the won't fix for this bug in Chrome: http://code.google.com/p/chromium/issues/detail?id=94369.

Still, at the very heart of this is the fact that render(), unlike send(), json(), jsonp(), redirect() is not detectable after it's been called. I guess download() and sendfile() also have the same issue - I haven't tried. As much as I'll probably regret some of the decisions I'm making now (still experimenting) - there needs to be a simple way to determine "somebody already did something to the response, so don't bother doing anything".

There's also the case of detecting "all possible middlewares executed, but nobody bothered to send anything - the request will just be dangling until it times out".

Are there any other hooks I missed that I could use for this?

@jonathanong

so from this convo, i see that the two problems you wish to solve are:

  • double callbacks, ie headers are already sent. really, it's the developers responsibility to make sure only one response is sent and that error is telling you that you did something wrong. express could make debugging easier, but that would considerably complicate things. what one be nice is a linter that goes through your middleware and makes sure only one next or res.somethingthatsendsaresponse() is executed.
  • "all possible middlewares executed, but nobody bothered to send anything - the request will just be dangling until it times out" - this, again, is the developers responsibility. this is easy to check - if your request times out, then you messed up your route.

what you could do is:

res.render(layout, options, function (err, html) {
  if (err) return next(err);
  if (res.headersSent) throw new Error('Developer error! Headers already sent!');
  res.send(html)
})

but I don't think Express will do anything to mitigate this problem. this is just a part of using Express/node.

feel free to provide suggestions, though. things should be easier with generators.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Something went wrong with that request. Please try again.