add deadlines #57
add deadlines #57
Conversation
| @@ -58,16 +58,16 @@ public class HTTPClient { | |||
| } | |||
| } | |||
|
|
|||
| public func get(url: String, timeout: Timeout? = nil) -> EventLoopFuture<Response> { | |||
| public func get(url: String, timeout: Timeout? = nil, deadline: NIODeadline? = nil) -> EventLoopFuture<Response> { | |||
There was a problem hiding this comment.
it should be either a dealine or a timeout I think, doesn't make sense to specify both.
There was a problem hiding this comment.
I was thinking about it, but it will basically double the number of methods... Do you think it's ok? Also, we have to deal with both timeout and deadline anyway, since client can have configuration timeouts for example.
There was a problem hiding this comment.
IMHO, better 2x methods that make sense rather then 1x methods that don't make sense.
If you really hate it, make an enum that is either timeout or deadline or just get rid of the timeouts :)
There was a problem hiding this comment.
@tanner0101 / @ianpartridge what are your thoughts here?
There was a problem hiding this comment.
I would be tempted to only do deadlines. You can make a timeout from a deadline.
There was a problem hiding this comment.
Actually, this is not a good idea :D, timeout is composite, consisting of read and connect timeouts, so it's not that strait-forward to convert deadline to timeout...
There was a problem hiding this comment.
@artemredkin could you expand here? The user only cares that the get request is done at a certain point in time, right? They don't care how much of that time is spent in connect or read. Am I missing something?
There was a problem hiding this comment.
Agreed, I would put the read and connect timeouts on the overall client configuration, then put a deadline on the individual requests.
There was a problem hiding this comment.
Lets try just deadlines, anything else worth fixing in this PR?
There was a problem hiding this comment.
@artemredkin I'd say let's get it in
Add deadline argument to API methods