-
Notifications
You must be signed in to change notification settings - Fork 3.2k
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
Add ability to detect connect timeouts #1711
Conversation
Fixes #1660. |
self.timeoutTimer = setTimeout(function () { | ||
var connectTimeout = self.req.socket && self.req.socket.readable === false; |
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.
Need to read this value before aborting, since the socket gets destroyed. Could pull the error creation up here too, not sure which way is preferable.
0e106ec
to
08bc390
Compare
Whoops - made a breaking change when writing a comment and didn't see the Travis results. Looks like there are some failures related to expired certificates. Not sure if they are related or not; I can investigate further. |
For the certificates just run Also add |
08bc390
to
4087a57
Compare
Whoops - not sure how that snuck in there. Sorry for wasting your time. Should be fixed now. |
Cert failures should hopefully go away once #1712 is merged. I will rebase once that's in. |
It's valuable to be able to detect whether a timeout occurred on the `connect` syscall or on the `write`/`read` syscall of an HTTP request - the former implies that your client can't reach the server and the latter implies the server might not have responded. For backwards compatibility, and standardization with Unix errno codes, the `code` property of connect timeout errors must remain as 'ETIMEDOUT'. Instead of changing this value, add a new `connect` property on timeout errors, which is set to true if the error occurred on the `connect` attempt. Clients have to opt in to checking it - in the common read timeout case, it will be set to `false`. Updates the documentation around each timeout to clarify what is going on, and what situations can trigger each timeout. Updates the README with more accurate documentation about timeout times in the wild. I tried `time curl http://10.255.255.1` and `time curl http://google.com:81` on four different Linux machines (Heroku, boot2docker, Centos on DigitalOcean, a VPS) and the timeout times ranged from 60 seconds to 120 seconds, much longer than the 20 seconds previously suggested. Updates the README with information on how to detect connection timeout failures.
4087a57
to
ee4cfbb
Compare
Tests are back to green. The code coverage on this is 0, because the timeout tests don't run on Travis. I believe that they should run okay, but let me know what you think, or if there's a workaround I can use for code coverage here. |
Looks good to me, thanks for working on and documenting this one, @kevinburke! I'll leave this PR open for a while in case anyone else have something to add. |
Add ability to detect connect timeouts
It's valuable to be able to detect whether a timeout occurred on the
connect
syscall or on the
write
/read
syscall of an HTTP request - the formerimplies that your client can't reach the server and the latter implies the
server might not have responded.
For backwards compatibility, and standardization with Unix errno codes, the
code
property of connect timeout errors must remain as 'ETIMEDOUT'. Insteadof changing this value, add a new
connect
property on timeout errors, whichis set to true if the error occurred on the
connect
attempt. Clients haveto opt in to checking it - in the common read timeout case, it will be set to
false
.Updates the documentation around each timeout to clarify what is going on, and
what situations can trigger each timeout.
Updates the README with more accurate documentation about timeout times
in the wild. I tried
time curl http://10.255.255.1
andtime curl http://google.com:81
on four different Linux machines (Heroku, boot2docker,Centos on DigitalOcean, a VPS) and the timeout times ranged from 60 seconds to
120 seconds, much longer than the 20 seconds previously suggested.
Updates the README with information on how to detect connection timeout
failures.