Document connect_timeout and set it to the nonzero minimum of itself and readtimeout #641
Conversation
This is to make it the same format as `readtimeout`. Also changes `connecttimeout` to be the nonzero minimum of `connecttimeout` and `readtimeout` if either is set. This is to get around connections hanging even if `readtimeout` is set. For example, this occurs if you try to connect to the EC2 metadata IP from outside AWS.
Codecov Report
@@ Coverage Diff @@
## master #641 +/- ##
==========================================
+ Coverage 77.53% 77.55% +0.01%
==========================================
Files 36 36
Lines 2324 2326 +2
==========================================
+ Hits 1802 1804 +2
Misses 522 522
Continue to review full report at Codecov.
|
src/ConnectionPool.jl
Outdated
| timeouts = filter(!iszero, [connect_timeout, readtimeout]) | ||
| connect_timeout = isempty(timeouts) ? 0 : minimum(timeouts) |
There was a problem hiding this comment.
Couldn't this be just
| timeouts = filter(!iszero, [connect_timeout, readtimeout]) | |
| connect_timeout = isempty(timeouts) ? 0 : minimum(timeouts) | |
| connect_timeout = min(connect_timeout, readtimeout) |
There was a problem hiding this comment.
No, because we want the smallest non-zero between the two. It could be:
connect_timeout = minimum(filter(!iszero, [connect_timeout, readtimeout]))Wait no. If both are 0 this will break.
There was a problem hiding this comment.
I don't believe so. If either connect_timeout or readtimeout is 0, then that will return 0, meaning no connection timeout will be set.
There was a problem hiding this comment.
How about just:
connect_timeout = connect_timeout == 0 && readtimeout > 0 ? readtimeout : connect_timeoutThat avoids allocating an unnecessary array and the need to call filter and minimum.
There was a problem hiding this comment.
Nice, I like that better, fixed.
sean-bennett112
changed the title
connect_timeout to connecttimeout and document
src/ConnectionPool.jl
Outdated
| timeouts = filter(!iszero, [connect_timeout, readtimeout]) | ||
| connect_timeout = isempty(timeouts) ? 0 : minimum(timeouts) |
There was a problem hiding this comment.
How about just:
connect_timeout = connect_timeout == 0 && readtimeout > 0 ? readtimeout : connect_timeoutThat avoids allocating an unnecessary array and the need to call filter and minimum.
This is to make it the same format as
readtimeout.Also changes
connecttimeoutto be the nonzero minimum ofconnecttimeoutandreadtimeoutif either is set.This is to get around connections hanging even if
readtimeoutis set. For example, this occurs if you try to connect to the EC2 metadata IP from outside AWS.I'm not sure of a great way to add a test for this. This works, for example:
But I'd obviously prefer to avoid trying to hit the EC2 metadata service.