Skip to content

fix(registry/consul): fix concurrency issues and improve performance #3511

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 3 commits into from
Jan 24, 2025
Merged

fix(registry/consul): fix concurrency issues and improve performance #3511

merged 3 commits into from
Jan 24, 2025

Conversation

lftk
Copy link
Contributor

@lftk lftk commented Jan 2, 2025

修复了一些并发相关的问题：

  1. Registry 锁的范围太大，比如 Watch 对整个函数加锁，不能同时处理多个（不同的）Service；

kratos/contrib/registry/consul/registry.go

Lines 183 to 185 in 5087366

func (r *Registry) Watch(ctx context.Context, name string) (registry.Watcher, error) {
r.lock.Lock()
defer r.lock.Unlock()

  1. 相同的 Service 没有共享 resolve，只要 Watch 就会新开一个 resolve 协程；

kratos/contrib/registry/consul/registry.go

Lines 211 to 213 in 5087366

if err := r.resolve(ctx, set); err != nil {
return nil, err
}

  1. Client 注销任一 Service 都会直接 cancel 掉 Client.ctx，导致所有的 heartbeat 协程退出；

kratos/contrib/registry/consul/client.go

Lines 246 to 249 in 5087366

func (c *Client) Deregister(_ context.Context, serviceID string) error {
defer c.cancel()
return c.cli.Agent().ServiceDeregister(serviceID)
}

  1. 在 Watch 时，由于 w 先被添加到 set 中，然后才会写入 services event，如果在两步中间正好遇到之前的 resolve 在 broadcast，209行的写入则会被卡住（w 还没有返回，不会有调用者主动调用 Next 消费）。

kratos/contrib/registry/consul/registry.go

Lines 196 to 214 in 5087366

// init watcher
w := &watcher{
event: make(chan struct{}, 1),
}
w.ctx, w.cancel = context.WithCancel(ctx)
w.set = set
set.lock.Lock()
set.watcher[w] = struct{}{}
set.lock.Unlock()
ss, _ := set.services.Load().([]*registry.ServiceInstance)
if len(ss) > 0 {
// If the service has a value, it needs to be pushed to the watcher,
// otherwise the initial data may be blocked forever during the watch.
w.event <- struct{}{}
}
if err := r.resolve(ctx, set); err != nil {
return nil, err
}
return w, nil

  1. 其他，提前判断 ctx.Err() 避免做冗长的无用功、sleepCtx 等。

@dosubot dosubot bot added the size:XL This PR changes 500-999 lines, ignoring generated files. label Jan 2, 2025
shenqidebaozi
shenqidebaozi reviewed Jan 22, 2025
View reviewed changes
@dosubot dosubot bot added the LGTM label Jan 24, 2025
@shenqidebaozi shenqidebaozi enabled auto-merge (squash) January 24, 2025 02:58
@shenqidebaozi shenqidebaozi merged commit ff30a75 into go-kratos:main Jan 24, 2025
33 checks passed
@shenqidebaozi shenqidebaozi mentioned this pull request Mar 7, 2025
Merged
chaosue pushed a commit to chaosue/kratos that referenced this pull request Mar 12, 2025
@lftk
fix(registry/consul): fix concurrency issues and improve performance (g…
45b705e 
…o-kratos#3511)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@shenqidebaozi shenqidebaozi shenqidebaozi approved these changes

@songzhibin97 songzhibin97 songzhibin97 approved these changes

Assignees
No one assigned
Labels
LGTM size:XL This PR changes 500-999 lines, ignoring generated files.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@lftk @shenqidebaozi @songzhibin97