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

Client not safe for concurrent use #396

Open
alfa-alex opened this issue Nov 8, 2024 · 0 comments
Open

Client not safe for concurrent use #396

alfa-alex opened this issue Nov 8, 2024 · 0 comments

Comments

@alfa-alex
Copy link

Background

The zitadel-go client in pkg/client/client.go is basically a wrapper that offers access to multiple (generated) gRPC clients. Unlike the individual gRPC clients themselves, this client is not safe for concurrent use, though, as its methods are structured like this:

func (c *Client) SystemService() system.SystemServiceClient {
	if c.systemService == nil {
		c.systemService = system.NewSystemServiceClient(c.connection)
	}
	return c.systemService
}

Since the access to the systemService field is not guarded by a mutex, this code could lead to race conditions.

That's a pity because it's useful to have just a single client instance in your code and to reuse it in different goroutines. That way you only need to create a single gRPC connection (for example, I use this heavily in tests where I instantiate the client once at the beginning of the outer test and use it in all, possibly parallel, subtests - here the Go race detector immediately fires right now).

Possible solutions

Guarding the above call with a mutex is probably too costly in the most common case. I've experimented with an alternative solution that doesn't "lazy-initialize" the individual service clients but initializes them all at once (1085e72). Since the only line of code executed by the New...ServiceClient(c.connection) calls is return &systemServiceClient{cc}, the impact of this shouldn't be too big.

If performance of the constructor is a concern, though, having an option you can provide to the constructor to pre-initialize all service clients could then be an alternative (72724b5).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Status: No status
Development

No branches or pull requests

1 participant