protobuf

protobuf copied to clipboard

Generated messages have mutexes and other variables that make it dangerous to shallow copy.

Add a function to the proto package that does the equivalent of a shallow copy in a safe way.

The function would semantically equivalent to:

func ShallowCopy(dst, src proto.Message) {
     dm := dst.ProtoReflect()
     sm := src.Protoreflect()
     if dm.Type() != sm.Type() {
         panic("mismatching type")
     }
     proto.Reset(dst)
     sm.Range(func(fd protoreflect.FieldDescriptor, v protoreflect.Value) bool {
         dm.Set(fd, v)
         return true
     })
}

but be optimized for performance.

I have just had to abandon an upgrade because we copy messages. Because they now contain mutexes the copylocks check of the go vet command now fails in numerous places. So I need this for correctness - it is a serious error to copy mutexes which explains why 'go vet' has check explicitly for when this happens.

I have just had to abandon an upgrade because we copy messages. Because they now contain mutexes the copylocks check of the go vet command now fails in numerous places.

The data structures that were not safe for shallow copying have always been present since v1.1 (and possibly earlier). The only difference is that we made it possible for vet to detect such cases more readily. In other words, just because vet didn't flag it for v1.3 and earlier doesn't mean it was okay on those versions.

I have just had to abandon an upgrade because we copy messages. Because they now contain mutexes the copylocks check of the go vet command now fails in numerous places.

The data structures that were not safe for shallow copying have always been present since v1.1 (and possibly earlier). The only difference is that we made it possible for vet to detect such cases more readily. In other words, just because vet didn't flag it for v1.3 and earlier doesn't mean it was okay on those versions.

It isn't obvious to users of these libraries that it wasn't safe. The point of generated code is that a user shouldn't need to be reading it to figure out implementation subtleties.

It is good that you've made everyone aware of the issue but... at the same time there's no way to remediate the problem. I too am stuck now in trying to upgrade to these libraries, but am hosed because I have thousands of these errors coming up now (mainly in logging statements).

Edit: To make things worse, I have a lot of code that uses require.Equal to compare two message structs in tests. This depends on passing by value, since the pointer values are not equal.

(Edited after I found the support to copy/comparison):

C++ API, Python API and Java API provide CopyFrom() or mergeFrom() methods.

The equivalent methods in Go are proto.Equal and proto.Merge. Both can be passed by reference.

For future reference: You probably mean proto.Clone, rather than Equal.

Question: proto.Clone() creates a new protobuff message from the old one. What If I do not want that? Instead I want to create a new struct that only contains the data fields without any protobuf fields (XXX and suchlike). In that case I want an explicit CopyFrom(), MergeFrom() methods ? proto.Clone() is not suitable in this case...

It might be possible to use proto.Marshal()/Unmarsha() which gets you most of the way there albeit with the protobuf fields still present but nil.?

What If I do not want that? Instead I want to create a new struct that only contains the data fields without any protobuf fields (XXX and suchlike). In that case I want an explicit CopyFrom(), MergeFrom() methods ?

😟 I don’t think something like that is going to be easily accomplished without using protobuf reflection. How could these hypothetical CopyFrom and MergeFrom methods work against an arbitrary type like that without using reflection?

@puellanivis yes - I think this can only be accomplished if the generated code creates these methods as part of the struct definition...

Mmm I found this topic because I was hoping for a better solution to shallow copy protobuf message(s). I'd guess you're right @puellanivis @eccles, in that the only way to accomplish "copy without reflection" would be to implement it manually, or generate code for it.

My use case typically involves patching part of an existing message, without actually modifying the existing value. It's been my experience that treating messages as read-only often the only sane choice, if you're passing protobuf messages around (within a single process).

As a point of comparison, I've added my implementation to the bottom of this comment, which is semantically similar to OP's but uses the reflect package, which appears to perform worse than the OP's solution. I am not sure why I didn't use protobuf reflection. I may have I run into issues, or perhaps I just didn't bother investigating it 😂.

EDIT: I did a little bit of benchmarking, and found the results quite interesting. Note that the manual implementations were effectively 1-1 with what the ProtoCopier func does (see below), consisting of a return statement, returning a new struct literal, with fields populated using their generated Get* methods.

BenchmarkProtoCopier/empty_13fields_shallow_copy-16      	  307670	      3674 ns/op
BenchmarkProtoCopier/empty_13fields_proto_copier-16      	  149216	      7870 ns/op
BenchmarkProtoCopier/empty_13fields_manual_copy-16       	  914919	      1323 ns/op
BenchmarkProtoCopier/13fields_1_shallow_copy-16          	  259792	      4448 ns/op
BenchmarkProtoCopier/13fields_1_proto_copier-16          	  213051	      5664 ns/op
BenchmarkProtoCopier/13fields_1_manual_copy-16           	 1000000	      1185 ns/op
BenchmarkProtoCopier/empty_63fields_shallow_copy-16     	  331166	      3660 ns/op
BenchmarkProtoCopier/empty_63fields_proto_copier-16     	   62065	     19318 ns/op
BenchmarkProtoCopier/empty_63fields_manual_copy-16      	  763878	      1568 ns/op
BenchmarkProtoCopier/63fields_1_shallow_copy-16         	  112834	     10642 ns/op
BenchmarkProtoCopier/63fields_1_proto_copier-16         	   61192	     19478 ns/op
BenchmarkProtoCopier/63fields_1_manual_copy-16          	  937689	      1293 ns/op

// shallow copy methods could also be implemented manually, though that's a bit of a pain
var copySomeMessage = ProtoCopier((*SomeMessage)(nil))
func (x *SomeMessage) Copy() *SomeMessage { return copySomeMessage(x).(*SomeMessage) }

// ProtoCopier returns a function that will shallow copy values of a given protobuf value's type, utilising any `Get`
// prefixed method, accepting no input parameters, and returning a single value of the same type, available for each
// given field, intended to be be used with generated code for protobuf messages
// NOTE a panic will occur if the v's type is not t
func ProtoCopier(v interface{}) func(v interface{}) interface{} {
	var (
		t            = reflect.TypeOf(v)
		fieldMethods = make([][2]int, 0)
	)
	for i := 0; i < t.Elem().NumField(); i++ {
		field := t.Elem().Field(i)
		if field.PkgPath != `` {
			continue
		}
		method, ok := t.MethodByName(`Get` + field.Name)
		if !ok ||
			method.Type.NumIn() != 1 ||
			method.Type.NumOut() != 1 ||
			method.Type.Out(0) != field.Type {
			continue
		}
		fieldMethods = append(fieldMethods, [2]int{i, method.Index})
	}
	return func(v interface{}) interface{} {
		src := reflect.ValueOf(v)
		protoCopierCheckType(t, src.Type())
		dst := reflect.New(t.Elem()).Elem()
		for _, fieldMethod := range fieldMethods {
			dst.Field(fieldMethod[0]).Set(src.Method(fieldMethod[1]).Call(nil)[0])
		}
		return dst.Addr().Interface()
	}
}

func protoCopierCheckType(dst, src reflect.Type) {
	if dst != src {
		panic(fmt.Errorf(`ProtoCopier dst %s != src %s`, dst, src))
	}
}