From b98563540c0a4edb38526bcd6e6c97f9fac1f453 Mon Sep 17 00:00:00 2001 From: Michael Stapelberg Date: Fri, 1 Nov 2024 13:50:07 +0100 Subject: [PATCH] internal/impl: fix TestMarshalMessageSetLazyRace (was a no-op!) This test was accidentally migrated from proto.GetExtension to proto.HasExtension in a semi-automated change and nobody noticed. proto.HasExtension does not actually trigger lazy extension decoding. I verified this using the dlv debugger: % go test -tags protolegacy -c -gcflags "all=-N -l" % dlv exec ./impl.test Type 'help' for list of commands. (dlv) r -test.run=TestMarshalMessageSetLazyRace -test.v Process restarted with PID 3586699 (dlv) b TestMarshalMessageSetLazyRace Breakpoint 1 set at 0xac9bf6 for google.golang.org/protobuf/internal/impl_test.TestMarshalMessageSetLazyRace() ./lazy_test.go:500 (dlv) c === RUN TestMarshalMessageSetLazyRace > [Breakpoint 1] google.golang.org/protobuf/internal/impl_test.TestMarshalMessageSetLazyRace() ./lazy_test.go:500 (hits goroutine(32):1 total:1) (PC: 0xac9bf6) => 500: func TestMarshalMessageSetLazyRace(t *testing.T) { (dlv) b ExtensionField.lazyInit Breakpoint 2 set at 0x8da076 for google.golang.org/protobuf/internal/impl.(*ExtensionField).lazyInit() ./codec_extension.go:123 (dlv) c --- PASS: TestMarshalMessageSetLazyRace (4.07s) PASS Process 3586699 has exited with status 0 (dlv) exit Note how breakpoint 2 is unexpectedly not hit! Here is how the output changes with my fix: % go test -tags protolegacy -c -gcflags "all=-N -l" % dlv --check-go-version=false exec ./impl.test Type 'help' for list of commands. (dlv) r -test.run=TestMarshalMessageSetLazyRace -test.v Process restarted with PID 3587344 (dlv) b TestMarshalMessageSetLazyRace Breakpoint 1 set at 0xac9bf6 for google.golang.org/protobuf/internal/impl_test.TestMarshalMessageSetLazyRace() ./lazy_test.go:500 (dlv) c === RUN TestMarshalMessageSetLazyRace > [Breakpoint 1] google.golang.org/protobuf/internal/impl_test.TestMarshalMessageSetLazyRace() ./lazy_test.go:500 (hits goroutine(28):1 total:1) (PC: 0xac9bf6) => 500: func TestMarshalMessageSetLazyRace(t *testing.T) { (dlv) b ExtensionField.lazyInit Breakpoint 2 set at 0x8da076 for google.golang.org/protobuf/internal/impl.(*ExtensionField).lazyInit() ./codec_extension.go:123 (dlv) c > [Breakpoint 2] google.golang.org/protobuf/internal/impl.(*ExtensionField).lazyInit() ./codec_extension.go:123 (hits goroutine(68):1 total:4) (PC: 0x8da076) > [Breakpoint 2] google.golang.org/protobuf/internal/impl.(*ExtensionField).lazyInit() ./codec_extension.go:123 (hits goroutine(70):1 total:4) (PC: 0x8da076) > [Breakpoint 2] google.golang.org/protobuf/internal/impl.(*ExtensionField).lazyInit() ./codec_extension.go:123 (hits goroutine(58):1 total:4) (PC: 0x8da076) > [Breakpoint 2] google.golang.org/protobuf/internal/impl.(*ExtensionField).lazyInit() ./codec_extension.go:123 (hits goroutine(54):1 total:4) (PC: 0x8da076) => 123: func (f *ExtensionField) lazyInit() { (dlv) bt 0 0x00000000008da076 in google.golang.org/protobuf/internal/impl.(*ExtensionField).lazyInit at ./codec_extension.go:123 1 0x00000000008dac3e in google.golang.org/protobuf/internal/impl.(*ExtensionField).Value at ./codec_extension.go:180 2 0x000000000094e5d9 in google.golang.org/protobuf/internal/impl.(*extensionMap).Get at ./message_reflect.go:276 3 0x000000000095a2ee in google.golang.org/protobuf/internal/impl.(*messageState).Get at ./message_reflect_gen.go:90 4 0x0000000000890099 in google.golang.org/protobuf/proto.GetExtension at /usr/local/google/home/stapelberg/protobuf/proto/extension.go:90 5 0x0000000000aca7d4 in google.golang.org/protobuf/internal/impl_test.TestMarshalMessageSetLazyRace.func2.1 at ./lazy_test.go:545 6 0x0000000000aca625 in google.golang.org/protobuf/internal/impl_test.TestMarshalMessageSetLazyRace.func2 at ./lazy_test.go:550 7 0x00000000006001c1 in runtime.goexit at /usr/lib/google-golang/src/runtime/asm_amd64.s:1700 Change-Id: Ie7a8621064af412a1db7efb3ad6b8473f9db58e8 Reviewed-on: https://go-review.googlesource.com/c/protobuf/+/624416 LUCI-TryBot-Result: Go LUCI Reviewed-by: Chressie Himpel --- internal/impl/lazy_test.go | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/internal/impl/lazy_test.go b/internal/impl/lazy_test.go index e9d6e8816..fe3035b09 100644 --- a/internal/impl/lazy_test.go +++ b/internal/impl/lazy_test.go @@ -542,7 +542,8 @@ func TestMarshalMessageSetLazyRace(t *testing.T) { go func() { defer wg.Done() if err := func() error { - if !proto.HasExtension(h.GetData(), lazytestpb.E_Rabbit_MessageSetExtension) { + mm := proto.GetExtension(h.GetData(), lazytestpb.E_Rabbit_MessageSetExtension).(*lazytestpb.Rabbit) + if mm == nil { return errors.New("proto: missing extension") } return nil