smallnest
commented
3 years ago 谢 @changkun 大佬指出问题
Closed changkun closed 3 years ago
smallnest
commented
3 years ago 谢 @changkun 大佬指出问题
smallnest
commented
3 years ago fixed
changkun
commented
3 years ago I think the fix may remain problematic. If BufLen call happens concurrently with line 67, and they both happens-before line 68. Assume a global order, there might be two different cases:
BufLen -> buffer.Write -> ch.bufCount++, the BufLen works as expectedbuffer.Write -> a call to BufLen -> ch.bufCount++, the BufLen returns a number that is less than the actual length because of buffer.Write has changed the buffer.From a memory order perspective, BufLen should observe the length as soon as buffer.Write is complete. The above experiment describes a failure case.
smallnest
commented
3 years ago You are right. Original intention of adding BufLen and Len method is only for evaluating approximate number of elements in this chan, and not for exact count. There is not only the issue you said but also may be some elements are put into In and some elements are read from Out, the Len is not accurate.
That means the number of the real elements is less or more than the value returned by Len and BufLen. I hope users don't check based on the method and only for evaluating approximate memory usage.
Maybe we can have a. method to implement a accurate Len and BufLen but I think it won't be complex. I'd rather keep it a simple implementation for readability and performance.
Anyway, I think I'd better add some comments for Len and BufLen to avoid misuse.
q191201771
commented
3 years ago 我进来学习的，两位大佬真棒。 我赞成smallnest基于可读、性能方面的考虑，以及增加注释的做法。
即使外层接口获取到的是绝对精准值，也是个瞬时值。没有想到有啥场景会造成大的影响。 个人感觉只要不因为误差导致出现负数即可（现在写操作都在一个协程里，应该不存在这个问题）。
现在可能出现的误差只是+1或-1吗？
changkun
commented
3 years ago I share a consensus of readability and performance concerns. This is precisely the same reason why sync.Map does not have a Len method because Len will never be accurate in a concurrent environment. See golang/go#20680.
Nevertheless, in terms of the semantics of Len, it is only about the rigorous definition of memory orders. Otherwise, it may cause misuse and nasty bugs from the user's perspective. Consider the following example:
ch.In <- 42
ch.In <- 42
ch.In <- 42
// Now the channel has three items.
// We create two loops that produce and consume elements from the channel.
go func() { for { ch.In <- 42 } }()
go func() { for { <-ch.Out } }()
// Also, a third party may loop and listen to the status of that channel, and do some work.
// If the soundness behavior is that the application terminates when the channel is empty,
// then the soundness is challenged, because ...
for ch.Len() > 0 {
// ... do stuff ...
}
// The loop may terminate even the length of the channel is not empty.One may argue that the following two lines are good, because it pops first then decrease the counter:
But compiler optimizations may reorder these two lines and hence a program bug is introduced. This is not a compiler's fault as there is no memory order guarantee for this case, even not in the recently proposed updates.
Nonetheless, if we intended to have a memory usage approximation, the method signatures may be renamed too:
-// Len returns len of In plus len of Out plus len of buffer.
-func (c UnboundedChan) Len() int
+// Cap returns the capacity of the channel.
+func (c *UnboundedChan) Cap() int
-// BufLen returns len of the buffer.
-func (c UnboundedChan) BufLen() int
+// EstimateLen returns an approximation of the length of
+// the channel where the error may be bound by 1.
+func (c *UnboundedChan) EstimateLen() intThe name
EstimateLenis not concise and may use different names such asApproxLen, etc.
Consider the following test:
The above test results in the following data race:
I think providing a
Len()method in abstracting an unbounded channel needs more careful handling (Mutex or lock-free pattern via an external atomic counter similar to the runtime channel implementation). The self-created internal buffer for an unbounded channel is maintained in a separate goroutine, a read vialen()creates a race condition with any writes with respect to that buffer.