Skip to content

Conversation

Ripple-Sunzikang
Copy link

Modified balanceMap and GetBalancedStorage to use atomic.AddInt64 for incrementing, preventing errors during parallel access.

@ILoveScratch2 ILoveScratch2 requested a review from Copilot August 26, 2025 10:41
Copy link
Contributor

@Copilot Copilot AI left a 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 storing int 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))
Copy link
Preview

Copilot AI Aug 26, 2025

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

Suggested change
i := int(atomic.AddInt64(p, 1) % int64(storageNum))
i := int(atomic.AddInt64(p, 1)) % storageNum

Copilot uses AI. Check for mistakes.

Comment on lines +383 to +384
p, _ := balanceMap.LoadOrStore(virtualPath, new(int64))
i := int(atomic.AddInt64(p, 1) % int64(storageNum))
Copy link
Preview

Copilot AI Aug 26, 2025

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.

Suggested change
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.

@ILoveScratch2
Copy link
Member

目前存储的加载阶段并发量较低,不太可能出现高并发导致的问题。您是否在实际使用中遇到了因并发请求而引发的错误?能否提供更多具体场景?如因并发导致错误,这更可能是一个需要修复的 Bug,而非功能性改进(Feature)?

@Ripple-Sunzikang
Copy link
Author

目前存储的加载阶段并发量较低,不太可能出现高并发导致的问题。您是否在实际使用中遇到了因并发请求而引发的错误?能否提供更多具体场景?如因并发导致错误,这更可能是一个需要修复的 Bug,而非功能性改进(Feature)?

你好!这是我第一次提交pull request,对提交有一些不熟悉,对不起。
实际上,这是我在学习OpenList源码过程中看到的,我认为这个地方可以改进。所以,我认为这应该是一个Bug。

@ILoveScratch2
Copy link
Member

目前存储的加载阶段并发量较低,不太可能出现高并发导致的问题。您是否在实际使用中遇到了因并发请求而引发的错误?能否提供更多具体场景?如因并发导致错误,这更可能是一个需要修复的 Bug,而非功能性改进(Feature)?

你好!这是我第一次提交pull request,对提交有一些不熟悉,对不起。 实际上,这是我在学习OpenList源码过程中看到的,我认为这个地方可以改进。所以,我认为这应该是一个Bug。

提升安全性确实是更现代的Go代码实践

在此基础上,也希望说明一下我们处理这类改动的考量:
经过评估,在当前代码的实际运行中,确实不太会触发高并发场景下的问题。这类改进应以一个更系统化的方式进行——对项目中所有类似的、不安全的实现进行一次统一的审查和重构,以保持代码风格的一致性,并全面提升质量

因此,在所有情况被解决前,单独合并这个PR的可能性较低。

非常欢迎继续为提出任何建议或提交PR。

@Ripple-Sunzikang
Copy link
Author

目前存储的加载阶段并发量较低,不太可能出现高并发导致的问题。您是否在实际使用中遇到了因并发请求而引发的错误?能否提供更多具体场景?如因并发导致错误,这更可能是一个需要修复的 Bug,而非功能性改进(Feature)?

你好!这是我第一次提交pull request,对提交有一些不熟悉,对不起。 实际上,这是我在学习OpenList源码过程中看到的,我认为这个地方可以改进。所以,我认为这应该是一个Bug。

提升安全性确实是更现代的Go代码实践

在此基础上,也希望说明一下我们处理这类改动的考量: 经过评估,在当前代码的实际运行中,确实不太会触发高并发场景下的问题。这类改进应以一个更系统化的方式进行——对项目中所有类似的、不安全的实现进行一次统一的审查和重构,以保持代码风格的一致性,并全面提升质量

因此,在所有情况被解决前,单独合并这个PR的可能性较低。

非常欢迎继续为提出任何建议或提交PR。

非常感谢。实际上,类似于GetBalancedStorage的自增,我也思考过是否可以将大部分结构体的Id定义为UUIDv7类型,但那样的话要修改的内容就非常非常多了,这也一定是曾经有人思考过的问题。实际上,我正在尝试构建自己的网盘项目,并打算在项目中使用UUIDv7类型。
我从OpenList当中学习到了很多,感谢回复。

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

Successfully merging this pull request may close these issues.

3 participants