-
Notifications
You must be signed in to change notification settings - Fork 3.9k
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
OkHttp is incompatible with newer C-core binaries #3032
Comments
@ericgribkoff to test in the Android interop he's working on. |
We don't run okhttp interop externally (full cross-product testing), but we do internally. We think the breaking change was delayed being brought internally for long enough that it was released without the test having failed yet. |
Created #3036 to address this, although I'm not sure what restrictions we have with modifying the third_party okhttp code. The bug was noticed on OkHttp3 (square/okhttp#2299), although I believe their fix still fails for the experimental settings ids used by C++.
This appears to have been the case. I did some work to add a gRPC Java OkHttp client to the grpc/grpc interop test suite, but encountered some issues with getting our test certificates working with OkHttp. I will look further into this tomorrow, as it seems like we definitely should be testing OkHttp clients as part of our external interops, to (help) avoid issues like this. |
This was fixed in #3036 |
As reported by grpc/grpc#11258. This is caused by grpc/proposal#19 and a bug in the OkHttp transport that doesn't ignore unknown settings frames (as required by the HTTP/2 spec).
It's unclear how this wasn't caught in the integration tests.
The text was updated successfully, but these errors were encountered: