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

Always set request timeout on keep-alive connections #2447

Merged
merged 1 commit into from Nov 3, 2016
Merged
Changes from all commits
Commits
File filter...
Filter file types
Jump to…
Jump to file or symbol
Failed to load files and symbols.

Always

Just for now

@@ -766,52 +766,59 @@ Request.prototype.start = function () {
self.emit('drain')
})
self.req.on('socket', function(socket) {
var setReqTimeout = function() {
// This timeout sets the amount of time to wait *between* bytes sent
// from the server once connected.
//
// In particular, it's useful for erroring if the server fails to send
// data halfway through streaming a response.
self.req.setTimeout(timeout, function () {
if (self.req) {
self.abort()
var e = new Error('ESOCKETTIMEDOUT')
e.code = 'ESOCKETTIMEDOUT'
e.connect = false
self.emit('error', e)
}
})
}
// `._connecting` was the old property which was made public in node v6.1.0
var isConnecting = socket._connecting || socket.connecting
// Only start the connection timer if we're actually connecting a new
// socket, otherwise if we're already connected (because this is a
// keep-alive connection) do not bother. This is important since we won't
// get a 'connect' event for an already connected socket.
if (timeout !== undefined && isConnecting) {
var onReqSockConnect = function() {
socket.removeListener('connect', onReqSockConnect)
clearTimeout(self.timeoutTimer)
self.timeoutTimer = null
// Set an additional timeout on the socket, via the `setsockopt` syscall.
// This timeout sets the amount of time to wait *between* bytes sent
// from the server once connected.
//
// In particular, it's useful for erroring if the server fails to send
// data halfway through streaming a response.
self.req.setTimeout(timeout, function () {
if (self.req) {
self.abort()
var e = new Error('ESOCKETTIMEDOUT')
e.code = 'ESOCKETTIMEDOUT'
e.connect = false
self.emit('error', e)
}
})
}
if (timeout !== undefined) {
// Only start the connection timer if we're actually connecting a new
// socket, otherwise if we're already connected (because this is a
// keep-alive connection) do not bother. This is important since we won't
// get a 'connect' event for an already connected socket.
if (isConnecting) {
var onReqSockConnect = function() {
socket.removeListener('connect', onReqSockConnect)
clearTimeout(self.timeoutTimer)
self.timeoutTimer = null
setReqTimeout()
}

socket.on('connect', onReqSockConnect)
socket.on('connect', onReqSockConnect)

self.req.on('error', function(err) {
socket.removeListener('connect', onReqSockConnect)
})
self.req.on('error', function(err) {
socket.removeListener('connect', onReqSockConnect)
})

// Set a timeout in memory - this block will throw if the server takes more
// than `timeout` to write the HTTP status and headers (corresponding to
// the on('response') event on the client). NB: this measures wall-clock
// time, not the time between bytes sent by the server.
self.timeoutTimer = setTimeout(function () {
socket.removeListener('connect', onReqSockConnect)
self.abort()
var e = new Error('ETIMEDOUT')
e.code = 'ETIMEDOUT'
e.connect = true
self.emit('error', e)
}, timeout)
// Set a timeout in memory - this block will throw if the server takes more
// than `timeout` to write the HTTP status and headers (corresponding to
// the on('response') event on the client). NB: this measures wall-clock
// time, not the time between bytes sent by the server.
self.timeoutTimer = setTimeout(function () {
socket.removeListener('connect', onReqSockConnect)
self.abort()
var e = new Error('ETIMEDOUT')
e.code = 'ETIMEDOUT'
e.connect = true
self.emit('error', e)
}, timeout)
} else {
// We're already connected
setReqTimeout()
}
}
self.emit('socket', socket)
})
@@ -199,6 +199,35 @@ tape('connect timeout with non-timeout error', function(t) {
})
})

tape('request timeout with keep-alive connection', function(t) {
var agent = new require('http').Agent({ keepAlive: true })
var firstReq = {
url: s.url + '/timeout',
agent: agent
}
request(firstReq, function(err) {
// We should now still have a socket open. For the second request we should
// see a request timeout on the active socket ...
t.equal(err, null)
var shouldReqTimeout = {
url: s.url + '/timeout',
timeout: 100,
agent: agent
}
request(shouldReqTimeout, function(err) {
checkErrCode(t, err)
t.ok(err.connect === false, 'Error should have been a request timeout error')
t.end()
}).on('socket', function(socket) {
var isConnecting = socket._connecting || socket.connecting
t.ok(isConnecting !== true, 'Socket should already be connected')
})
}).on('socket', function(socket) {
var isConnecting = socket._connecting || socket.connecting
t.ok(isConnecting === true, 'Socket should be new')
})
})

tape('cleanup', function(t) {
s.close(function() {
t.end()
ProTip! Use n and p to navigate between commits in a pull request.
You can’t perform that action at this time.