Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add section on concurrency to documentation #1345

Closed
timbunce opened this issue Jun 30, 2017 · 9 comments

Comments

Projects
None yet
8 participants
@timbunce
Copy link

commented Jun 30, 2017

What did you do?

I've looked for documentation to clarify what client behaviour is safe for concurrent goroutines using grpc-go.

What did you expect to see?

A section of the documentation labeled "Concurrency" with answers to questions like:

  • Can concurrent goroutines share use of a single connection object? (It seems yes.)
    • If not, then how best to manage/pool them. Perhaps like this?
  • Can concurrent goroutines share use of a single client object?
    • If not, then how best to manage/pool them.
  • Can concurrent goroutines share use of a single stream object? (It seems not.)
  • How is concurrency handled on the server?
  • What are some best-practices and traps around concurrency in grpc-go?

What did you see instead?

Hardly any mention of concurrency, thread safety, pooling etc.

This slide (in a presentation by Sameer Ajmani) implies that client objects can be reused because there's no locking or use of pools yet the Search() function can be called concurrently when handling concurrent requests. But perhaps that code was simplified for the presentation.

See also issues #85, #682, #730.

@britt

This comment has been minimized.

Copy link

commented Jul 13, 2017

I would add a few questions related to how grpc.ClientConn manages HTTP2 connections for concurrent RPC calls.

  • When does a connection form an HTTP2 connection to the server?
  • Does is form a connection for each concurrent RPC? (seems like yes)
  • Does it pool connections? (seems like no)
@timbunce

This comment has been minimized.

Copy link
Author

commented Jul 16, 2017

I was just at GopherCon '17 and saw Alan Shreve's "grpc: From Tutorial to Production" talk along with a few hundred other developers. It was a very good talk with a nice structure and flow that covered the topic well.

Except for one thing... Here's a version of his slides. Notice that the server-side cache as no concurrency protection:

type CacheService struct {
    store map[string][]byte
}
func (s *CacheService) Get(ctx context.Context, req *rpc.GetReq) (*rpc.GetResp,
    error) {
    val := s.store[req.Key]
    return &rpc.GetResp{Val: val}, nil
}
func (s *CacheService) Store(ctx context.Context, req *rpc.StoreReq) (*rpc.StoreResp, error) {
    s.store[req.Key] = req.Val
    return &rpc.StoreResp{}, nil
}

There was no mention of concurrency safety in the talk. (There wasn't time for questions at the end so I couldn't query that at the time.)

Am I right in thinking that that code is unsafe, or am I missing something?

/cc @inconshreveable

@inconshreveable

This comment has been minimized.

Copy link

commented Jul 17, 2017

@timbunce You're not missing anything. I mentioned during the talk that the code was not safe for concurrent usage and that any real implementation would need synchronization. Concurrency safety was omitted from the slides/talk in the interest of 1) focusing on the code related to grpc-specific issues 2) keeping the code on the slides big enough to be readable

@dfawley dfawley self-assigned this Aug 24, 2017

@cloudzhou

This comment has been minimized.

Copy link

commented Aug 25, 2017

yes, need more documentation on concurrency.
take a example:

cli, cerr := clientv3.NewFromURL("http://localhost:2379")
r := &etcdnaming.GRPCResolver{Client: cli}
b := grpc.RoundRobin(r)
conn, gerr := grpc.Dial("my-service", grpc.WithBalancer(b))

1 does grpc.RoundRobin balancer concurrency safe ?
2 what if we implement Resolver, does it required concurrency safe ?
3 does conn concurrency safe ?

I think the easy way is:
as default, all method, struct is unsafe for concurrency. mark it down if is safe.

@ostlerc

This comment has been minimized.

Copy link

commented Nov 9, 2017

can someone answer these questions? I came here looking for the answer to 'Can concurrent goroutines share use of a single client object?' but so far nobody knows...

@dfawley

This comment has been minimized.

Copy link
Contributor

commented Nov 9, 2017

Can concurrent goroutines share use of a single client object?

Short answer: yes.

The only part of grpc-go that isn't concurrency-friendly is calling Recv (or Send) multiple times concurrently on a single stream. (Recv and Send may be called concurrently with each other.)

@enocom

This comment has been minimized.

Copy link
Contributor

commented Apr 18, 2018

@dfawley Would an addition Documentation help close this issue? I would be happy to provide a summary of all the above discussion and that of linked issues.

@menghanl

This comment has been minimized.

Copy link
Contributor

commented Apr 18, 2018

@enocom That will be great! Thanks a lot!
What was not mentioned above but could still be helpful: https://godoc.org/google.golang.org/grpc#Stream.

@enocom

This comment has been minimized.

Copy link
Contributor

commented Apr 18, 2018

@menghanl Sounds good. I'll put a document together.

@lock lock bot locked as resolved and limited conversation to collaborators Nov 6, 2018

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
You can’t perform that action at this time.