Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Added connect.timeout() middleware #519

Closed
wants to merge 16 commits into from
Closed
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
50 changes: 50 additions & 0 deletions lib/middleware/timeout.js
@@ -0,0 +1,50 @@
/*!
* Connect - timeout
* Ported from https://github.com/LearnBoost/connect-timeout
* MIT Licensed
*/

/**
* Timeout:
*
* Invokes `socket.setTimeout` to timeout idle requests
* Adds req.clearTimeout() for long-running requests
*
* - `throwError`: throw an error instead of writing a status code
* - `time`: timeout length in ms (default: 10000)
*
* @param {Object} options
* @return {Function}
* @api public
*/

module.exports = function timeout(options) {
options = options || {};
options.time = options.time || 8000;
options.code = options.code || 500;
options.throwError = options.throwError || false;

return function(req, res, next) {
var writeHead = res.writeHead,
at = req.method + ' ' + req.url,
timer = setTimeout(function() {
if (options.throwError) {
return next(new Error('Timeout ' + req.method + ' ' + req.url));
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this actually doesn't work as expected, I'll see if I can find a work-around

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I was curious about how that was expected to work; I just duplicated the old connect-timeout stuff.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this sort of use-case will work better with #522

I need to think on that some more though, both have pros/cons, deferred errors like this one might invoke app.error several times which is good for reporting but bad for responding. we could tell people to check if (res.headerSent) but that's kinda lame, almost need one callback for reporting errors and one for responding :s but that's sorta lame too

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The only way a timeout should occur is if res.end() is not called within a preset amount of time. This means that there can only be one timeout error. This deferred error can only happen once at most. IMHO, this behavior is desired.

Say an error occurs (i.e. next(err) is called)... the error handler can decide whether or not to call next() or do res.send(500); or whatever. Maybe the error handler prints the message to stderr, or maybe an email is sent. If the error handler decides to call res.send(), then technically, no timeout has happened. So, there is only one error reported to the client. Similarly, only one timeout error can happen.

}
res.writeHead(options.code);
res.end();
}, options.time);

req.clearTimeout = function() {
clearTimeout(timer);
};

res.writeHead = function() {
res.writeHead = writeHead;
req.clearTimeout();
return res.writeHead.apply(res, arguments);
}

return next();
};
};
Binary file added test/fixtures/favicon.ico
Binary file not shown.
79 changes: 79 additions & 0 deletions test/timeout.js
@@ -0,0 +1,79 @@

var connect = require('../');

var fixtures = __dirname + '/fixtures';

var app = connect();

app.use(connect.timeout({
code: 503,
time: 500
}));

app.use(connect['static'](fixtures));

var timeouts;
app.use(function(err, req, res, next) {
timeouts++;
});

app.use(function(req, res, next) {
if (req.url === '/should/timeout') {
// chill and wait for timeout
}
else if (req.url === '/should/not/timeout') {
res.writeHead(200);
res.end();
}
else if (req.url === '/should/interrupt/timeout') {
req.clearTimeout();
setTimeout(function() {
res.writeHead(200);
res.end();
}, 1000); // Wait until after timeout `time`
}
});

// Tests

describe('connect.timeout()', function() {
it('should timeout', function(done) {
app.request()
.get('/should/timeout')
.end(function(res) {
res.statusCode.should.equal(503);
done();
});
});

it('should not timeout', function(done) {
app.request()
.get('/should/not/timeout')
.end(function(res) {
res.statusCode.should.equal(200);
done();
});
});

it('should interrupt timeout', function(done) {
app.request()
.get('/should/interrupt/timeout')
.end(function(res) {
res.statusCode.should.equal(200);
done();
});
});

it('should serve static files without timeout', function(done){
timeouts = 0;
app.request()
.get('/favicon.ico')
.end(function(res) {
setTimeout(function() {
timeouts.should.equal(0);
done();
}, 1000);
});
});

});