race when using codec with grpc #205

chocnutfriday · 2016-09-30T10:38:21Z

grpc expects marshalers to be thread-safe. This is not true in gogo protobuf currently. In most cases this is best, soI am not suggesting that the default behavior changes. However, it would be nice if an option, such as
"option (gogoproto.concurrent_marshaler_all) = true"
could be used to enforce thread-safety in marshalers.

I have created a small example that demonstrates the issue:
https://gist.github.com/chocnutfriday/a864630a5bcbafd5c15ae634ccc776d3
Run it with "go test -race"

awalterschulze · 2016-09-30T18:56:46Z

On 30 September 2016 at 12:38, chocnutfriday notifications@github.com
wrote:

grpc expects marshalers to be thread-safe. This is not true in gogo
protobuf currently. In most cases this is best, soI am not suggesting that
the default behavior changes. However, it would be nice if an option, such
as
"option (gogoproto.concurrent_marshaler_all) = true"
could be used to enforce thread-safety in marshalers.

What would this generated marshaler look like?

The generated Marshal method is already thread safe.
And the generated MarshalTo method is as thread safe as the byte buffer you
are passing in.

I have created a small example that demonstrates the issue:
https://gist.github.com/chocnutfriday/a864630a5bcbafd5c15ae634ccc776d3
Run it with "go test -race"

Also this test passes for me.
And also passes for me with the -race flag.

—
You are receiving this because you are subscribed to this thread.
Reply to this email directly, view it on GitHub
#205, or mute the thread
https://github.com/notifications/unsubscribe-auth/ABvsLfQ5uURUxcKSlP_g031Keg71Co6iks5qvOaegaJpZM4KK9B6
.

chocnutfriday · 2016-10-03T08:01:51Z

I'm not sure why the race isn't happening for you. Did you just run the generate script and go test? I just checked after "go get"ing the latest gogo/protobuf and grpcs.
Is there a way to specify what type of buffer to use for the marshalTo method? This is where the race happens. By default GRPC uses a byte slice as well as MarshalTo, which isn't safe for the server which handles concurrent requests. For performance reasons it would be nice if the marshaler did not make a new buffer each time, but used a buffer pool or something safe.

chocnutfriday · 2016-10-03T08:36:00Z

Gist now updated with example output from the race detector as well as the generated pbs.

awalterschulze · 2016-10-03T18:43:05Z

I think you need to send me the new gist link.
Also I don't see you using the codec that might also be a problem.

chocnutfriday · 2016-10-04T09:44:15Z

My bad. I forgot to hit the update button or something yesterday. Maybe it will make more sense after you looked at the code that is generated as well as the output.

awalterschulze · 2016-10-05T20:23:30Z

Ok its updated, but I still don't see where you are importing codec?

chocnutfriday · 2016-10-06T06:44:19Z

Well, in the generated pb protobuf/proto is imported. Proto.MarshalTo is called.
I quote from the race detector output:
WARNING: DATA RACE
Write at 0x00c4202781ce by goroutine 80:
concurrentgrpc.(_Payload).MarshalTo()
gopath/concurrentgrpc/data.pb.go:192 +0x6d
github.com/gogo/protobuf/codec.(_codec).Marshal()
gopath/github.com/gogo/protobuf/codec/codec.go:80 +0x15a
google.golang.org/grpc.encode()
gopath/google.golang.org/grpc/rpc_util.go:264 +0x495
google.golang.org/grpc.(_Server).sendResponse()
gopath/google.golang.org/grpc/server.go:513 +0xce
google.golang.org/grpc.(_Server).processUnaryRPC()
gopath/google.golang.org/grpc/server.go:638 +0x11e4
google.golang.org/grpc.(_Server).handleStream()
gopath/google.golang.org/grpc/server.go:770 +0xf9c
google.golang.org/grpc.(_Server).serveStreams.func1.1()
gopath/google.golang.org/grpc/server.go:423 +0xb8

Previous write at 0x00c4202781ce by goroutine 78:
concurrentgrpc.(_Payload).MarshalTo()
gopath/concurrentgrpc/data.pb.go:192 +0x6d
github.com/gogo/protobuf/codec.(_codec).Marshal()
gopath/github.com/gogo/protobuf/codec/codec.go:80 +0x15a
google.golang.org/grpc.encode()
gopath/google.golang.org/grpc/rpc_util.go:264 +0x495
google.golang.org/grpc.(_Server).sendResponse()
gopath/google.golang.org/grpc/server.go:513 +0xce
google.golang.org/grpc.(_Server).processUnaryRPC()
gopath/google.golang.org/grpc/server.go:638 +0x11e4
google.golang.org/grpc.(_Server).handleStream()
gopath/google.golang.org/grpc/server.go:770 +0xf9c
google.golang.org/grpc.(_Server).serveStreams.func1.1()
gopath/google.golang.org/grpc/server.go:423 +0xb8

awalterschulze · 2016-10-06T08:36:16Z

But can you explain to me how grpc/rpc_util is able to call gogo/protobuf/codec without you passing it to the grpc server?

chocnutfriday · 2016-10-06T11:54:15Z

From the grpc source:
// protoCodec is a Codec implementation with protobuf. It is the default codec for gRPC.
type protoCodec struct{}

func (protoCodec) Marshal(v interface{}) ([]byte, error) {
return proto.Marshal(v.(proto.Message))
}

awalterschulze · 2016-10-06T12:05:36Z

Ok and now. How does it get to the github.com/gogo/protobuf/codec ?

On 6 October 2016 at 13:54, chocnutfriday notifications@github.com wrote:

From the grpc source:
// protoCodec is a Codec implementation with protobuf. It is the default
codec for gRPC.
type protoCodec struct{}

func (protoCodec) Marshal(v interface{}) ([]byte, error) {
return proto.Marshal(v.(proto.Message))
}

—
You are receiving this because you commented.
Reply to this email directly, view it on GitHub
#205 (comment), or mute
the thread
https://github.com/notifications/unsubscribe-auth/ABvsLQS4WYc_MvU4LllZV5OPNg5Pw7aPks5qxOFngaJpZM4KK9B6
.

chocnutfriday · 2016-10-06T15:05:58Z

gogo protobuf satisfies the grpc codec interface and it is used by default.
From the grpc docs:
type Codec interface {
// Marshal returns the wire format of v.
Marshal(v interface{}) ([]byte, error)
// Unmarshal parses the wire format into v.
Unmarshal(data []byte, v interface{}) error
// String returns the name of the Codec implementation. The returned
// string will be used as part of content type in transmission.
String() string
}
Supplying a custom codec to grpc that uses a buffer pool made the race go away.

awalterschulze · 2016-10-06T15:10:40Z

How is it used by default? where?
It probably needs to be injected somewhere, no?

chocnutfriday · 2016-10-06T15:25:39Z

Yes, it is injected somewhere. Magic of auto-generated code. It shows up on the race-detector in most of my grpc projects. The fact of the matter is that when something injects is into something that expects a thread-safe buffer it's not going to happen, causing a race.

awalterschulze · 2016-10-06T16:02:24Z

generated code can be read. Code is not magic. Where is it begin injected?

chocnutfriday · 2016-10-07T06:55:53Z

Yes, you are completely right. I couldn't find where the use of codec was generated, because it isn't generated. It is done in the grpc server itself. Which makes me wonder how the race isn't a problem for more people.

in
gopath/google.golang.org/grpc/server.go
we have:
"github.com/gogo/protobuf/codec"
being imported

at line 196:
if opts.codec == nil {
// Set the default codec.
opts.codec = codec.New(0)
}

Well I'm surprised.

awalterschulze · 2016-10-09T11:58:26Z

Well done, see no magic :)
I "guess" "someone" must have edited your git subtree to only import github.com/gogo/protobuf and removed all imports of github.com/golang/protobuf. This was "probably" done because "you" didn't want to add the extra unneeded dependency and possibly also to avoid confusion about which protobuf library to use.
Ek sou vir jou vroeer hiervan vertel het as ek onthou het, jammer.

Now you will need to edit your gist to always use the github.com/gogo/protobuf/codec . Please see the grpc-go Server and Client Options. Their should be some option called WithCodec or something like that.

chocnutfriday · 2016-10-10T08:30:15Z

The gist has been updated to use the protobuf codec as "custom codec".

awalterschulze · 2016-10-10T08:40:43Z

Looks like it has only been updated on the server and not the client. But might be a good idea to see if that alone reproduces the problem. I'll have a look as soon as I can, but I am quite busy at the moment with moving. So expect some delays.

chocnutfriday · 2016-10-10T08:52:45Z

Yes, the output from the race detector shows that the race happens for the server. I think it is because new requests can arrive while other requests are being processed. The client code in this case is quite thread-safe, so there's no need for a thread-safe codec.

Thank you.

awalterschulze · 2016-10-10T09:23:00Z

Thats also good to know.
Thank you :)

awalterschulze · 2016-10-18T15:38:47Z

Its really weird, but when I clone your gist I can really weird file names.

concurrentgrpc\data.pb.go
concurrentgrpc\data.proto
concurrentgrpc\exampleOutput
concurrentgrpc\genpb.sh

Maybe its easier if you remove the folder.

Also please change the proto_path to just assume that you are in ./src/somefolder and that gogoprotobuf and all deps are already cloned.

That way someone can just check it out and start fiddling.

awalterschulze · 2016-11-06T14:35:21Z

hello?

chocnutfriday · 2016-11-16T08:09:43Z

Sorry for not being active on this. I have stopped working on this issue for now. The default PB marshaler is good enough for its purpose and this is what GRPC uses by default. I could not come up with a scheme for a codec that is efficient, fast and thread-safe. Though the current proto/codec is not safe for concurrent use, it is efficient, fast and good for reuse. If you can think of a way to get similar performance in a thread-safe way let me know. I tried using sync.Pool, but gave up on that since I had to choose between doing a copy or risk a race when returning the buffer coupled with a defered insert into the pool.

awalterschulze · 2016-11-16T08:57:00Z

All that work can be very useful in making an argument to grpc-go.
If you tidy up your gists and post the alternative gist that shows the slow down with sync.Pool that will be amazing.
Since the only way to make the codec thread safe and as fast as it is now, is to get buy in from grpc-go.
Are you maybe willing to do this?

chocnutfriday · 2016-11-16T11:08:36Z

I don't mind doing this. I just want to get clarity about what a good solution would be.

Parallel encoding is currently done by sharing a single codec. My thoughts are that, for go, it would be better to have a pool of codec objects that never gets accessed concurrently. Given the cost of assuring synchronisation having a single shared codec isn't worth it currently. At least, this is true for go, but not for all languages. C++ has arena to get around the safety issue, for example. In other words, this default behavior of sharing a codec isn't always bad and the implications of changing this assumption extends beyond go.

So before I raise the issue let me know what you would suggest as I'm at a loss.
To post benchmarks for a codec that is not supported by them will probably be ignored. Asking for the behavior of GRPC to change is probably not going to be accepted due to other languages and there being a working codec all be it slower. So the only real alternative is to create a thread-safe codec that is faster or settle for using the default pb marshaler.

awalterschulze · 2016-11-20T12:03:15Z

We are not challenging grpc just the grpc-go library.
So even though change is unlikely we should still at least try.
Since if they decide to have a pool of codec objects (and they support custom implementations of codecs, its there interface that exposes WithCodec) it will be the fastest.
Only if this comes down to a definite no can we give up on this solution.

I don't mind raising the issue with them, if you don't want to, but I need data and code to support this and as you know this takes time to make.

We need three things:

One that exposes the race condition. This you have, but you need to tidy up. It needs to be as simple as checking it out in the right place and running some make option. With no absolute paths. It should also be easy to modify and rerun. Just like any normal github project, just smaller.
Next we need an implementation that tries to use sync.Pool inside the codec, like you have said you have already tried.
Finally we need benchmark numbers for these two implemenations.

Then in the issue (you or I) can link to the gist(s) that you made you simply argue the point that asks them to implement codec pooling.

chocnutfriday · 2016-11-21T07:15:39Z

Ok, I'll work on this. I'll let you know when I've updated the gist.

steeve · 2017-01-23T22:43:31Z

Jumping on the bandwagon here.

gRPC expect the Codec object to be long lived, as such, the current implementation at https://github.com/gogo/protobuf/blob/master/codec/codec.go is not safe since it keeps the resulting []byte as state.

As for using a pool, I did some benchmark using a sync.Pool of []byte and using MarshalTo.

Here are the results, first one is simple proto.Marshal, second is using a pool:

BenchmarkProtoBuffer-4    	10000000	       268 ns/op	      96 B/op	       4 allocs/op
BenchmarkProtoBufferPool-4   	 5000000	       275 ns/op	      64 B/op	       3 allocs/op

The performance advantage of the pool, in our case, is lost ~~doing the type assertion~~ and calling Size().

type marshalerTo interface {
	Size() int
	MarshalTo(data []byte) (n int, err error)
}

In the end, we are using this very simple codec:

type GogoCodec struct {
	grpc.Codec
}

func (g *GogoCodec) Marshal(v interface{}) ([]byte, error) {
	return proto.Marshal(v.(proto.Message))
}

func (g *GogoCodec) Unmarshal(data []byte, v interface{}) error {
	return proto.Unmarshal(data, v.(proto.Message))
}

func (g *GogoCodec) String() string {
	return "proto"
}

awalterschulze · 2017-01-24T08:04:15Z

Thank you for the data. Can you also provide the protobuf you used, since it looks like it could be a small one?

steeve · 2017-01-24T12:43:58Z

Sure, here goes (proto3), compiled with marshaler and sizer:

message Sample {
    string str1 = 1;
    string str2 = 2;
    string str3 = 3;
    repeated string str_array1 = 4;
    repeated string str_array2 = 5;
}

steeve · 2017-01-24T12:44:25Z

All the fields were set, and the arrays to one item only

awalterschulze · 2017-01-24T13:17:25Z

Ok yes as suspected this is a very small proto message. Good to know. Thanks alot

…

On Tue, 24 Jan 2017, 13:44 Steeve Morin, ***@***.***> wrote: All the fields were set, and the arrays to one item only — You are receiving this because you commented. Reply to this email directly, view it on GitHub <#205 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/ABvsLaPRLsI7vUqZ26oAa1Ad-dy5Xj8Kks5rVfIpgaJpZM4KK9B6> .

steeve · 2019-01-25T08:47:51Z

Going back on this, is there a reason for the gogo grpc codec to keep its bytes as state, thus suffering from a data race when concurrent calls to Marshal are done ?

awalterschulze changed the title ~~Data Race when using grpc~~ race when using codec with grpc Sep 30, 2016

awalterschulze added the question label Oct 1, 2016

awalterschulze mentioned this issue Dec 11, 2016

NULL deserialization of Oneof when using GRPC #238

Closed

xiang90 mentioned this issue Dec 14, 2016

Watch: inefficient event distribution etcd-io/etcd#4627

Closed

awalterschulze added the depends-on-grpc-go label Nov 3, 2017

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

race when using codec with grpc #205

race when using codec with grpc #205

chocnutfriday commented Sep 30, 2016

awalterschulze commented Sep 30, 2016

chocnutfriday commented Oct 3, 2016

chocnutfriday commented Oct 3, 2016

awalterschulze commented Oct 3, 2016

chocnutfriday commented Oct 4, 2016

awalterschulze commented Oct 5, 2016

chocnutfriday commented Oct 6, 2016

awalterschulze commented Oct 6, 2016

chocnutfriday commented Oct 6, 2016

awalterschulze commented Oct 6, 2016

chocnutfriday commented Oct 6, 2016

awalterschulze commented Oct 6, 2016

chocnutfriday commented Oct 6, 2016

awalterschulze commented Oct 6, 2016

chocnutfriday commented Oct 7, 2016

awalterschulze commented Oct 9, 2016

chocnutfriday commented Oct 10, 2016

awalterschulze commented Oct 10, 2016

chocnutfriday commented Oct 10, 2016

awalterschulze commented Oct 10, 2016

awalterschulze commented Oct 18, 2016

awalterschulze commented Nov 6, 2016

chocnutfriday commented Nov 16, 2016

awalterschulze commented Nov 16, 2016

chocnutfriday commented Nov 16, 2016

awalterschulze commented Nov 20, 2016

chocnutfriday commented Nov 21, 2016

steeve commented Jan 23, 2017 •

edited

Loading

awalterschulze commented Jan 24, 2017

steeve commented Jan 24, 2017 •

edited

Loading

steeve commented Jan 24, 2017

awalterschulze commented Jan 24, 2017 via email

steeve commented Jan 25, 2019

race when using codec with grpc #205

race when using codec with grpc #205

Comments

chocnutfriday commented Sep 30, 2016

awalterschulze commented Sep 30, 2016

chocnutfriday commented Oct 3, 2016

chocnutfriday commented Oct 3, 2016

awalterschulze commented Oct 3, 2016

chocnutfriday commented Oct 4, 2016

awalterschulze commented Oct 5, 2016

chocnutfriday commented Oct 6, 2016

awalterschulze commented Oct 6, 2016

chocnutfriday commented Oct 6, 2016

awalterschulze commented Oct 6, 2016

chocnutfriday commented Oct 6, 2016

awalterschulze commented Oct 6, 2016

chocnutfriday commented Oct 6, 2016

awalterschulze commented Oct 6, 2016

chocnutfriday commented Oct 7, 2016

awalterschulze commented Oct 9, 2016

chocnutfriday commented Oct 10, 2016

awalterschulze commented Oct 10, 2016

chocnutfriday commented Oct 10, 2016

awalterschulze commented Oct 10, 2016

awalterschulze commented Oct 18, 2016

awalterschulze commented Nov 6, 2016

chocnutfriday commented Nov 16, 2016

awalterschulze commented Nov 16, 2016

chocnutfriday commented Nov 16, 2016

awalterschulze commented Nov 20, 2016

chocnutfriday commented Nov 21, 2016

steeve commented Jan 23, 2017 • edited Loading

awalterschulze commented Jan 24, 2017

steeve commented Jan 24, 2017 • edited Loading

steeve commented Jan 24, 2017

awalterschulze commented Jan 24, 2017 via email

steeve commented Jan 25, 2019

steeve commented Jan 23, 2017 •

edited

Loading

steeve commented Jan 24, 2017 •

edited

Loading