-
Notifications
You must be signed in to change notification settings - Fork 1.6k
Improved retrofit timeout handling. #1618
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
Improved retrofit timeout handling. #1618
Conversation
@slandelle, i'm a bit confused about AHC timeouts; as seen from newly created test it's possible to set read timeout to a higher value than request timeout. Which of those values are then being used by AHC instance? If request timeout is always enforced i'll change retrofit timeout computation method.
If read timeout is longer than request timeout, it will be ignored.
@slandelle i've just pushed c3585b1 with updated timeout computation method. Does it make sense to you? If yes, i'm going to squash commit, then you're free to merge.
No, that doesn't work. read timeout is about stalled response when nothing is received for a long time. Think large file download. You might want a 5s read timeout and a 2 min request timeout.
So what would you suggest? timeout() retrofit Call method is used solely by okhttp native client, so it's irrelevant; But there's a possibility that some retrofit call adapters would call execute() in a blocking manner (example: rxjava call adapter initialized with some scheduler) where correct timeout is quite essential; I know that this is not super important, because the whole idea of using AHC with retrofit is to use async retrofitx rxjava call adapter, where timeout handling is done by AHC itself, but nevertheless i would like to improve experience also for users that for some reason initialize call adapters with some other scheduler.
In essence, what i would like to achieve in this PR is to retrieve correct timeout from actual configuration of http client that is going to execute this call; this is improvement, because previously, timeout has been semi-hardcoded in a field and was not configurable by the user.
Just map on request timeout.
👍 i've updated the code, squashed commits; now solely request timeout is used; you're free to merge. And yeah, this time i've tested snapshot with my service :-) Releasing new fix version would be awesome, but it's up to you.
Retrofit `Call` now uses http client's configured request timeout to compute value for `timeout()` and blocking `execute()` method execution timeout.
@bfg I can't release because your PR breaks javadoc (which is required in order to release on maven central).
Run mvn install javadoc:javadoc -DskipTests in order to reproduce.
It looks like an issue with the latest lombok changes you've introduced.
Could you please fix?
Sigh, sure, on it :-)
@slandelle it builds successfully for me using maven 3.5/3.6; which maven version are you using?
Can you check retrofit_javadoc_issue branch on my fork?
I've managed to reproduce javadoc issue. Build works without issues on openjdk 8, but it fails on oracle jdk 8 🤦♂️
Java version: 1.8.0_201, vendor: Oracle Corporation, runtime: /opt/java/jdk1.8.0_201/jre
I'll find a way to work around this issue, one way or another :-)
Uh oh!
There was an error while loading. Please reload this page.
Retrofit
Callnow uses http client's configured timeout to computevalue for
timeout()and blockingexecute()method execution timeout.