-
Notifications
You must be signed in to change notification settings - Fork 97
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
Upgrade OkHttp to 3.11.0. #794
Conversation
a379760
to
30860e5
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Seems like a straight forward bump, I don't think any of the issues mentioned would affect us
@ferozco what should I do about the OkHttpClientTest#handlesSuccessfulResponseCodesWithSuccessHandler? |
Note that on the last PR, there were a couple of questions: from @uschi2000 What does this mean for us?
from @cakofony Note that 3.10.0 removed support for hostname verification via CN, and only validates SAN values. |
I think its fine to omit since it is 100s are such a special case |
@iamdanfox Yep, I copied those in this PR description, see above. Would like to get the opinions of those people, ideally. @j-baker had an opinion on the SAN thing (shouldn't affect us), Retry-After I would need to check if we send those on QoS exceptions, but I don't believe so. |
@@ -44,7 +44,7 @@ public Response intercept(Chain chain) throws IOException { | |||
.url(rebasedUrl) | |||
// Request.this.tag field by default points to request itself if it was not set in RequestBuilder. | |||
// We don't want to reference old request in new one - that is why we need to reset tag to null. | |||
.tag(originalRequest.tag().equals(originalRequest) ? null : originalRequest.tag()) | |||
.tag(originalRequest.equals(originalRequest.tag()) ? null : originalRequest.tag()) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
was this a bug? if so, can you make it a separate PR, please?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
no, they broke
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
tags are now a map keyed by Class, and the old one is the Object case
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@jkozlowski this whole block should no longer be necessary, looking at the new code - don't need to unset the tag
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
but maybe we want it to continue working the way it does for people who force their okhttp version, and so this'd be fine
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
cool ignore me :)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Soo, here's my understanding: in 3.9.0 the Request.Builder#tag method had the following javadoc:
/**
* Attaches {@code tag} to the request. It can be used later to cancel the request. If the tag
* is unspecified or null, the request is canceled by using the request itself as the tag.
*/
The value, as @j-baker pointed out, is automatically set in Request(Builder) constructor to Request.this, and then propagated across to the next builder when the request is cloned (e.g. for retries). This means it builds a linked list of requests which means they all linger on in memory.
It would seem to me this was an omission and not intentional behavior: this is now gone in OkHttp 3.11 so I think we are good to remove this.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Going to resolve this point, unless people object.
re 503: i'd prefer we continue using our explicit 503 handling and turn off the okhttp one. similar to the existing
|
you don't get to pick |
Unless we want to make all of our bodies Unrepeatable, which is its own can of worms |
Right, I'll clean up the test tomorrow, look into the 503 (although looks like we can't opt out of that behaviour). Would be good to understand why we thought we needed to unset the tags and what's the behaviour we want. |
@@ -211,13 +211,13 @@ | |||
] | |||
}, | |||
"com.squareup.okhttp3:logging-interceptor": { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Let's discuss SAN vs CN verification here: the only thing that I can think would break were internal ETE tests, but we verified with a standard test certificate and they are already setup with SAN pointing to localhost and internal test domains.
As far as my understanding goes, we should already be generating correct certs in production, so I think we can close this issue?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes
@@ -9,7 +9,7 @@ com.netflix.feign:feign-*= 8.17.0 | |||
com.palantir.remoting-api:* = 1.9.0 | |||
com.palantir.safe-logging:* = 1.3.0 | |||
com.palantir.tritium:tritium-registry = 0.8.0 | |||
com.squareup.okhttp3:* = 3.9.0 | |||
com.squareup.okhttp3:* = 3.11.0 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Now for Retry-After handling, let's discuss here. The behavior change is as follows:
- 408 (CLIENT_TIMEOUT) is now automatically retried if Retry-After header is not set or is set to 0, as per above and here's the code: (https://github.com/square/okhttp/blob/master/okhttp/src/main/java/okhttp3/internal/http/RetryAndFollowUpInterceptor.java#L367). I don't think we particularly care about this, but in practice it would mean one extra retry: it will give up if THAT extra retry fails.
- 503 are retried if Retry-After is set to 0, here's the code: https://github.com/square/okhttp/blob/master/okhttp/src/main/java/okhttp3/internal/http/RetryAndFollowUpInterceptor.java#L374. Again, I don't believe we ever set Retry-After in 503s and again it means one extra retry (it will bail if that retry fails). Someone could still do this by throwing javax.ws.rs.ServiceUnavailableException.
So, I think we should be good here too, unless anyone objects?.
Can you guys review the reasoning and merge if we think this is good? |
@iamdanfox, @uschi2000, @ferozco ping, can we get a merge/no-merge? |
* Expose HostMetricsRegistry record methods (#780) This is so that we can separately implement a HostMetricsSink for it (see #779) such that we can share host metrics when constructing both conjure and remoting3 clients * publish BOM for jaxrs-client (#789) * Excavator: Render CircleCI file using template specified in .circleci/template.sh (#791) * Upgrade OkHttp to 3.11.0. (#794) * AssertionErrors are converted into service exceptions with type internal (#727) * No more ThreadLocals, delegate everything to 'palantir/tracing-java' (#799) * Use BINTRAY_*_REMOTING envs (#802) The project's default bintray creds are currently set up to publish to `conjure-java-runtime`. Use these custom env vars to maintain ability to publish http-remoting. * Better behaviour in the presence of 429s (#786)
Potentially supersedes #747.
Why?
The 2 releases since 3.9.0 fixed bugs in Http2 handling (especially wrt. flow control).
See:
These bugs have been hit by an internal product and are blocking feature work.
Remaining issues: