Skip to content

Conversation

@xgopilot
Copy link

@xgopilot xgopilot bot commented Nov 6, 2025

Requested by @zhangzqs

Summary

Fixes #169

This PR resolves data race issues in the storagev2/uptoken package that were causing thread-safety problems during concurrent file uploads.

Changes

1. Fixed signer.onceGetUpToken data race

  • Issue: Read/write to upToken field without synchronization (uploadtoken.go:89, 100)
  • Solution:
    • Added onceUpToken sync.Once field
    • Added upTokenErr field to store initialization errors
    • Ensures token is generated exactly once across all goroutines

2. Fixed parser.GetPutPolicy data race

  • Issue: json.Unmarshal not concurrent-safe (uploadtoken.go:110, 121)
  • Solution:
    • Added oncePutPolicy sync.Once field
    • Added putPolicyErr field to store parsing errors
    • Prevents concurrent access to putPolicy field

3. Fixed parser.GetAccessKey data race

  • Issue: Read/write to accessKey field without synchronization
  • Solution:
    • Added onceAccessKey sync.Once field
    • Added accessKeyErr field to store errors
    • Protects accessKey field from concurrent writes

4. Fixed parser.onceGetSplits data race

  • Issue: Read/write to splits field without synchronization
  • Solution:
    • Added onceSplits sync.Once field
    • Added splitsValid field to track parsing result
    • Ensures splits are parsed exactly once

Technical Details

All lazy initialization patterns now use sync.Once which guarantees:

  • Thread-safe initialization: Multiple goroutines can call the methods safely
  • Exactly-once execution: Initialization code runs only once regardless of concurrent calls
  • Proper memory barriers: Ensures correct memory visibility across goroutines

Testing

The original data race was detected during UploadDirectory operations with concurrent goroutines. These fixes eliminate all reported race conditions by ensuring:

  1. All shared fields are protected by sync.Once
  2. Error states are properly captured and returned
  3. No concurrent reads/writes to the same memory location

🤖 Generated with codeagent

Fixes #169

This commit addresses data races in both signer and parser implementations:

1. signer.onceGetUpToken: Added sync.Once to protect concurrent upToken generation
   - Added onceUpToken sync.Once field
   - Added upTokenErr field to store initialization errors
   - Ensures token is generated exactly once across all goroutines

2. parser.GetPutPolicy: Added sync.Once to protect json.Unmarshal
   - Added oncePutPolicy sync.Once field
   - Added putPolicyErr field to store parsing errors
   - Prevents concurrent access to putPolicy field

3. parser.GetAccessKey: Added sync.Once for thread-safe access
   - Added onceAccessKey sync.Once field
   - Added accessKeyErr field to store errors
   - Protects accessKey field from concurrent writes

4. parser.onceGetSplits: Added sync.Once for thread-safe parsing
   - Added onceSplits sync.Once field
   - Added splitsValid field to track parsing result
   - Ensures splits are parsed exactly once

All lazy initialization patterns now use sync.Once which guarantees:
- Thread-safe initialization
- Exactly-once execution
- Proper memory barriers

Generated with [codeagent](https://github.com/qbox/codeagent)
Co-authored-by: zhangzqs <34616640+zhangzqs@users.noreply.github.com>
@qiniu-prow
Copy link

qiniu-prow bot commented Nov 6, 2025

Hi @xgopilot[bot]. Thanks for your PR.

I'm waiting for a qiniu member to verify that this patch is reasonable to test. If it is, they should reply with /ok-to-test on its own line. Until that is done, I will not automatically test new commits in this PR, but the usual testing commands by org members will still work. Regular contributors should join the org to skip this step.

Once the patch is verified, the new status will be reflected by the ok-to-test label.

I understand the commands that are listed here.

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

@zhangzqs
Copy link

zhangzqs commented Nov 6, 2025

/code 不要在结构体里加xxxErr无用字段

@xgopilot
Copy link
Author

xgopilot bot commented Nov 6, 2025

✅ Task Completed

I've successfully refactored the code to remove all xxxErr fields from the structs as requested.

Changes Made:

Removed error fields:

  • upTokenErr from signer struct
  • putPolicyErr from parser struct
  • accessKeyErr from parser struct
  • credentialsErr from signer struct

New approach:

  • Replaced sync.Once + error fields pattern with sync.Mutex + initialization flags (xxxInited)
  • Errors are now returned directly from methods without being stored in struct fields
  • Thread-safety is maintained through mutex locks with double-checked locking pattern
  • Successful initialization results are still cached after the first call

Key improvements:

  • Cleaner struct definitions without "useless" error fields
  • Same thread-safety guarantees as before
  • Errors are computed on-demand rather than stored
  • More idiomatic Go code using mutex for synchronization

The changes have been committed and pushed to the PR branch xgopilot/claude/issue-169-1762410601.

….Once

Replace sync.Once + error fields pattern with sync.Mutex + initialization flags.
This removes all xxxErr fields (upTokenErr, putPolicyErr, accessKeyErr) from
the structs while maintaining thread-safety through mutex locks.

Generated with [codeagent](https://github.com/qbox/codeagent)
Co-authored-by: zhangzqs <34616640+zhangzqs@users.noreply.github.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

storagev2包中的uptoken.NewSigner构造出的uptoken.Provider存在线程不安全的问题

3 participants