status/Details: unexpected code breaks after upgrading to v1.62.0 and higher #7679
Comments
povsister
changed the title
arjan-bal
added
the
Area: RPC Features
|
I tried casting the output of @povsister can you share the generated |
|
@arjan-bal
I am not sure which version of Here is the snippet of the generated code. package pb
import (
fmt "fmt"
proto "github.com/golang/protobuf/proto"
math "math"
anypb "google.golang.org/protobuf/types/known/anypb"
)
// Reference imports to suppress errors if they are not otherwise used.
var _ = proto.Marshal
var _ = fmt.Errorf
var _ = math.Inf
// This is a compile-time assertion to ensure that this generated file
// is compatible with the proto package it is being compiled against.
// A compilation error at this line likely means your copy of the
// proto package needs to be updated.
const _ = proto.ProtoPackageIsVersion2 // please upgrade the proto package
type Error struct {
ErrCode int32 `protobuf:"varint,1,opt,name=err_code,json=errCode,proto3" json:"err_code,omitempty"`
ErrMessage string `protobuf:"bytes,2,opt,name=err_message,json=errMessage,proto3" json:"err_message,omitempty"`
ErrDetail *anypb.Any `protobuf:"bytes,3,opt,name=err_detail,json=errDetail,proto3" json:"err_detail,omitempty"`
XXX_NoUnkeyedLiteral struct{} `json:"-"`
XXX_unrecognized []byte `json:"-"`
XXX_sizecache int32 `json:"-"`
}
func (m *Error) Reset() { *m = Error{} }
func (m *Error) String() string { return proto.CompactTextString(m) }
func (*Error) ProtoMessage() {}
func (*Error) Descriptor() ([]byte, []int) {
return fileDescriptor_ecode_9897508b96e13b12, []int{0}
}
func (m *Error) XXX_Unmarshal(b []byte) error {
return xxx_messageInfo_Error.Unmarshal(m, b)
}
func (m *Error) XXX_Marshal(b []byte, deterministic bool) ([]byte, error) {
return xxx_messageInfo_Error.Marshal(b, m, deterministic)
}
func (dst *Error) XXX_Merge(src proto.Message) {
xxx_messageInfo_Error.Merge(dst, src)
}
func (m *Error) XXX_Size() int {
return xxx_messageInfo_Error.Size(m)
}
func (m *Error) XXX_DiscardUnknown() {
xxx_messageInfo_Error.DiscardUnknown(m)
}
var xxx_messageInfo_Error proto.InternalMessageInfo
func (m *Error) GetErrCode() int32 {
if m != nil {
return m.ErrCode
}
return 0
}
func (m *Error) GetErrMessage() string {
if m != nil {
return m.ErrMessage
}
return ""
}
func (m *Error) GetErrDetail() *anypb.Any {
if m != nil {
return m.ErrDetail
}
return nil
}
func init() {
proto.RegisterType((*Error)(nil), "err.Error")
}
func init() {
proto.RegisterFile("status/internal/pb/ecode.proto", fileDescriptor_ecode_9897508b96e13b12)
}
var fileDescriptor_ecode_9897508b96e13b12 = []byte{
// 206 bytes of a gzipped FileDescriptorProto
0x1f, 0x8b, 0x08, 0x00, 0x00, 0x00, 0x00, 0x00, 0x02, 0xff, 0x84, 0x8f, 0x3f, 0x4f, 0xc3, 0x30,
0x10, 0xc5, 0x15, 0xaa, 0x02, 0x75, 0x37, 0x8b, 0x21, 0x65, 0x21, 0x62, 0xca, 0x00, 0x3e, 0x89,
0x7e, 0x02, 0xfe, 0x8d, 0x2c, 0x19, 0x59, 0x2a, 0x3b, 0x3e, 0xac, 0x48, 0x8e, 0x2f, 0x7a, 0x31,
0x82, 0x7c, 0x7b, 0x94, 0x44, 0xcc, 0x1d, 0xef, 0xfd, 0xee, 0xee, 0xa7, 0xa7, 0x4c, 0xec, 0x1c,
0x2c, 0x26, 0x4a, 0x9c, 0x09, 0x43, 0x4b, 0x3f, 0x16, 0x9e, 0x13, 0x75, 0x29, 0x33, 0x92, 0x8d,
0x34, 0x38, 0xe2, 0x56, 0x3c, 0x9b, 0x01, 0x92, 0x45, 0x6f, 0x18, 0xb8, 0x3d, 0x04, 0x91, 0x10,
0x99, 0x96, 0xc8, 0x7d, 0x7f, 0x91, 0x4d, 0xd3, 0xca, 0xef, 0x7f, 0xd5, 0xf6, 0x1d, 0x10, 0xe8,
0x83, 0xba, 0x66, 0xe0, 0x34, 0x9f, 0x96, 0x45, 0x55, 0xd4, 0xdb, 0xe6, 0x8a, 0x81, 0x57, 0xf1,
0xac, 0xef, 0xd4, 0x7e, 0x46, 0x3d, 0x8f, 0xa3, 0x0d, 0x5c, 0x5e, 0x54, 0x45, 0xbd, 0x6b, 0x14,
0x03, 0x1f, 0x6b, 0xa2, 0x8f, 0x6a, 0x9e, 0x4e, 0x9e, 0xb3, 0xed, 0x62, 0xb9, 0xa9, 0x8a, 0x7a,
0xff, 0x74, 0x63, 0x56, 0xa9, 0xf9, 0x97, 0x9a, 0xe7, 0x34, 0x35, 0x3b, 0x06, 0xde, 0x96, 0xb5,
0x17, 0xf3, 0xf9, 0x10, 0xe4, 0xb1, 0x95, 0xbe, 0x97, 0x44, 0xe7, 0x5b, 0xb9, 0xcb, 0xe5, 0xd1,
0xf1, 0x2f, 0x00, 0x00, 0xff, 0xff, 0x4e, 0xef, 0xdc, 0x8d, 0x02, 0x01, 0x00, 0x00,
} |
|
Thanks for the details. I was able to repro the issue after generating a |
|
Great! Please keep me updated if you are going to fix that. |
|
@povsister I discussed the issue with other maintainers. The change (#6919) seems to be backward incompatible when using old generated protobuf code. Since we've not gotten other reports about this issue since 1.62.0 was released 6 months ago, we don't want to make a change now. You should be able to unblock yourself by either regenerating the |
Not exactly, at least you have reproduced this issue with
That's ok to me. But, as I have mentioned in the thread, gRPC doesn't make any guarantees on return types of which is quite confusing:
Those questions are still unclear, the only way we can figure it out is to read the code of gRPC. But it doesn't help much on keeping long-term compatibility, that's the reason I am still here. Let me give you a detail example of what I am concerned. So, 2 ways beneath should work.
They did work before v1.62.0, But both fails after upgraing to grpc-go >= v1.62.0. I have several suggestion:
|
I meant to say the change is backaward "incompatible". Sorry about the confusion. I was able to get the failing test in the repro to pass by wrapping the messages using Let me get opinions from the team and get back. |
|
Okay, I was feeling weird but didn't aware that was a typo.
Since there is no more report about this issue, I prefer make some enhancement instead of keeping 100% compatibility. Thus, I think concrete messages of return values make more sense in this situation. |
|
Wouldn't changing the return type require a new API because the current |
|
Literally speaking, I don't think this is a API change cause the return type of The key is "what type of values gRPC guarantees to return?" There is no guarantees before, so we inferred it by reading code of gRPC. But it has been implicitly/unintentionally changed during legacy deps cleanup for more than 6 months and nobody except us noticed. It proves this is not very risky, so I think introducing new a API is not necessary. It's more important to make things right and elegant. |
There are users of the The doc comment is also a contract, even though it isn't enforced by the compiler. If we were to ensure that the type Ret interface {
error
protoiface.MessageV1
}However, implementing both interface is not the same as implementing either of the interface. So the code that does a type switch on the returned value will never execute the second case statement after the change: switch v := a.(type) {
case protoiface.MessageV1:
// Always gets executed now.
case error:
// Dead code.
}Based on this, I'm planning to keep the return types the same to avoid breaking users. I'll make the change to ensure that the returned value implements |
|
Sorry I just realized I might not make myself clear. I am not suggesting narrow return type of As you have written above, using Actually, this issue is caused by implicitly wrapping old-generated v1 messages with What's worse, the approach you suggest may cause future problems if gRPC decides to remove So I belive the root problem is: We might don't need to wrap it, just returning the vanilla-generated Go structs registered in protobuf registry is more viable.
// Details returns a slice of proto.Message(either v1 or v2) attached to the status.
// If a detail cannot be decoded through anypb.Unmarshal, the error is returned in place of the detail.
// It is guaranted that proto.Message is in its vanilla type registered in protobuf registry.
// So users can always try casting it into generated vanilla Go structs,
// or you can cast it into MessageV1/V2 interface which depends on the generated code.
func (s *Status) Details() []any {
// 1. Get a instance of vanilla Go type registered in protobuf registry.
// 2. Using `anypb.UnmarshalTo` to set data into Go structs.
// 3. Return instance from 1 or error from 2, without wrapping.
}I am not good at English, if you have anything unclear, please let me know. |
|
In addition, I just realized that: even before v1.62.0, https://github.com/grpc/grpc-go/pull/6919/files#diff-d78e63ce979e5c67b7fa98a36052a2529e4232343046533343d9772c0cd52549R157-R160 As for now, generated code from latest So literally, v1.62.0 indeed introduced a bug while unmarshaling anypb from gRPC status. The only problem is, this bug has been released for more than 6 months, and someone may rely on this "bug feature" to work properly. // Empty returns a new message of the type specified in an anypb.Any message.
// It returns protoregistry.NotFound if the corresponding message type could not
// be resolved in the global registry.
//
// Deprecated: Use protoregistry.GlobalTypes.FindMessageByName instead
// to resolve the message name and create a new instance of it.
func Empty(any *anypb.Any) (proto.Message, error) {
name, err := anyMessageName(any)
if err != nil {
return nil, err
}
mt, err := protoregistry.GlobalTypes.FindMessageByName(name)
if err != nil {
return nil, err
}
return proto.MessageV1(mt.New().Interface()), nil
} |
|
@povsister to summarize, before 1.62, The reported issue is caused because func getMessageInstance(anyMsg *anypb.Any) (proto.Message, error) {
// Lookup the message type in the global registry
msgType, err := protoregistry.GlobalTypes.FindMessageByURL(anyMsg.GetTypeUrl())
if err != nil {
return nil, fmt.Errorf("could not find message type: %v", err)
}
// Create a new instance of the message type
msg := msgType.New().Interface()
if _, ok := msg.(protoadapt.MessageV1); !ok {
fmt.Println("Message doesn't implement V1 interface", msg)
}
return msg, nil
}If this is true, then the second approach suggested by you in #7679 (comment) doesn't work, right? |
|
Can you give an example code snippet that would break if we make the change to wrap the status protos in |
Yes, the returned type always implements The key point is: even before 1.62, Adding
No, this does not work as you thought. // Interface unwraps the message reflection interface and
// returns the underlying ProtoMessage interface.
Interface() ProtoMessageSo, there is nothing to do with The problem is: wrapped message is directly returned by
Yes, the snippet given by your comments will fail on this issue.
Sure, as I have said, if we have // generated message which only implements MessageV2 interface
type V2OnlyMessage struct {
...
}
func (m *V2OnlyMessage) ProtoReflect() protoreflect.Message {
...
}
func getCustomizedMsgFromGST(gst *status.Status) (*V2OnlyMessage, error) {
for _, st := range gst.Details() {
var v2Msg protoV2.Message
switch pbMsg := st.(type) {
case protoV2.Message:
v2Msg = pbMsg
case proto.Message:
v2Msg = protoadapt.MessageV2Of(pbMsg)
default:
return nil, fmt.Errorf("not a proto message: %T", st)
}
if v2Msg.ProtoReflect().Descriptor().FullName() ==
(*V2OnlyMessage)(nil).ProtoReflect().Descriptor().FullName() {
// This works before 1.62, and also works for now.
// But it will fail if protoadapt.MessageV1Of() called inside status.Details().
// Because return values will be wrapped, just like what we are discussing.
if retMsg, ok := v2Msg.(*V2OnlyMessage); ok {
return retMsg, nil
} else {
// How can we safely cast it into vanilla form of Go structs ??
}
}
}
}In a nutshell, adding |
This is a problem that gRPC Go will need to be solve when the V1 API is removed anyways, since the grpc-go/internal/status/status.go Lines 132 to 134 in bdd444d |
|
Yeah, I agree that this is a problem that gRPC Go will need to be solve in the future. But, code will suddenly stop working if user re-generates their
I don't think they are equivalent. Implicite wrappers have already caused one problem here, I wish not see same thing happens again in future. |
|
Let me make it more clear and practical:
|
|
To clarify, is the following understanding correct?
|
Totally correct |
What version of gRPC are you using?
v1.62.0 and higher
What version of Go are you using (
go version)?go version go1.23.1 darwin/arm64
What operating system (Linux, Windows, …) and version?
Not related
What did you do?
After upgrading to v1.62.0, we have encountered serious problem on gRPC status detail extension: Our gRPC clients can not properly retrieve the customized err detail message from gRPC status.
After some investigation, we found it is caused by PR #6919
internal/status/status.go L157-L160https://github.com/grpc/grpc-go/pull/6919/files#diff-d78e63ce979e5c67b7fa98a36052a2529e4232343046533343d9772c0cd52549R157-R160
The problem is: status.Details always return
proto.MessageV1before, but it now implicitly returnsproto.MessageV2because of changingprotoV1.UnmarshalAnytoany.UnmarshalNew.This implicit behavior actually introduces compatibility issue, though gRPC itself doesn't make any guarantees on that.
Here is an example of our code extracting customized err details.
It unexpctly breaks after upgrading to v1.62.0 and higher.
What did you expect to see?
Our Code continues to work as expected after upgrading.
What did you see instead?
Code breaks because of implicit changes of return values from gRPC.
The text was updated successfully, but these errors were encountered: