-
Notifications
You must be signed in to change notification settings - Fork 30.1k
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
http: remove stale timeout listeners #9440
Changes from 4 commits
65790af
5c37b4a
274cb28
c32ee77
2c4ebae
fa6ec18
516ba6f
60ff0fb
3039b1d
6cc3f0b
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -562,7 +562,13 @@ function tickOnSocket(req, socket) { | |
socket.on('close', socketCloseListener); | ||
|
||
if (req.timeout) { | ||
socket.once('timeout', () => req.emit('timeout')); | ||
const emitRequestTimeout = () => req.emit('timeout'); | ||
socket.once('timeout', emitRequestTimeout); | ||
req.on('response', (r) => { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. hm, I wonder if the response event is emitted in all cases if the request is aborted...will have to dig into that There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. No, I don't think it is, but I think the socket is destroyed, which solves the cleanup issue in that case. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Can you call the argument |
||
r.on('end', () => { | ||
socket.removeListener('timeout', emitRequestTimeout); | ||
}); | ||
}); | ||
} | ||
req.emit('socket', socket); | ||
} | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,55 @@ | ||
'use strict'; | ||
const common = require('../common'); | ||
const http = require('http'); | ||
const assert = require('assert'); | ||
|
||
const agent = new http.Agent({ keepAlive: true }); | ||
|
||
const server = http.createServer((req, res) => { | ||
res.end(''); | ||
}); | ||
|
||
const options = { | ||
agent, | ||
method: 'GET', | ||
port: undefined, | ||
host: common.localhostIPv4, | ||
path: '/', | ||
timeout: 100 | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Can you wrap the timeout in |
||
}; | ||
|
||
process.on('unhandledRejection', function(reason) { | ||
common.fail('A promise rejection was unhandled: ' + reason); | ||
}); | ||
|
||
server.listen(0, options.host, (e) => { | ||
options.port = server.address().port; | ||
|
||
doRequest().then(doRequest) | ||
.then((numListeners) => { | ||
assert.strictEqual(numListeners, 1); | ||
server.close(); | ||
agent.destroy(); | ||
}); | ||
}); | ||
|
||
function doRequest() { | ||
return new Promise((resolve, reject) => { | ||
http.request(options, (response) => { | ||
const sockets = agent.sockets[`${options.host}:${options.port}:`]; | ||
if (sockets.length !== 1) { | ||
reject(new Error('Only one socket should be created')); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Should there be a |
||
} | ||
const socket = sockets[0]; | ||
const timeoutEvent = socket._events.timeout; | ||
let numListeners = 0; | ||
if (Array.isArray(timeoutEvent)) { | ||
numListeners = timeoutEvent.length; | ||
} else if (timeoutEvent) { | ||
numListeners = 1; | ||
} | ||
response.resume(); | ||
response.on('end', () => resolve(numListeners)); | ||
}).end(); | ||
}); | ||
} | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I'd write the test in simple callback style. Less chance of accidentally swallowing errors and easier to debug because you can simply |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not to derail this PR which is desperately needed, but isn't this section very similar to the existing
ClientRequest.prototype.setTimeout
method? Would just callingreq.setTimeout(req.timeout)
at this point achieve almost the same behavior?There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I thought along the same lines when I found this bug. The code implementing this feature didn't look like I expected, but I just wanted to make a small, conceptually simple fix to get it merged as quickly as possible.
It think your suggestion would work. What does @evanlucas say?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I reused your unit test code and took a stab at the approach described above in #9831
The primary difference is that any timeout set applies from the time a socket is assigned to a request, i.e. it includes socket pre-connect timeouts. In existing code, if a timeout is set via
ClientRequest.prototype.setTimeout
, it is "reapplied" and starts counting only from the time the socket connects. Perhaps this would count as an API change.In other HTTP libraries, the connect-timeout and the transaction-timeout are two separate parameters, e.g. in curl https://curl.haxx.se/libcurl/c/CURLOPT_CONNECTTIMEOUT.html and https://curl.haxx.se/libcurl/c/CURLOPT_TIMEOUT.html