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

getCachedClient 存在并发bug #772

Closed
mini-bix opened this issue Dec 23, 2022 · 6 comments
Closed

getCachedClient 存在并发bug #772

mini-bix opened this issue Dec 23, 2022 · 6 comments

Comments

@mini-bix
Copy link

rpcx/client/xclient.go

Lines 282 to 338 in 6d27bc7

func (c *xClient) getCachedClient(k string, servicePath, serviceMethod string, args interface{}) (RPCClient, error) {
// TODO: improve the lock
var client RPCClient
var needCallPlugin bool
defer func() {
if needCallPlugin {
c.Plugins.DoClientConnected(client.GetConn())
}
}()
if c.isShutdown {
return nil, errors.New("this xclient is closed")
}
// if this client is broken
breaker, ok := c.breakers.Load(k)
if ok && !breaker.(Breaker).Ready() {
return nil, ErrBreakerOpen
}
c.mu.Lock()
client = c.findCachedClient(k, servicePath, serviceMethod)
if client != nil {
if !client.IsClosing() && !client.IsShutdown() {
c.mu.Unlock()
return client, nil
}
c.deleteCachedClient(client, k, servicePath, serviceMethod)
}
client = c.findCachedClient(k, servicePath, serviceMethod)
c.mu.Unlock()
if client == nil || client.IsShutdown() {
generatedClient, err, _ := c.slGroup.Do(k, func() (interface{}, error) {
return c.generateClient(k, servicePath, serviceMethod)
})
c.slGroup.Forget(k)
if err != nil {
return nil, err
}
client = generatedClient.(RPCClient)
if c.Plugins != nil {
needCallPlugin = true
}
client.RegisterServerMessageChan(c.serverMessageChan)
c.mu.Lock()
c.setCachedClient(client, k, servicePath, serviceMethod)
c.mu.Unlock()
}
return client, nil
}

上面这段代码存在并发问题,锁的使用存在问题,可能多个线程都拿到了nil的client, c.slGroup.Do似乎只能保证同时创建一个连接,但是c.slGroup.Forget之后可能创建出多个连接,导致连接泄露,创建一些不被使用的连接

@smallnest
Copy link
Owner

fixed. only forget k when the client is cached.

@supermario1990
Copy link
Contributor

fixed. only forget k when the client is cached.

我觉得改了之后还是有点问题,当 c.slGroup.Do() 中调用的函数返回后,还没缓存client前,这里仍然会创建一些不被使用的链接。 这里应该直接使用mutex来控制并发就行了

@smallnest
Copy link
Owner

fixed. only forget k when the client is cached.

我觉得改了之后还是有点问题,当 c.slGroup.Do() 中调用的函数返回后,还没缓存client前,这里仍然会创建一些不被使用的链接。 这里应该直接使用mutex来控制并发就行了

这个应该修复了。
或者使用同一个singleflight key的结果,或者从cache中获取

@lk-j
Copy link

lk-j commented Mar 18, 2024

还是存在问题,当网络延时的时候 c.slGroup.Do(k, func() (interface{}, error) {
return c.generateClient(k, servicePath, serviceMethod)
}) k不一样,还是会生成大量连接
直接使用mutex来控制并发更合理

@smallnest
Copy link
Owner

还是存在问题,当网络延时的时候 c.slGroup.Do(k, func() (interface{}, error) { return c.generateClient(k, servicePath, serviceMethod) }) k不一样,还是会生成大量连接 直接使用mutex来控制并发更合理

你的rpcx的版本是?

@smallnest smallnest reopened this Mar 18, 2024
@lk-j
Copy link

lk-j commented Apr 28, 2024

还是存在问题,当网络延时的时候 c.slGroup.Do(k, func() (interface{}, error) { return c.generateClient(k, servicePath, serviceMethod) }) k不一样,还是会生成大量连接 直接使用mutex来控制并发更合理

你的rpcx的版本是?

1.7.15

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

No branches or pull requests

4 participants