Expand on why its not recommended as a "top-level" middleware

[expressjs]

Expand on why its not recommended as a "top-level" middleware

[dougwilson]

Thanks :) I will try to write up something meaningful to put in the README.

[dougwilson]

The documentation should provide better information on this note, now.

[basarat]

Thanks for the clarification @dougwilson!

But I think it is wrong to say

once this module passes the request to the next middleware

Since it only passes control onto the next error middleware. demo:

var express = require('express');
var timeout = require('connect-timeout');

var app = express()
    .use(timeout(5000))
    .use(function (req, res, next) {
        // simulate a hanging request by doing nothing
    })
    .use(function (req, res, next) {
        // no need for checking as never called
        console.log('never called!'); 
    })
    .use(function (error, req, res, next) {
        console.log('called!', req.timedout);
        res.end('timed out');
    })
    .listen(3000);

And in fact this example is wrongish

var express = require('express');
var timeout = require('connect-timeout');

// example of using this top-level; note the use of haltOnTimedout
// after every middleware; it will stop the request flow on a timeout
var app = express();
app.use(timeout(5000));
app.use(bodyParser());
app.use(haltOnTimedout);
app.use(cookieParser());
app.use(haltOnTimedout);

// Add your routes here, etc.

function haltOnTimedout(req, res, next){
  if (!req.timedout) next();
}

app.listen(3000);

As haltOnTimedout is never called and doesn't need to check req.timedout since it is not an error handling middleware.

But I do get the point that there may be some middleware that might already be doing something async and when this something async comes back the req might have timedout. But even then haltOnTimeout is unnecessary.

[dougwilson]

Since it only passes control onto the next error middleware

I think you mis-understand. The control was passed to the next middleware, i.e. the middleware that "hung", which clearly is not an error middleware. Add a console.log in there if you want to see...

As haltOnTimedout is never called

Did you try the example out? Perhaps adding a console.log in the haltOnTimedout function? You'll see it clearly is called.

[dougwilson]

Here is a concrete example, if you want to wrap your head around the problem, since it doesn't seem you believe me:

var express = require('express');
var timeout = require('connect-timeout');

var app = express()
app.use(timeout(5000))
app.use(function (req, res, next) {
  // simulate database action that takes 6s
  setTimeout(function(){
    req.thingie = 'db_value'
    next()
  }, 6000)
})
app.use(function (req, res, next) {
  // send the user the db thingine
  process.nextTick(function(){
    // i hope i don't crash here, since i didn't check req.timedout
    res.send(req.thingie)
  })
})
app.listen(3000);

Result of a request coming into the above app:

Error: Response timeout
    at generateTimeoutError (node_modules/connect-timeout/index.js:62:13)
    at IncomingMessage.<anonymous> (node_modules/connect-timeout/index.js:71:15)
    at IncomingMessage.EventEmitter.emit (events.js:95:17)
    at null._onTimeout (node_modules/connect-timeout\index.js:35:11)
    at Timer.listOnTimeout [as ontimeout] (timers.js:110:15)

http.js:689
    throw new Error('Can\'t set headers after they are sent.');
          ^
Error: Can't set headers after they are sent.
    at ServerResponse.OutgoingMessage.setHeader (http.js:689:11)
    at node_modules\express\lib\application.js:156:9
    at node_modules\express\lib\router\index.js:140:5
    at node_modules\express\lib\router\index.js:265:10
    at next (node_modules\express\lib\router\index.js:165:14)
    at null._onTimeout (test.js:10:5)
    at Timer.listOnTimeout [as ontimeout] (timers.js:110:15)

[basarat]

Here is a concrete example,

👍 I believe you. And I think this should be the example in the docs. This is what I am saying as well

But I do get the point that there may be some middleware that might already be doing something async and when this something async comes back it calls next and the req might have timedout

[dougwilson]

I'm still not sure what you want. The other two examples in the README will randomly cause a timeout and they show how to correctly check for the condition. Are they not enough? It's there my example above came from.

[basarat]

Are they not enough?

sorry. My bad. Thanks for you patience!

[dougwilson]

OK, but do tell me if it's not a good enough example/provide me an example, please. bodyParser is async, by the way.

[basarat]

Having a bit of a weird behavior, the following server still crashes :

var express = require('express');
var timeout = require('connect-timeout');

var app = express()
    .use(timeout(1000))
    .use(function (req, res, next) {
        // simulate database action that takes 2s
        setTimeout(function () {
            next(); // ERROR here 
        }, 2000)
    })
    .use(haltOnTimedout)
    .listen(3000);


function haltOnTimedout(req, res, next) {
    if (!req.timedout) next();
}

It seems that calling next after the timeout has occurred (and response terminated) is itself an error and haltOnTimedout isn't called (and not helpful):

http.js:691
    throw new Error('Can\'t set headers after they are sent.');
          ^
Error: Can't set headers after they are sent.
    at ServerResponse.OutgoingMessage.setHeader (http.js:691:11)
    at node_modules\express\lib\application.js:156:9
    at node_modules\express\lib\router\index.js:140:5
    at node_modules\express\lib\router\index.js:265:10
    at next (node_modules\express\lib\router\index.js:165:14)
    at null._onTimeout (\propErrorHandled.js:9:13)
    at Timer.listOnTimeout [as ontimeout] (timers.js:110:15)

[dougwilson]

What version of express are you using?

[dougwilson]

Ah, I ran your test against express 4 and it crashed, but it correctly does not crash with express 3. It looks like the examples only work with express 3.x. If you want to figure out how to make it correctly work with express 4.x, that would be great :)

[dougwilson]

It also does not crash with connect 2.x or connect 3.x, so it looks like just something with express 4.x (the doc does show the example as specifically being express 3.x, btw).

[dougwilson]

the doc does show the example as specifically being express 3.x, btw

I can update the example above the "express 3.x" example to also state that the express in that example is 3.x.

[basarat]

if you want to figure out how to make it correctly work with express 4.x, that would be great

Since I haven't found the time I've created a new clear issue that can be taken up by a hero #11