Add support for call timeouts #70
Conversation
| @@ -77,7 +77,7 @@ public HttpMethod httpMethod() { | |||
| }; | |||
|
|
|||
| /** Returns a new blocking {@link SampleService} implementation whose calls are executed on the given channel. */ | |||
| public static SampleService blocking(Channel channel, ConjureRuntime runtime) { | |||
| public static SampleService blocking(Channel channel, ConjureRuntime runtime, Duration readTimeout) { | |||
There was a problem hiding this comment.
wondering if it'd make more sense for this to be a builder-pattern thing or possibly for the third arg to be some kind of settings object
There was a problem hiding this comment.
I'll add a TODO, think that'd be a separate PR.
| @@ -98,10 +98,12 @@ public String stringToString(String objectId, String header, String body) { | |||
| .build(); | |||
|
|
|||
| Call call = channel.createCall(STRING_TO_STRING, request); | |||
| ListenableFuture<Response> response = Calls.toFuture(call); | |||
| ListenableFuture<String> response = Futures.transform( | |||
There was a problem hiding this comment.
thoughts on composing the timeouts as a Channel implementation?
There was a problem hiding this comment.
hmm. it wouldn't be so straightforward, because then the async and the blocking client would need different types of channels. (the async client as it stands doesn't do timeouts as callers are in control.) I think I'd revisit that problem when exposing clients that interact with an Observer, because we'd then have to think about how to expose the timeout to those observers.
There was a problem hiding this comment.
(sorry, missed this comment before the last one I posted)
It seems like async clients should still enforce timeouts on the requests/time-to-response, in which case it'd be the same Channel impl?
|
Is the goal to produce a total request duration timeout from the first bytes sent until the response has been completely read (ignoring streaming binary responses I suppose)? Would you mind documenting that? There are lots of types of timeouts, and most http libraries don't document what they're actually measuring very well. |
|
ready |
| throw new RuntimeException("Failed to deserialize response", e); | ||
| } | ||
| }, | ||
| response -> sampleObjectToSampleObjectDeserializer.deserialize(response), |
There was a problem hiding this comment.
does this mean the async calls have no timeout?
There was a problem hiding this comment.
correct. you're handed a Future and are in charge of timing it out yourself via .get(timeout)
There was a problem hiding this comment.
we can revisit this design if you like, but it seems non-trivial to plumb the timeouts into the underlying http client, in particular for the java one.
There was a problem hiding this comment.
for instance, the java client doesn't distinguish between read and write timeouts while the okhttp client does
No description provided.