double next call leads to cryptic error when req.params is called again

[defunctzombie]

The following snippet shows the failure scenario. A user is calling next() twice in some middleware (due to whatever) and then as a result, the req.params(...) call in a route handler fails with a less than helpful error message. I can't think of a reason for double calling next() to be supported so maybe we can error out quicker or with something more helpful seems sane and safe for users.

var express = require('./');
var supertest = require('supertest');

var app = express();

app.use(function(req, res, next) {

    // double callback here leads to cryptic error (see below)
    // I think there should be better protection against invoking twice, thoughts?
    next();
    next();
});

app.get('/:product', function(req, res) {
    req.params('foobar');
    res.send('hello');
});

supertest(app)
.get('/foo')
.end(function(err, res) {
    console.log(res.text);
});

/*
TypeError: Property 'params' of object #<IncomingMessage> is not a function
    at /Users/dz/projects/express/fail.js:12:9
    at Layer.handle [as handle_request] (/Users/dz/projects/express/lib/router/layer.js:95:5)
    at next (/Users/dz/projects/express/lib/router/route.js:131:13)
    at Route.dispatch (/Users/dz/projects/express/lib/router/route.js:112:3)
    at Layer.handle [as handle_request] (/Users/dz/projects/express/lib/router/layer.js:95:5)
    at /Users/dz/projects/express/lib/router/index.js:277:22
    at param (/Users/dz/projects/express/lib/router/index.js:349:14)
    at param (/Users/dz/projects/express/lib/router/index.js:365:14)
    at Function.process_params (/Users/dz/projects/express/lib/router/index.js:410:3)
    at next (/Users/dz/projects/express/lib/router/index.js:271:10)
*/

[dougwilson]

Are you sure that error has anything to do with double-calling next()? req.params has never been a function at all, so you'd always get TypeError: Property 'params' of object #<IncomingMessage> is not a function calling req.params('something') regardless of next()...

[dougwilson]

Example:

$ cat test.js
var express = require('./');
var supertest = require('supertest');

var app = express();

app.get('/:product', function(req, res) {
    req.params('foobar');
    res.send('hello');
});

supertest(app)
.get('/foo')
.end(function(err, res) {
    console.log(res.text);
});

$ node test.js
TypeError: Property 'params' of object #<IncomingMessage> is not a function
    at test.js:7:9
    at Layer.handle [as handle_request] lib\router\layer.js:95:5)
    at next (lib\router\route.js:131:13)
    at Route.dispatch (lib\router\route.js:112:3)
    at Layer.handle [as handle_request] (lib\router\layer.js:95:5)
    at lib\router\index.js:277:22
    at param (lib\router\index.js:349:14)
    at param (lib\router\index.js:365:14)
    at Function.process_params (lib\router\index.js:410:3)
    at next (lib\router\index.js:271:10)
TypeError: Property &#39;params&#39; of object #&lt;IncomingMessage&gt; is not a function

[defunctzombie]

Hm, let me double check that. Unfortunately I lost the original stack trace so was trying to remember from my sticky-notes how to reproduce. It was deff something around double callbacks and params. Let me dig more.

[defunctzombie]

@dougwilson ok figured out how to reproduce the issue I saw. It had to do with multiple next calls and params handling. See below.

var express = require('./');
var supertest = require('supertest');

var app = express();

app.use(function(req, res, next) {
    // double callback here leads to cryptic error (see below)
    // I think there should be better protection against invoking twice, thoughts?
    next();
    next();
});

app.param('foo_id', function(req, res, next, id) {
    // when this next is called after response is sent
    // then there is a cryptic error
    setTimeout(next, 1000);
});

app.get('/:foo_id', function(req, res) {
    res.send('hello');
});

supertest(app)
.get('/foo')
.end(function(err, res) {
    console.log(res.text);
});

/*
/Users/dz/projects/express/lib/router/index.js:392
    paramCalled.value = req.params[key.name];
                                  ^
TypeError: Cannot read property 'foo_id' of undefined
at paramCallback [as _onTimeout] (/Users/dz/projects/express/lib/router/index.js:392:35)
    at Timer.listOnTimeout [as ontimeout] (timers.js:112:15)
*/

Has to do with internal express params handling clearing the params.

[defunctzombie]

I still separately think that we could help the user better when calling next twice (as this is related).

[dougwilson]

Gotcha. There are people likely grand-fathered into doing this (looking through issues here, are are people who even posted code calling next() twice to abuse the system. This means that we'll need to address this in 5.0, which is coming up really, really fast (last week of this month), so the decision, PR, etc. needs to get moving very quickly. 5.0 has already deleted all the router code, so we should probably continue this discussion in an issue over at https://github.com/pillarjs/router

[defunctzombie]

Do you want this fixed up in the router? I can create a PR for the behavior around protecting against double next. Do you think there is a valid/good/reasonable use case for supporting double next? My experience has been that it leads to more sadness than anything else but would love to hear what clever hacks are possible with double next that are not otherwise.

[dougwilson]

Yes, we should fix this up for router 2.0/Express 5.0. I had, a long time ago, jotted down some thoughts here: senchalabs/connect#1042

[defunctzombie]

An issue I see is that it would be nice to notify the user of a double next callback call, however we can't do that by passing the error to next() since we ourselves would be double calling next. What I think would be reasonable in the typical node approach is to emit an error when this happens. This error would also contain some helpful info on tracking down the double next call. Main problem here is router and express both are not event emitters iirc and thus this would be more changes.

Alternative is to throw but that just seems like a bad time. I would be more inclined to make them event emitters and let the user handle routing errors that can't be tied to a particular request.

Final alternative is to make the error and pass it to next(err) and document that the specific error type or field on the error that indicates this is a double callback error and that the developer should not attempt to resend a response. This requires more manual hand holding and messing up over the emitter approach but is a possibility if we are anti-emitter.

[dougwilson]

Yea, but I have always thought that it probably should be OK to "double call next", but only if you have an error to provide. The main reason is not only because it's the only mechanism to provide errors upstream using the current model, but also because you could have theoretically called "next()" all normal-like, but are still doing some kind of async processing related to said request and want to "next(err)" an error about it.

[defunctzombie]

Fair. One last alternative I thought about is to emit the error on request. Let me know which approach you favor and I will PR that against the new router module.

[dougwilson]

I'm up for whatever, and of course, it's always easier to just accept a PR :) As for emitting an error event, I'm a little -1 on that, because it would mean the process will likely crash unless everyone explicitly listens to the event.

[sjanuary]

pillarjs/router#26 for reference.