From bb74ef09ac696d5c0d21f4c4508c54f7cfbc8ffa Mon Sep 17 00:00:00 2001 From: Josh Humphries <2035234+jhump@users.noreply.github.com> Date: Tue, 9 Apr 2024 16:24:54 -0400 Subject: [PATCH] fix caching when wrapping protoreflect.Descriptor instances --- desc/cache.go | 9 --------- desc/convert.go | 4 +--- desc/load.go | 7 ------- desc/wrap.go | 21 ++++++++++++--------- 4 files changed, 13 insertions(+), 28 deletions(-) diff --git a/desc/cache.go b/desc/cache.go index e67cf494..418632b7 100644 --- a/desc/cache.go +++ b/desc/cache.go @@ -46,12 +46,3 @@ func (c mapCache) get(d protoreflect.Descriptor) Descriptor { func (c mapCache) put(key protoreflect.Descriptor, val Descriptor) { c[key] = val } - -type noopCache struct{} - -func (noopCache) get(protoreflect.Descriptor) Descriptor { - return nil -} - -func (noopCache) put(protoreflect.Descriptor, Descriptor) { -} diff --git a/desc/convert.go b/desc/convert.go index 9aa72a32..01a6e9ea 100644 --- a/desc/convert.go +++ b/desc/convert.go @@ -95,9 +95,7 @@ func convertFile(d protoreflect.FileDescriptor, fd *descriptorpb.FileDescriptorP ret.deps = make([]*FileDescriptor, len(fd.GetDependency())) for i := 0; i < d.Imports().Len(); i++ { f := d.Imports().Get(i).FileDescriptor - if c := cache.get(f); c != nil { - ret.deps[i] = c.(*FileDescriptor) - } else if c, err := wrapFile(f, cache); err != nil { + if c, err := wrapFile(f, cache); err != nil { return nil, err } else { ret.deps[i] = c diff --git a/desc/load.go b/desc/load.go index 193bbe88..8776ab0b 100644 --- a/desc/load.go +++ b/desc/load.go @@ -53,13 +53,6 @@ func LoadFileDescriptor(file string) (*FileDescriptor, error) { var fd *FileDescriptor loadedDescriptors.withLock(func(cache descriptorCache) { - // double-check cache, in case it was concurrently added while - // we were waiting for the lock - f := cache.get(d) - if f != nil { - fd = f.(*FileDescriptor) - return - } fd, err = wrapFile(d, cache) }) return fd, err diff --git a/desc/wrap.go b/desc/wrap.go index 82610a45..5491afda 100644 --- a/desc/wrap.go +++ b/desc/wrap.go @@ -21,7 +21,7 @@ type DescriptorWrapper interface { // WrapDescriptor wraps the given descriptor, returning a desc.Descriptor // value that represents the same element. func WrapDescriptor(d protoreflect.Descriptor) (Descriptor, error) { - return wrapDescriptor(d, noopCache{}) + return wrapDescriptor(d, mapCache{}) } func wrapDescriptor(d protoreflect.Descriptor, cache descriptorCache) (Descriptor, error) { @@ -65,10 +65,13 @@ func WrapFiles(d []protoreflect.FileDescriptor) ([]*FileDescriptor, error) { // WrapFile wraps the given file descriptor, returning a *desc.FileDescriptor // value that represents the same file. func WrapFile(d protoreflect.FileDescriptor) (*FileDescriptor, error) { - return wrapFile(d, noopCache{}) + return wrapFile(d, mapCache{}) } func wrapFile(d protoreflect.FileDescriptor, cache descriptorCache) (*FileDescriptor, error) { + if res := cache.get(d); res != nil { + return res.(*FileDescriptor), nil + } fdp := protoutil.ProtoFromFileDescriptor(d) return convertFile(d, fdp, cache) } @@ -76,7 +79,7 @@ func wrapFile(d protoreflect.FileDescriptor, cache descriptorCache) (*FileDescri // WrapMessage wraps the given message descriptor, returning a *desc.MessageDescriptor // value that represents the same message. func WrapMessage(d protoreflect.MessageDescriptor) (*MessageDescriptor, error) { - return wrapMessage(d, noopCache{}) + return wrapMessage(d, mapCache{}) } func wrapMessage(d protoreflect.MessageDescriptor, cache descriptorCache) (*MessageDescriptor, error) { @@ -97,7 +100,7 @@ func wrapMessage(d protoreflect.MessageDescriptor, cache descriptorCache) (*Mess // WrapField wraps the given field descriptor, returning a *desc.FieldDescriptor // value that represents the same field. func WrapField(d protoreflect.FieldDescriptor) (*FieldDescriptor, error) { - return wrapField(d, noopCache{}) + return wrapField(d, mapCache{}) } func wrapField(d protoreflect.FieldDescriptor, cache descriptorCache) (*FieldDescriptor, error) { @@ -121,7 +124,7 @@ func wrapField(d protoreflect.FieldDescriptor, cache descriptorCache) (*FieldDes // WrapOneOf wraps the given oneof descriptor, returning a *desc.OneOfDescriptor // value that represents the same oneof. func WrapOneOf(d protoreflect.OneofDescriptor) (*OneOfDescriptor, error) { - return wrapOneOf(d, noopCache{}) + return wrapOneOf(d, mapCache{}) } func wrapOneOf(d protoreflect.OneofDescriptor, cache descriptorCache) (*OneOfDescriptor, error) { @@ -138,7 +141,7 @@ func wrapOneOf(d protoreflect.OneofDescriptor, cache descriptorCache) (*OneOfDes // WrapEnum wraps the given enum descriptor, returning a *desc.EnumDescriptor // value that represents the same enum. func WrapEnum(d protoreflect.EnumDescriptor) (*EnumDescriptor, error) { - return wrapEnum(d, noopCache{}) + return wrapEnum(d, mapCache{}) } func wrapEnum(d protoreflect.EnumDescriptor, cache descriptorCache) (*EnumDescriptor, error) { @@ -159,7 +162,7 @@ func wrapEnum(d protoreflect.EnumDescriptor, cache descriptorCache) (*EnumDescri // WrapEnumValue wraps the given enum value descriptor, returning a *desc.EnumValueDescriptor // value that represents the same enum value. func WrapEnumValue(d protoreflect.EnumValueDescriptor) (*EnumValueDescriptor, error) { - return wrapEnumValue(d, noopCache{}) + return wrapEnumValue(d, mapCache{}) } func wrapEnumValue(d protoreflect.EnumValueDescriptor, cache descriptorCache) (*EnumValueDescriptor, error) { @@ -176,7 +179,7 @@ func wrapEnumValue(d protoreflect.EnumValueDescriptor, cache descriptorCache) (* // WrapService wraps the given service descriptor, returning a *desc.ServiceDescriptor // value that represents the same service. func WrapService(d protoreflect.ServiceDescriptor) (*ServiceDescriptor, error) { - return wrapService(d, noopCache{}) + return wrapService(d, mapCache{}) } func wrapService(d protoreflect.ServiceDescriptor, cache descriptorCache) (*ServiceDescriptor, error) { @@ -193,7 +196,7 @@ func wrapService(d protoreflect.ServiceDescriptor, cache descriptorCache) (*Serv // WrapMethod wraps the given method descriptor, returning a *desc.MethodDescriptor // value that represents the same method. func WrapMethod(d protoreflect.MethodDescriptor) (*MethodDescriptor, error) { - return wrapMethod(d, noopCache{}) + return wrapMethod(d, mapCache{}) } func wrapMethod(d protoreflect.MethodDescriptor, cache descriptorCache) (*MethodDescriptor, error) {