search

grpc / grpc-java

The Java gRPC implementation. HTTP/2 based RPC
https://grpc.io/docs/languages/java/
Apache License 2.0
11.31k stars 3.8k forks source link

Upgrade to okhttp3 #6119

Open tao1 opened 4 years ago

tao1 commented 4 years ago

Please answer these questions before submitting your issue.

What version of gRPC are you using?

1.23

What did you expect to see?

okhttp3 is more than 3 years old now (released in 2016-01-13). Please upgrade grpc-okhttp to use okhttp3. Indeed a lot of android apps now use okhttp3 to make http requests. But if an android app also uses grpc, it must still embed okhttp 2.5.0 library.

Kind regards,

Laurent

ran-su commented 4 years ago

Currently, we do not have a plan for that.

gildor commented 4 years ago

@ran-su Are there any particular reason for this? It's not a problem if you use grpc in your app, but actually most of the applications just have this dependency because of Firebase, it's not only polluting autocomplete but also adds unnecessary code to the classpath and increase the size of the app, which even more important for SDKs. Maybe make sense to introduce a new artifact grpc-okhttp3 so it would be possible to select more appropriate version of http library

One more possible advantage would be if we could share Okhttp client between grpc and our own code, so also can share Dispatcher and underlying thread pool

ran-su commented 4 years ago

The okhttp need to be modified to suit our needs. We did that for okhttp2.5. Currently, we do not have plan for upgrade to okhttp3. I will bring your requests to the team meetings.

ejona86 commented 4 years ago

Our usage of okhttp 2 adds negligibly to the size of the app after proguard. The Android helloworld example retains 67 methods from okhttp. This is because grpc has a fork/reimplementation of the HTTP/2 code, so very little of the dependency is used. In some ways we use Okio more than OkHttp.

The few methods retained are for HTTP CONNECT proxy support: https://github.com/grpc/grpc-java/blob/e866d3539c3d331e3a5d0095caaa2b1369c4950d/okhttp/src/main/java/io/grpc/okhttp/OkHttpClientTransport.java#L30-L32

Those could be migrated to okhttp 3, but at that point grpc would technically depend on both okhttp 2 and okhttp 3. However, Proguard could strip all of okhttp 2.

The OkHttpChannelBuilder API allows you to specify a com.squareup.okhttp.ConnectionSpec , which is part of the OkHttp 2 API. If we support OkHttp 3, we'd probably add another method to also support the OkHttp 3 equivalent. This is because we don't use either directly; we end up copying the details to an internal format. If you don't use the method, then Proguard strips the usages.

One more possible advantage would be if we could share Okhttp client between grpc and our own code

That has never been possible, even with okhttp 2.

Maybe make sense to introduce a new artifact grpc-okhttp3 so it would be possible to select more appropriate version of http library

No, that is quite an undertaking to make a fork and maintain both.

malachid commented 4 years ago

The security implications of using a really old outdated version without the bugfixes and security patches is the biggest downside to using the io.grpc version for me. Looking at the release notes for OkHttp 4 and OkHttp 3, I am surprised that anyone would be willing to still use okhttp 2.7.5 in production. Especially when we already have OkHttp 3 or 4 in our apps. This issue has come up over and over (#1628 #2549 #2999 #5103 #6119 etc).

It seems like the best option would be to have a proper dependency on an updated version rather than trying to get everyone to use an out-of-date copy. If it doesn't have some support io.grpc needs; why is a PR not being passed onto OkHttp?

ejona86 commented 4 years ago

The security implications of using a really old outdated version

Except there aren't many security implications. We are maintaining the "internal" code from OkHttp, but the OkHttp code that we are using is providing some primitives. We have our own full implementation of HTTP/2 on top and manage our own TLS, etc.

As I already mentioned, ProGuard will strip out most of the com.squareup.okhttp namespace.

It seems like the best option would be to have a proper dependency on an updated version rather than trying to get everyone to use an out-of-date copy.

The problem is it is a lot of work for little gain. I do think it should happen, but we can't find enough technical reason it should happen soon, above other work.

Part of the problem is that the "OkHttp" name for the transport is a bit of a misnomer. We are using OkHttp, but it isn't a "normal" usage by any means; most people don't realize how little of OkHttp we are using.

(It would be nice to use OkHttp more directly. But there are some fundamental API differences that have strong impact to overhead, like blocking vs async API.)

Kernald commented 3 years ago

The APK size isn't the only issue - this dependency on OkHttp 2 also pulls a really old version of Okio (1.x vs 2.x), which in turns causes issues with OkHttp3 - see https://github.com/square/okhttp/issues/5378 for example. Overriding the dependency to use Okio 2.x fixes the issue, except that grpc-java then doesn't build anymore (I have no idea if there are more errors, but https://github.com/grpc/grpc-java/blob/b2e475712d67a0660e1d16d6465a4ccb9b8a6b40/okhttp/src/main/java/io/grpc/okhttp/OkHttpReadableBuffer.java#L43 throws an exception in Okio 2, which isn't handled here).

This is becoming a real problem, not limited to the APK size or security concerns.

ejona86 commented 3 years ago

@Kernald, that seems to be a different issue than this one. Please open a separate issue for tracking Okio compatibility issues.

Kernald commented 3 years ago

@ejona86 Done in #8004 - it's essentially similar I suspect, as OkHttp3 4.x depends on Okio 2.x (I stumbled on this by trying to depend on OkHttp3, not Okio directly).

tkaitchuck commented 2 years ago

@ejona86 Is this blocking https://github.com/grpc/grpc-java/issues/7431 ?

ejona86 commented 2 years ago

@tkaitchuck, not as much as it may seem. It only impacts the ability to tell gRPC to use TLS v1.3 via a ConnectionSpec on the builder.

anuraaga commented 2 years ago

@ejona86 We package gRPC into a javaagent so care about binary size, but currently don't use proguard. We don't expose the knobs that rely on the okhttp types to our end users. I'm curious what you think about excluding the dependency, as I tried https://github.com/open-telemetry/opentelemetry-java/pull/3678 This thread makes me believe that okhttp types won't be used on the hot path and this approach should basically be OK and would be called out in release notes otherwise. Excluding it is hacky, so I don't think it's a great approach or deserves endorsement - but would appreciate hearing if there are any current, obvious pitfalls or in-progress plans that would cause pitfalls.

Also curious if there is any appetite for deprecating the existing methods and add ones that accept a gRPC-defined type. That way the non-deprecated API would have no public surface exposing okhttp.

ejona86 commented 2 years ago

We package gRPC into a javaagent so care about binary size, but currently don't use proguard.

@anuraaga, which means to me you don't care about binary size very much. Minification tools can dramatically reduce binary size. If you strongly cared, it'd seem you'd be using one.

I'm curious what you think about excluding the dependency

You can't currently safely exclude the okhttp dependency because we do use small parts of their API in our implementation. For example:

https://github.com/grpc/grpc-java/blob/fcc7b9694ee1fef20f12d21bdea193362fa1874b/okhttp/src/main/java/io/grpc/okhttp/OkHttpClientTransport.java#L31-L34

Those specific usages are for client-side proxy usage, so excluding okhttp on your machine may seem to work, but it may break on another machine.

I'm actually going to be trying to "clean" some of those up in Q4 so that grpc-okhttp could become an optional dependency. At that point we could add okhttp3 as another optional dependency and put this stuff behind us. Users that use methods with okhttp types would then need to depend on the version of okhttp they wanted.

anuraaga commented 2 years ago

If you strongly cared, it'd seem you'd be using one.

I'd say the world is too nuanced for such generalizations in OSS :P Proguard being mostly automatic in Android builds but not simple outside of that doesn't preclude keeping size down by reducing actual dependent artifacts.

Thanks for the pointers and the tip on potential cleanup in Q4, that'll be quite nice.

valeriyo commented 1 year ago

most people don't realize how little of OkHttp we are using

Sounds fishy. If that's the case, just copy the "little" stuff that you're using and get rid of the dependency.

ejona86 commented 1 year ago

Sounds fishy. If that's the case, just copy the "little" stuff that you're using and get rid of the dependency.

It is on the API surface and, as I said, there was no technical reason it should be a priority. However, since https://github.com/grpc/grpc-java/pull/8971 (v1.46.0) we don't depend on okhttp 2; you depend on okhttp 2 yourself if you need to use the API that uses okhttp 2.