-
Notifications
You must be signed in to change notification settings - Fork 9.2k
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
[Feature request] - Whole operation timeout #2840
Comments
Yep. Let's do something for this. |
If you happen to be using Retrofit with its RxJava call adapter you can already get this for free: interface MyService {
@GET("/user")
Observable<User> getUser();
} MyService service = // ...
service.getUser()
.timeout(2, SECONDS) // covers sending the request and waiting for the response
.subscribe(..); |
I'm no using retrofit. I just want to create a client with a timeout for the whole operation. |
I’d also be interested in a whole operation timeout. |
We successfully implemented a deadline for both the request and response body reading by subclassing Call in our application in production: https://gist.github.com/ericcj/49118c877f471b3d8d53db545160e4ac Looks like there have been a couple related implementations, one inside the HTTP codecs (#2729) and a simple solution for just the body reading (#1130). Much of it could also be implemented in an interceptor, except that we would have to close the socket instead of calling the more precise cancel method and it wouldn't cover the seemingly infinite retries on IOExceptions getting a connection in RetryAndFollowUpInterceptor which does while (true) until if (canceled) throw. Note that we also have a separate interceptor to timeout DNS (https://gist.github.com/ericcj/a4e5c99982d36d3348ff3c732fa51038) since even our deadline doesn't cover that as per #95 although now using the new Dns interface would be preferable. |
Will this functionality be implemented? |
we waiting ... :)) |
@swankjesse any update regarding this issue? Thanks in advance. |
We can probably just hook up
|
Strictly-speaking this change is backwards-incompatible because it adds a new method to the Call interface. The method returns the call's timeout. The trickiest part of this is signaling the end of the call, which occurs after the last byte is consumed of the last follow up request, or when the call fails. Fortunately this is made easier by borrowing the sites used by EventListener, which already plots out where calls end. #2840
Strictly-speaking this change is backwards-incompatible because it adds a new method to the Call interface. The method returns the call's timeout. The trickiest part of this is signaling the end of the call, which occurs after the last byte is consumed of the last follow up request, or when the call fails. Fortunately this is made easier by borrowing the sites used by EventListener, which already plots out where calls end. #2840
Next steps:
|
Our initial timeout needs to be no-timeout. Otherwise upgrading is made very difficult when the code-being-upgraded doesn’t expose its OkHttpClient instance to be configured. For example, if the use of OkHttp is an implementation detail of a 3rd party SDK. |
Fixed! |
Hi, I'm working on an application that needs to check the entire request-response cycle time spent and set a max for that. It could be a good thing to add a timeout for the complete operation instead of one for connection, one for read and one for right.
Thanks in advance.
The text was updated successfully, but these errors were encountered: