Category: Programming

Mirages of data ownership in Go

Rust has a well-known borrow checker and a whole programming model that ensures there will be no data races. In particular, it is only possible to have one mutable reference or many read-only references but not both types at the same time. Technically, you might think that because Go is pass-by-value i.e. the arguments that you’re giving to a function are copied into some memory location before calling the function, it is impossible to have races in Go too. However, some types like slices and maps are implemented as references so you have to take a lot of care when returning them from a function because the caller might modify them.

This is especially important when using RPC frameworks like gRPC.

I use quite a bit of grpc-go at my work and for Thanos as well, and I recently ran into this “fun” quirk. Take a look at the following service and the corresponding server’s code:

syntax = "proto3";

package helloworld;

service Greeter {
  rpc SayHello (HelloRequest) returns (HelloReply) {}
}

message HelloRequest {
  repeated string names = 1;
}

// The response message containing the greetings for each given name.
message HelloReply {
  map<string, string> replies = 1;
}

And the example code:

type server struct {
	pb.UnimplementedGreeterServer

	replies     map[string]string
	repliesLock sync.Mutex
}

// SayHello implements helloworld.GreeterServer
func (s *server) SayHello(_ context.Context, in *pb.HelloRequest) (*pb.HelloReply, error) {
	log.Printf("Received: %v", in.GetName())
	s.repliesLock.Lock()
	defer s.repliesLock.Unlock()

	return &pb.HelloReply{
		Replies: s.replies,
	}, nil
}

Imagine that the map is being updated concurrently by another goroutine with code like this:

	for i := 0; i < 100; i++ {
		go func() {
			for {
				srv.repliesLock.Lock()
				for k := range srv.replies {
					srv.replies[k] = genRandomString(10)
				}
				srv.repliesLock.Unlock()
			}
		}()
	}

It’s all protected with a mutex so there shouldn’t be any problems, right? … right? No. grpc-go actually “takes ownership” (that’s technically not even possible in Go) of the returned value by sending the returned value (message type) to the client at some unspecified future time. So, the programmer’s function that is called by gRPC must always allocate memory. Otherwise, you get a fun race condition like this:

WARNING: DATA RACE
Write at 0x00c0001d9ec0 by goroutine 78:
  runtime.mapassign_faststr()
      /usr/local/go/src/runtime/map_faststr.go:223 +0x0
  main.main.func1()
      /home/giedriusstatkevicius/dev/grpc-go/examples/helloworld/greeter_server/main.go:89 +0x146

Previous read at 0x00c0001d9ec0 by goroutine 128:
  runtime.mapiternext()
      /usr/local/go/src/runtime/map.go:937 +0x0
  reflect.mapiternext()
      /usr/local/go/src/runtime/map.go:1537 +0x12
  google.golang.org/protobuf/internal/impl.appendMap()
      /home/giedriusstatkevicius/go/pkg/mod/google.golang.org/[email protected]/internal/impl/codec_map.go:274 +0x2eb
  google.golang.org/protobuf/internal/impl.encoderFuncsForMap.func2()
      /home/giedriusstatkevicius/go/pkg/mod/google.golang.org/[email protected]/internal/impl/codec_map.go:56 +0xc4
  google.golang.org/protobuf/internal/impl.(*MessageInfo).marshalAppendPointer()
      /home/giedriusstatkevicius/go/pkg/mod/google.golang.org/[email protected]/internal/impl/encode.go:139 +0x4ac
  google.golang.org/protobuf/internal/impl.(*MessageInfo).marshal()
      /home/giedriusstatkevicius/go/pkg/mod/google.golang.org/[email protected]/internal/impl/encode.go:107 +0xd0
  google.golang.org/protobuf/internal/impl.(*MessageInfo).marshal-fm()
      <autogenerated>:1 +0xc4
  google.golang.org/protobuf/proto.MarshalOptions.marshal()

It’s such a hidden foot gun that even the official grpc-go example almost contains a bug. Here is what the code looks like:

// ListFeatures lists all features contained within the given bounding Rectangle.
func (s *routeGuideServer) ListFeatures(rect *pb.Rectangle, stream pb.RouteGuide_ListFeaturesServer) error {
	for _, feature := range s.savedFeatures {
		if inRange(feature.Location, rect) {
			if err := stream.Send(feature); err != nil {
				return err
			}
		}
	}
	return nil
}

Since feature ends up on the wire at some unspecified time in the future and because feature is a variable that gets reused between iterations (until Go 1.22), means there is a race. The only saving grace is that s.savedFeatures is read-only i.e. it is set once and never modified.

It’s even more fun if you are using data where []byte is mmaped. At least my understanding is that Prometheus is free to munmap once Select() is over so Thanos needs to make a copy of each []byte. Here it is happening in Receive: https://github.com/thanos-io/thanos/blob/03c96d05a02425421e0d0b80814c2cd9c765b371/pkg/store/tsdb.go#L334-L335 (https://github.com/thanos-io/thanos/pull/6203).

So, overall, even if a programming language passes everything by value, if the value is a reference, unexpected races can occur. I hope grpc-go will improve the interface in the future. The recent pooling changes is a good step.

Related Posts

My gRPC Annoyances

We have been using gRPC in Thanos since Thanos inception – it has served us great and it has a ton of useful functionality, it solves a lot of problems, it is easy to use, and so on. However, I feel like some stuff is lacking, especially performance that will most likely never be fixed (or, I should say, changed). The framework just does not solve 100% of the things that the Thanos project needs right now. Let’s go through the list of my pet peeves.

Sophisticated compression

A huge part of network traffic between servers in Thanos is used for sending sets of labels, essentially a map<string, string>. Typically, each map only differs by two values between consecutive maps.

Also, we really need streaming to avoid having to buffer the whole response in memory beforehand. Strictly speaking, streaming might not be required because we buffer everything on the querier side either way right now, but there needs to be a way of building the message incrementally.

However, because gRPC is message-based, it means that it is impossible to have a compression scheme that encompasses multiple messages. What I would like to have is some string table that is shared over the whole stream. This also might be in part because gRPC uses HTTP as its transport and there it is impossible to negotiate compression parameters “out of band”. While researching this topic, I stumbled upon https://github.com/WICG/compression-dictionary-transport. Perhaps this will get through at some point and gRPC will be able to leverage this work.

Compression helps a little bit here but not a lot. The gRPC codec interface has []byte as an input, meaning that the input needs to be a contiguous array of bytes in memory. Generally speaking with compression, repeated sequences of bytes have references to them so it would be possible to avoid allocating memory for those repeated sequences if the protobuf unmarshaling interface didn’t need a []byte and would instead be a io.Reader. https://github.com/grpc/grpc-go/issues/499 misses a huge point – the unmarshaling, I believe, still could accept an io.Reader instead of []byte. The point about Marshal() is valid, though.

Fortunately, there is some recent movement to fix this: https://github.com/grpc/grpc-go/issues/6619.

Mirages of ownership

gRPC-Go presents an interesting conundrum to its users – when the user-written code returns from a function that serves a remote procedure call, gRPC-Go takes that variable and marshals it at some point in the future. This is done for performance purposes but it also means that the user is giving away the ownership of the data and that the data must always be not changed after returning. This could become a foot gun in case slices or maps are used because the value of variables of such types are references, making it easy to mutate them accidentally.

Here’s how the “hello world” example looks in grpc-go:

// SayHello implements helloworld.GreeterServer
func (s *server) SayHello(ctx context.Context, in *pb.HelloRequest) (*pb.HelloReply, error) {
	log.Printf("Received: %v", in.GetName())
	return &pb.HelloReply{Message: "Hello " + in.GetName()}, nil
}

In this case, grpc-go takes ownership of &pb.HelloReply{Message: "Hello " + in.GetName()}. This obviously presents no problems but what if there were some slice or map?

type server struct {
  names []string
  ...
}

// Constantly running in the background.
func (s *server) updateNames() {
  for {
    for i := 0; i < len(s.names); i++ {
      s.names[i] = generateRandomName()
    }
    time.Sleep(1*time.Second)
  }
}

// SayHello implements helloworld.GreeterServer
func (s *server) SayHello(ctx context.Context, srv pb.HelloServer) (error) {
        return srv.SendMsg(&pb.HelloReply{Names: s.names})
}

Now this is a bit bad because grpc-go might be trying to marshal s.names while it is being updated in the background. You can find some more context here.

Of course, this is a contrived example and you might think how this could even happen in practice. I had a botched attempt at adding pooling to the marshaling side in Thanos: https://github.com/thanos-io/thanos/pull/4609/files. Fortunately, SendMsg now includes a helpful text:

	// It is not safe to modify the message after calling SendMsg. Tracing
	// libraries and stats handlers may use the message lazily.

Hopefully, all of these problems will be fixed at some point. I am confident that the Codec interface will soon change for the better but the compression stuff will take a longer time. It would allow us to really reduce the memory and CPU usage of Thanos.

Perhaps we will also look at other RPC frameworks in the future. dRPC is a recent project that piqued my interest. I have only dabbled with it for a few hours so I don’t have any opinion on it so far. That’s something for future posts!

Related Posts