vendor: can't upgrade to grpc-go v1.34 #60531
Labels
C-investigation
Further steps needed to qualify. C-label will change.
Comments
stevendanna
added
the
C-investigation
stevendanna
added a commit
to stevendanna/cockroach
that referenced
this issue
Feb 18, 2021
This change enables future upgrades of google.golang.org/grpc. Recent changes to google.golang.org/grpc have changed the default proto codec such that it is incompatible with gogoproto: grpc/grpc-go#3958 While `proto.Marshal` does do a check for the Marshal/Unmarshal it appears to do this through reflection methods that will panic on fields using gogoproto.nullable. To avoid this, we install a proto codec that does an interface check before dispatching to proto.Marshal/proto.Unmarshal. Note that I haven't replicated the protobuffer caching that previously existed in google.golang.org/grpc. The referenced change claims removing the caching may be a small performance win; however, they also removed the typecheck that we need to do, so it is unclear. Fixes cockroachdb#60531 Release note: None
stevendanna
added a commit
to stevendanna/cockroach
that referenced
this issue
Mar 10, 2021
This change enables future upgrades of google.golang.org/grpc. Recent changes to google.golang.org/grpc have changed the default proto codec such that it is incompatible with gogoproto: grpc/grpc-go#3958 While `proto.Marshal` does do a check for the Marshal/Unmarshal it appears to do this through reflection methods that will panic on fields using gogoproto.nullable. To avoid this, we install a proto codec that does an interface check before dispatching to proto.Marshal/proto.Unmarshal. Note that I haven't replicated the protobuffer caching that previously existed in google.golang.org/grpc. The referenced change claims removing the caching may be a small performance win; however, they also removed the typecheck that we need to do, so it is unclear. Fixes cockroachdb#60531 Release note: None
stevendanna
added a commit
to stevendanna/cockroach
that referenced
this issue
Mar 18, 2021
This change enables future upgrades of google.golang.org/grpc. Recent changes to google.golang.org/grpc have changed the default proto codec such that it is incompatible with gogoproto: grpc/grpc-go#3958 While `proto.Marshal` does do a check for the Marshal/Unmarshal it appears to do this through reflection methods that will panic on fields using gogoproto.nullable. To avoid this, we install a proto codec that does an interface check before dispatching to proto.Marshal/proto.Unmarshal. Note that I haven't replicated the protobuffer caching that previously existed in google.golang.org/grpc. The referenced change claims removing the caching may be a small performance win; however, they also removed the typecheck that we need to do, so it is unclear. Fixes cockroachdb#60531 Release note: None
|
I think the scope of this is different from #56147 although we could certainly choose to fix the problem as part of that work. Some gogoproto-generated Go code is incompatible with the latest version of google.golang.org/grpc and the version of github.com/golang/protobuf it depends on. This incompatibility exists even using the most recent version of gogoproto. Thus, if we complete #56147 and upgrade gogoproto, we would still see breakage when upgrading google.golang.org/grpc. The incompatibility exists because the default "codec" in the gRPC library to marshal protobufs no longer does interface checks for custom Marshaler functions (grpc/grpc-go#3958) and assumes that However, since this issue was originally written, the scope of incompatibility has narrowed. Before, the proto.Marshal function would fail on any field using the gogoproto's support for 'non-nullable' fields. However, their most recent release adds support for "aberrant types" which includes messages with non-nullable fields. Unfortunately, even with this change, they still don't seem to support gogoproto's customtypes options. If you currently attempt to upgrade grpc, You will get this same error even if you regenerate all of the protos with the most recent version of gogoproto (at least, I did in a small local experiment). One way to fix this is likely the same fix that I've already submitted (although I haven't retested it). However, given the narrower scope of the incompatibility, there may be other paths available to fix it as well. |
|
thanks for explaining |
craig bot
pushed a commit
that referenced
this issue
Mar 23, 2021
61333: rpc: install custom proto codec r=[erikgrinaker,pbardea] a=stevendanna This change enables future upgrades of google.golang.org/grpc. Recent changes to google.golang.org/grpc have changed the default proto codec such that it is incompatible with gogoproto: grpc/grpc-go#3958 While `proto.Marshal` does do a check for the Marshal/Unmarshal it appears to do this through reflection methods that will panic on fields using gogoproto.nullable. To avoid this, we install a proto codec that does an interface check before dispatching to proto.Marshal/proto.Unmarshal. Note that I haven't replicated the protobuffer caching that previously existed in google.golang.org/grpc. The referenced change claims removing the caching may be a small performance win; however, they also removed the typecheck that we need to do, so it is unclear. Fixes #60531 Release note: None Co-authored-by: Steven Danna <danna@cockroachlabs.com>
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Describe the problem
Cockroach built with grpc-go v1.34 panics shortly after start with the following error when running
./cockroach demo:I believe the likely cause is grpc/grpc-go#3958.
It isn't immediately necessary to upgrade this dependency, so there is no impact at the moment.
The text was updated successfully, but these errors were encountered: