Upgrade gRPC-C++ to 1.28.0 #4312
Conversation
b2749b2 to
90998a3
Compare
These are now exposed because libprotobuf is no longer polluting the compiler command-line with -Wno-unused-parameter.
90998a3 to
4efb5c4
Compare
In addition to the version change, this comes with some other changes:
* gRPC now depends upon Abseil, so we no longer directly include
abseil-cpp for ourselves.
* gRPC's CMake system now allows all interesting dependencies to
specify their location via a ROOT_DIR variable (e.g.
ABSL_ROOT_DIR). Use these to move sources outside the gRPC source
tree, removing the need for a separate grpc-download target. This
fixes the problem where each gRPC upgrade would wipe out its
dependencies.
This unblocks nanopb changes proposed in #4264.
Use a non-empty hostname for gRPC testing gRPC doesn't validate this but the new Abseil version asserts that gRPC accesses past the end of the string when passed an empty hostname.
4efb5c4 to
403264b
Compare
|
Note that I've rebased this change onto current master and updated the versions this tackles. So far this hasn't run into any of the issues I saw with gRPC 1.25.0. |
|
Argh. I've been re-running the integration tests repeatedly and can still see the occasional flake. This isn't safe to merge as-is :-(. |
Disable CFStream-based transport on Apple platforms due to b/133182964, wherein CFStream will occasionally fail to raise a has-bytes-available events, causing Firestore to appear to hang. Without this, about 1 in 100 Firestore integration tests will hang after making a request.
|
PTAL, 5e74ae5 represents a significant change. |
| // CFStream will occasionally fail to raise a has-bytes-available events, | ||
| // causing Firestore to appear to hang. | ||
| // | ||
| // Use a constructor of a globally scoped object to set the `grpc_cfstream` |
There was a problem hiding this comment.
This is ingenious, though I'd like to double-check if it's necessary. Does the gRPC team recommend doing it this way? Is there perhaps some well-defined point better suited for setenv calls? Note that this is mostly me playing the devil's advocate -- I'm pretty sure this is a good implementation.
There was a problem hiding this comment.
gRPC team didn't have any more advice than "set grpc_cfstream=0". Here's their official documentation: https://github.com/grpc/grpc/blob/master/src/objective-c/README-CFSTREAM.md
In general, the best way to handle calls to setenv is to put them at the top of your main function. This avoids data races that are otherwise inherent in multiple threads setting and getting environment variables concurrently. As a library author we don't really control main so it's kind of a crapshoot as to whether or not we'll make the call before anyone else uses gRPC.
Indeed, my initial implementation was to put this in the GrpcConnection constructor:
#if __APPLE__
static std::once_flag grpc_cfstream_disabled;
std::call_once(grpc_cfstream_disabled, [] {
setenv("grpc_cfstream", "0", 1);
});
#endif // __APPLE__However, this didn't work. In gRPC-C++, the call to grpc_init happens implicitly and it seems to happen for essentially any API call. Other calls must have happened earlier and prevented the variable from being read.
I looked for other places insert this to make this earlier (for example, making a static Datastore::InitGrpc and calling it from the api::Firestore constructor (or similar) but this causes the problem that every different way we construct Firestore or FirestoreClient instances has to be adjusted or it will be subject to flakes if we forget. I didn't want to take the chance that we might forget to make this call so this approach seemed better.
There was a problem hiding this comment.
Thanks for elaborating on the rationale. I think it's a great solution.
This unblocks nanopb changes proposed in #4264. This upgrades everything in the gRPC constellation of dependencies to match gRPC at 1.28: * gRPC to 1.28.0 (note that gRPC 1.28.1, while released, is a fix for just the python binding and there's no gRPC-C++ CocoaPod at that version) * Abseil to 20200225 (as of gRPC 1.27 this is now a gRPC dependency too; there's a later patch but gRPC doesn't use it yet) * Protobuf C++ to 3.11.4 (only used for testing in Firestore) * C-Ares to 1.15.0 * BoringSSL to match (no change between gRPC 1.27.0 and 1.28.0) Also, this disables CFStream-based transport on Apple platforms. This works around b/133182964, wherein CFStream will occasionally fail to raise a has-bytes-available events, causing Firestore to appear to hang.
This unblocks nanopb changes proposed in #4264.
This upgrades everything in the gRPC constellation of dependencies to match gRPC at 1.28:
This is best reviewed as individual commits, largely ignoring 243bef4, which is a commit of generated code to keep our CocoaPod-based build simple.