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

Add ability to detect connect timeouts #1711

Merged
merged 1 commit into from Aug 10, 2015

Conversation

kevinburke
Copy link
Contributor

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.

@kevinburke
Copy link
Contributor Author

Fixes #1660.

self.timeoutTimer = setTimeout(function () {
var connectTimeout = self.req.socket && self.req.socket.readable === false;
Copy link
Contributor Author

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.

@kevinburke
Copy link
Contributor Author

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.

@simov
Copy link
Member

simov commented Aug 3, 2015

For the certificates just run tests/ssl/ca/gen-client.sh and tests/ssl/ca/gen-localhost.sh

Also add npm-debug.log to .gitignore and remove it from your commit.

@kevinburke
Copy link
Contributor Author

Also add npm-debug.log to .gitignore and remove it from your commit.

Whoops - not sure how that snuck in there. Sorry for wasting your time. Should be fixed now.

@kevinburke
Copy link
Contributor Author

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.
@kevinburke
Copy link
Contributor Author

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.

@simov
Copy link
Member

simov commented Aug 4, 2015

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.

simov added a commit that referenced this pull request Aug 10, 2015
Add ability to detect connect timeouts
@simov simov merged commit 94536b4 into request:master Aug 10, 2015
@plroebuck plroebuck mentioned this pull request Apr 3, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants