Skip to content
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

Closed
fededonna opened this issue Sep 6, 2016 · 13 comments
Closed

[Feature request] - Whole operation timeout #2840

fededonna opened this issue Sep 6, 2016 · 13 comments
Labels
enhancement Feature not a bug
Milestone

Comments

@fededonna
Copy link

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.

@swankjesse
Copy link
Member

Yep. Let's do something for this.

@JakeWharton
Copy link
Member

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(..);

@fededonna
Copy link
Author

I'm no using retrofit. I just want to create a client with a timeout for the whole operation.

@tobinibot
Copy link

I’d also be interested in a whole operation timeout.

@swankjesse swankjesse added the enhancement Feature not a bug label Oct 16, 2016
@swankjesse swankjesse added this to the 3.6 milestone Oct 16, 2016
@swankjesse swankjesse modified the milestones: 3.6, 3.7 Jan 29, 2017
@ericcj
Copy link

ericcj commented Feb 28, 2017

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.

@robbiepl
Copy link

Will this functionality be implemented?

@robbiepl
Copy link

we waiting ... :))

@swankjesse swankjesse modified the milestones: 3.7, 3.8 Apr 28, 2017
@fededonna
Copy link
Author

@swankjesse any update regarding this issue? Thanks in advance.

@swankjesse
Copy link
Member

We can probably just hook up AsyncTimeout to call Call.cancel(). Heck, we could even make Call work like Source and Sink so it can be adjusted independent of the client.

  /** Returns the timeout for this call */
  Timeout timeout();

swankjesse added a commit that referenced this issue Nov 3, 2018
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
@swankjesse swankjesse modified the milestones: 3.13, 3.12 Nov 3, 2018
swankjesse added a commit that referenced this issue Nov 4, 2018
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
@swankjesse
Copy link
Member

swankjesse commented Nov 4, 2018

Next steps:

  • Pick a default value (none? 60 seconds?)
  • Configuration on OkHttpClient.Builder
  • Confirm it does something reasonable for web sockets and SSE

@swankjesse
Copy link
Member

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.

@swankjesse
Copy link
Member

#4369

@swankjesse
Copy link
Member

Fixed!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement Feature not a bug
Projects
None yet
Development

No branches or pull requests

6 participants