Express should throw an error if next() is called without any more middleware in the stack

[adamreisnz]

I've spent half an hour trying to debug why the client was getting a 404 response back, and it turns out the cause was because we didn't have a middleware in place to close the request (e.g. res.end()). In other words, the last middleware in the stack called next() but there wasn't any more middleware to handle the request, so Express sent a 404 response.

I don't think this should be the default behaviour, as this is clearly a case of programmer error. As such, I think express should throw an error instead, so that we can be made aware of the issue and add correct middleware in place or otherwise handle the error.

Silently responding with a 404 makes a developer hunt their own middleware stack first to see if and why they are terminating the request with a 404, which they aren't.

Thoughts?

[dougwilson]

Yes, we agree! This is how Express 5.0 will work. The issue with Express 4 design is that next function is the actual same function reference for every middleware, so in Express 4, there isn't a way to understand who called next; this design is changing in Express 5 to detect the error situation you are talking about.

I hope that helps!

[adamreisnz]

@dougwilson great, thanks for the clarification, look forward to work with Express 5.0!

[dougwilson]

Me too :) It's an often-requested foot gun to fix :) There are a few different candidate PRs for Express 5 to make the adjustment and I believe pillarjs/router#26 is the current front-runner, if you wanted to watch :)

[dougwilson]

I didn't mean to close the issue, BTW, crazy button placement. I'll re-open the issue since it's not technically done yet, besides some PRs. Of course, feel free to close it if you feel like your issue has been addressed, otherwise I'll auto close it when something is merged.

[adamreisnz]

That's fine, thanks. We can leave it open in case someone else stumbles on it, so they are aware of the status.

[hacksparrow]

@dougwilson if we are planning to change the behavior of next, we should add this item on the list at #2237

[dougwilson]

So just revisiting issues, I'm not sure how I actually interpreted the original post (perhaps didn't actually read through it before bundling with the other planned next() changes). The request is actually something we are not going to change. For example, I believe you are asking that given the following app:

var app = express()
app.use(function (req, res, next) {
  next() // [1]
})
app.listen(3000)

You are expecting that the call [1] should throw because you didn't place any middleware afterwards. Well, this is not really how Express works, as there is always a middleware at the end of the stack, which is the 404 middleware. If we threw there, it would break apps that actually need to 404 like

var app = express()
app.use(function (req, res, next) {
  if (!somePrecondition) return next() // i don't handle this
  // ... do things
})
app.listen(3000)

Especially considering that that app may or may not end up mounted at some point such that the trailing next() hands the control flow back to the parent app.

[adamreisnz]

Isn't it the responsibility of the app developers to throw a (custom) 404 error if needed?
I would think that's not the responsibility of Express, and that Express should warn developers about it instead, or perhaps return the 404 if you've configured Express to do so explicitly.

Currently, this implicit 404 was causing me to scratch my head a lot of times, because we issue manual 404's in specific cases, and I was spending a lot of time analysing them before realising it was Express sending the 404's, not our app.

In your second example app, instead of return next() one could simply do return next(new NotFoundError('I dont handle this')) to be more explicit, which is how we approach it most of the time. I guess I just don't like auto-magic behaviour as it's often hard to track.

[dougwilson]

It is not the responsibility of the developer to 404 themselves; that's an implicit operation of nothing handling the request. This enables transparent mounting of apps without them needing to be made to be mountable, since a 404 is just a fallthough, so it makes it really easy to compose multiple apps together with before before after after behavior, just like express.static. For example, consider the following:

var app = express()
app.use('/api', legacyApi)
app.use('/api', newApi)
// ... etc

That would be impossible if the legacyApi threw an error and didn't let 404s fall down.