From f98dd98c15bbc3e8accc353f5bff6fca06657a4a Mon Sep 17 00:00:00 2001 From: Joe Tsai Date: Mon, 5 Aug 2019 13:41:37 -0700 Subject: [PATCH] internal/filedesc: defer evaluation of weak reference until actual use Suppose some init logic performs protobuf reflection. If so, it will cause the lazy descriptor init logic to be executed. This is problematic for weak fields since we can not be certain that all weak references have been registered at this point in time. This is a fundamental issue with with weak dependencies, since it means that we cannot enforce any ordering constraints on the weak dependency unless we directly import the weakly referenced package (which would defeat the point of weak imports). Alleviate the problem by pushing evaluation of weak reference to actual usage. This does not completely fix the problem, but signifcantly reduces the probability of it being problematic. In general, people should avoid interacting with weak fields at init time. Change-Id: Ie5957ddedd61333e72ee9a1bba0c53dace65547c Reviewed-on: https://go-review.googlesource.com/c/protobuf/+/188982 Reviewed-by: Damien Neil --- internal/filedesc/desc.go | 18 ++++++++++++++---- internal/filedesc/desc_lazy.go | 6 +----- 2 files changed, 15 insertions(+), 9 deletions(-) diff --git a/internal/filedesc/desc.go b/internal/filedesc/desc.go index e69856337..f18428f86 100644 --- a/internal/filedesc/desc.go +++ b/internal/filedesc/desc.go @@ -16,6 +16,7 @@ import ( "google.golang.org/protobuf/internal/pragma" "google.golang.org/protobuf/internal/strs" pref "google.golang.org/protobuf/reflect/protoreflect" + "google.golang.org/protobuf/reflect/protoregistry" ) // The types in this file may have a suffix: @@ -309,10 +310,19 @@ func (fd *Field) ContainingOneof() pref.OneofDescriptor { return fd.L1.Cont func (fd *Field) ContainingMessage() pref.MessageDescriptor { return fd.L0.Parent.(pref.MessageDescriptor) } -func (fd *Field) Enum() pref.EnumDescriptor { return fd.L1.Enum } -func (fd *Field) Message() pref.MessageDescriptor { return fd.L1.Message } -func (fd *Field) Format(s fmt.State, r rune) { descfmt.FormatDesc(s, r, fd) } -func (fd *Field) ProtoType(pref.FieldDescriptor) {} +func (fd *Field) Enum() pref.EnumDescriptor { + return fd.L1.Enum +} +func (fd *Field) Message() pref.MessageDescriptor { + if fd.L1.IsWeak { + if d, _ := protoregistry.GlobalFiles.FindDescriptorByName(fd.L1.Message.FullName()); d != nil { + return d.(pref.MessageDescriptor) + } + } + return fd.L1.Message +} +func (fd *Field) Format(s fmt.State, r rune) { descfmt.FormatDesc(s, r, fd) } +func (fd *Field) ProtoType(pref.FieldDescriptor) {} // EnforceUTF8 is a pseudo-internal API to determine whether to enforce UTF-8 // validation for the string field. This exists for Google-internal use only diff --git a/internal/filedesc/desc_lazy.go b/internal/filedesc/desc_lazy.go index ae9358ac0..1e3167645 100644 --- a/internal/filedesc/desc_lazy.go +++ b/internal/filedesc/desc_lazy.go @@ -32,12 +32,8 @@ func (file *File) resolveMessages() { for j := range md.L2.Fields.List { fd := &md.L2.Fields.List[j] - // Weak fields are only resolved by name. + // Weak fields are resolved upon actual use. if fd.L1.IsWeak { - r := file.builder.FileRegistry - if d, _ := r.FindDescriptorByName(fd.L1.Message.FullName()); d != nil { - fd.L1.Message = d.(pref.MessageDescriptor) - } continue }