-
Notifications
You must be signed in to change notification settings - Fork 175
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 ConnectionPool connect_timeout and idle_timeout #215
Merged
Merged
Changes from all commits
Commits
Show all changes
8 commits
Select commit
Hold shift + click to select a range
b6667b0
add ConnectionPool connect_timeout and idle_timeout
samoconnor a636f37
tweak Servers.jl for new Connection args
samoconnor 483cbef
close connections on idle_timeout
samoconnor 132e037
fix Connection(io) constructor
samoconnor ce4906b
revised connect_timeout implementation
samoconnor f594189
revised timeout
samoconnor ed87912
set tcp.status = Base.StatusClosing when closing on timeout
samoconnor a5330d5
Merge branch 'master' into idle_timeout
samoconnor File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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.
Why do you want to make sure it throws this exception?
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.
Because I don't think
UVError
should ever be thrown from a user-level API unless it means there is a bug. See JuliaLang/julia#15879 etc.I think application level code will want to handle "I set a short timeout and it wasn't met" differently from "something bad happened deep in the network stack.
Elsewhere in HTTP.jl we wrap things like
UVError
andArgumentError "stream is closed or unusable"
in anIOError
type:https://github.com/JuliaWeb/HTTP.jl/blob/master/src/ConnectionRequest.jl#L36
In higher level code it is important to handle some specific errors by retrying but make sure everything else finds its way up to create a crash and a stack trace and an issue report and a fix. e.g. https://github.com/JuliaCloud/AWSCore.jl/blob/master/src/http.jl#L47-L51
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.
ArgumentError "stream is closed or unusable"
seems pretty wrong because it looks like a precondition error, but there is now way to check the precondition before calling the API because the stream can beisopen()
before the call but be found to be "closed or unusable" while the call is running.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.
Hi @vtjnash, I just noticed that
poll_fd
has atimeout_s
option. Would that work here?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'm not sure how that's relevant here, since you don't have a
fd
. But yes, that does show another pattern that can be used to implement timeouts (with some more difficulty ... in fact, you've just inspired me to rewrite that file better ... you may see a new PR to base tomorrow sometime 🙂)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 was assuming that I could get the
fd
out of the socket withuv_fileno
or something like that.Is there a gotcha along the lines of windows socket handles won't work with
poll_fd
?👍🙃
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.
(a) the socket isn't open, so you can't poll it (b) it's invalid to pass libuv-derived fd handles to libuv
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.
thx for taking the time to explain :)