search

Terry-Mao / goim

goim
https://goim.io/
MIT License
7.21k stars 1.78k forks source link

data race #128

Closed leemingtian closed 8 years ago

leemingtian commented 8 years ago

Hi，毛大。最近在用data race detector测试goim，发现几处warning。我还没来得及研究各个warning会对业务产生什么影响，先发个issue引起大家注意。 贴一个comet里的报警，每个客户端连接会有两个goroutine处理消息，两个goroutine都会用到ch.CliProto，而该Ring结构里提供的方法里并没有加锁，导致data race： image

liutaihua commented 8 years ago

我也来围观下 ：） G1的Ring是在SetAdv之后，才做一次proto signal G2的Ring总是会在收到一个proto ready，才执行一次CliProto.Get

另外它们操作的是2个不同的资源， rp, wp。

应该不存在data race

Terry-Mao commented 8 years ago

不要相信data race，只是指引你，潜在风险，如果知道自己的访问行为就不会了。

Terry-Mao commented 8 years ago

这是比较经典的 Lock-free实现，data race检查不出来的。。。

Terry-Mao commented 8 years ago

https://en.wikipedia.org/wiki/Circular_buffer

leemingtian commented 8 years ago

@liutaihua 图里红圈的两行代码是data race的地方 image @Terry-Mao 我理解的Lock-free的循环buffer是需要用原子操作之类的方式访问读写指针r.rp/r.wp的。图中r.rp++不是原子操作，这样使用循环buffer会有错误吧？

liutaihua commented 8 years ago

严格的算可以算脏读吧 这个场景只是为了复用，对这个不敏感。 频率很低。

Terry-Mao commented 8 years ago

这里的场景非常特殊，是one reader，one writer，原子操作的场景是multi Reader&Writer，

Terry-Mao commented 8 years ago

@leemingtian 如图，即使是GetAdv 和 Set 同时执行也是无风险的，最多返回一个ErrRingFull 因为wp 始终是>= rp 的

leemingtian commented 8 years ago

@Terry-Mao 多人读写需要Lock，判断是否满也需要Lock。不过正如你所说，当前Ring的实现也不会导致严重的业务风险，且Ring也不会有性能瓶颈，较真的同学可以考虑加锁或使用原子操作消除data race报警。 另外贴一处job组件中不同goroutine访问RoomServersMap时的data race报警： image 当一个goroutine遍历RoomServersMap时另一个goroutine对RoomServersMap赋新值是否会有问题？

tonybase commented 8 years ago

RoomServersMap只是聚合各个server上面roomId过来，是直接替换整个map引用，并没有进行修改和删除。所以这里并不会有问题。

leemingtian commented 8 years ago

建议尽量不要无视data race warning。不排除data race有误报的可能，但是保证代码data race clean能帮助我们规避、及时发现很多问题。这个issue就先关闭啦，感谢各位。