-
-
Notifications
You must be signed in to change notification settings - Fork 1.2k
fix(op): use atomic counter in GetBalancedStorage to prevent concurre… #1156
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
base: main
Are you sure you want to change the base?
Conversation
…ncy issues in round-robin selection
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull Request Overview
This PR fixes a concurrency issue in the GetBalancedStorage
function by replacing non-atomic counter operations with atomic operations to prevent race conditions during parallel access.
- Replaces direct integer operations with atomic counter using
sync/atomic
package - Changes
balanceMap
type from storingint
to storing*int64
pointers for atomic operations - Updates the load balancing logic to use
atomic.AddInt64
for thread-safe counter incrementation
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
i = (i + 1) % storageNum | ||
balanceMap.Store(virtualPath, i) | ||
p, _ := balanceMap.LoadOrStore(virtualPath, new(int64)) | ||
i := int(atomic.AddInt64(p, 1) % int64(storageNum)) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The modulo operation should be applied after converting back to int, not before. The current code performs modulo on int64 which could cause incorrect results when storageNum is large. Change to: i := int(atomic.AddInt64(p, 1)) % storageNum
i := int(atomic.AddInt64(p, 1) % int64(storageNum)) | |
i := int(atomic.AddInt64(p, 1)) % storageNum |
Copilot uses AI. Check for mistakes.
p, _ := balanceMap.LoadOrStore(virtualPath, new(int64)) | ||
i := int(atomic.AddInt64(p, 1) % int64(storageNum)) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The atomic counter will continuously increment and may eventually overflow. Consider using atomic.AddInt64(p, 1) - 1
and then applying modulo to get zero-based indexing, or implement a reset mechanism when the counter approaches overflow limits.
p, _ := balanceMap.LoadOrStore(virtualPath, new(int64)) | |
i := int(atomic.AddInt64(p, 1) % int64(storageNum)) | |
val := atomic.AddInt64(p, 1) | |
// Reset counter if approaching overflow | |
if val > math.MaxInt64-int64(storageNum) { | |
atomic.StoreInt64(p, 0) | |
val = 0 | |
} | |
i := int(val % int64(storageNum)) |
Copilot uses AI. Check for mistakes.
目前存储的加载阶段并发量较低,不太可能出现高并发导致的问题。您是否在实际使用中遇到了因并发请求而引发的错误?能否提供更多具体场景?如因并发导致错误,这更可能是一个需要修复的 Bug,而非功能性改进(Feature)? |
你好!这是我第一次提交pull request,对提交有一些不熟悉,对不起。 |
提升安全性确实是更现代的Go代码实践 在此基础上,也希望说明一下我们处理这类改动的考量: 因此,在所有情况被解决前,单独合并这个PR的可能性较低。 非常欢迎继续为提出任何建议或提交PR。 |
非常感谢。实际上,类似于GetBalancedStorage的自增,我也思考过是否可以将大部分结构体的Id定义为UUIDv7类型,但那样的话要修改的内容就非常非常多了,这也一定是曾经有人思考过的问题。实际上,我正在尝试构建自己的网盘项目,并打算在项目中使用UUIDv7类型。 |
Modified balanceMap and GetBalancedStorage to use atomic.AddInt64 for incrementing, preventing errors during parallel access.