okhttp: update okhttp and okio dependencies

[joelpedraza]

Update OkHttp to 3.x, OkIo to 2.x

This has been denied when requested in #5103, #2999, #2549, #1628

The case for upgrading

okhttp 2.5.0 is ~5 years old. The major version upgrade previously denied is now just over 3 years old.

Many libraries have migrated dependencies to okhttp3 in recent versions.

General Libs

• Retrofit
• Picasso
• Glide
• react-native

Vendor SDKs

• Parse
• Mapbox
• Google Maps
• Branch
• Paypal

It would be preferable to not need to include multiple versions of the same http lib in our project, especially for those apps targeting Google Instant Apps: which have a hard 4MB apk size limit (an oversimplification, but the http and grpc libs are likely to be in the 'base' module of an app)

Changelog entries relevant to my project

(paraphrased)

OkHttp

• The DNS service is now pluggable
• Many fixes related to certificate pinning
• Use newer TLS primitives when available (android api 17+)
• Gracefully recover when Android 7.0's sockets throw an unexpected NullPointerException
• Recover gracefully when Android's DNS crashes with an unexpected NullPointerException.
• Recover gracefully when Android's socket connections crash with an unexpected ClassCastException.

OkIo

• Perf improvements.

OkIo 2.x is binary compatible with 1.x

Vendoring both versions

Could work for my project. 2.x would fall out when tree shaking.

Motivations on doing so are unclear to me given the age of 2.x branch. Looking at the project history guava got a major version upgrade from 19 to 20, though admittedly netty has only had revision bumps.

This PR does not try to vendor both 2.x and 3.x versions. It's only a version bump.

[thelinuxfoundation]

Thank you for your pull request. Before we can look at your contribution, we need to ensure all contributors are covered by a Contributor License Agreement.

After the following items are addressed, please respond with a new comment here, and the automated system will re-verify.

• User @joelpedraza isn't covered by a CLA. They will need to complete the form at https://identity.linuxfoundation.org/projects/cncf

Regards,
CLA GitHub bot

[joelpedraza]

Thank you for your pull request. Before we can look at your contribution, we need to ensure all contributors are covered by a Contributor License Agreement.

After the following items are addressed, please respond with a new comment here, and the automated system will re-verify.

* User @joelpedraza isn't covered by a CLA. They will need to complete the form at https://identity.linuxfoundation.org/projects/cncf

Regards, CLA GitHub bot

I signed it

[ejona86]

I'll note that if we removed DEFAULT_CONNECTION_SPEC (which could be done immediately in a separate PR) we really wouldn't be using many okhttp methods.

OkHttpChannelBuilder:

• We should remove DEFAULT_CONNECTION_SPEC. We can do that in a separate PR
• We should continue supporting OkHttp 2's ConnectionSpec for a while. We should do the same convertSpec() goo for it, but just another copy of it (overloading the current method). Proguard will strip them when unused. This alone should be all that's necessary to support both OkHttp 2 and 3 simultaneously for migration.

OkHttpClientTransport changes should be fine.

I'm not wild about the piecemeal copying of CipherSuite and TlsVersion (without copying more from OkHttp 3.12). But I see why it was done. @ericgribkoff, thoughts?

The DNS service is now pluggable

gRPC doesn't use this.

Many fixes related to certificate pinning

I'm not sure if this works with gRPC in the normal way.

Use newer TLS primitives when available (android api 17+)

Doh, I thought we had already modified ConnectionSpec to add chacha20 support, but apparently not. We'd need to update OkHttpChannelBuilder's cipher suite list to use the new suites. We should be able to just look at Netty's list (as the comment mentions).

Gracefully recover when Android 7.0's sockets throw an unexpected NullPointerException
Recover gracefully when Android's socket connections crash with an unexpected ClassCastException.

I'd have to see the specific locations these are done to tell whether gRPC would see the changes.

Recover gracefully when Android's DNS crashes with an unexpected NullPointerException.

gRPC doesn't use okhttp's DNS.

OkIo
Perf improvements.

Upgrading okio is fine. I am sad about okio swapping languages though; reading the okio 2 code is hard so I always swap to looking at the old Java code and assuming it works basically the same way.

We will need to make sure we can upgrade okio internally.

[space-pope]

Noticed this was still open and that the OkHttp version is still at 2.5. The linked issue that's the most recent item in this thread (closed because this one is still open) is another reason to upgrade, IMO.

[ericgribkoff]

Noticed this was still open and that the OkHttp version is still at 2.5. The linked issue that's the most recent item in this thread (closed because this one is still open) is another reason to upgrade, IMO.

I think the reporter of googleapis/google-cloud-java#4727 may have misdiagnosed their problem. At least, I don't see anything in the referenced OkHttp 3 bug fix (square/okhttp#4365) to indicate that it has anything to do with uncaught RejectExecutionExceptions in gRPC or that switching to OkHttp 3 would do anything to alleviate it. The OkHttp issue is about an executor that has shutdown: the issue reported in gRPC's stack is about executors occasionally rejecting new tasks, which is normal non-shutdown executor behavior. Rather, it looks like the issue reported there is more to do with #636

[ejona86]

I added a comment to googleapis/google-cloud-java#4727 talking about the things that may be related. This PR is unrelated to that issue (it was misdiagnosed).

[space-pope]

Sorry for the misguided necro, but thanks for the quick reply! In principle, I'd still be for bumping dependencies, but I've read a bit and realize it isn't that simple/doesn't add a ton of size overhead after proguard's done its thing.

[ejona86]

Most of this isn't very relevant after #8971 . We could still expose the okhttp 3+ ConnectionSpec API on OkHttpChannelBuilder, and we would accept a contribution for that. But it should be a new method. We want to leave the okhttp 2 ConnectionSpec API on OkHttpChannelBuilder for a while after the new method is added (to allow easy migration). See #8732