Optimize user management map based storage #22
Conversation
- Replace slice-based client storage with map[string]api.Account for O(1) lookups - Add convertClientsMapToSlice() and convertClientsSliceToMap() helpers - Refactor syncUsers(), updateUser(), and removeUser() to use maps - Convert maps to slices only during JSON serialization in ToBytes() - Maintain backward compatibility in NewXRayConfig() for existing JSON configs Performance improvements: - updateUser(): O(n) -> O(1) (~5000x faster for 10k users) - removeUser(): O(n) -> O(1) (~5000x faster for 10k users) - syncUsers(): Eliminates repeated O(n) scans during rebuilds This resolves delayed user activation issues after /user/sync by eliminating expensive slice operations that caused multi-minute delays during high load or after node restarts.
- Add extractIDFromEmail() helper to extract ID part from 'id.username' format - Update all sorting functions to sort by ID (part before dot) instead of full email - Update updateUser() and removeUser() to use binary search with ID comparison - Maintain O(log n) search performance while correctly identifying users by ID This ensures binary search works correctly with the email field format where the ID is the part before the dot (e.g., '12345.username' -> ID: '12345')
|
Caution Review failedThe pull request is closed. WalkthroughID-based ordering was introduced in Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~50 minutes Poem
Pre-merge checks and finishing touches✅ Passed checks (3 passed)
📜 Recent review detailsConfiguration used: Organization UI Review profile: CHILL Plan: Pro 📒 Files selected for processing (1)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
backend/xray/config.go (1)
454-471: Locks are not released ifjson.Marshalpanics.The current pattern acquires all read locks before marshaling and releases them after, but if
json.Marshalpanics (e.g., due to a customMarshalJSONmethod or concurrent modification), the locks will never be released, causing a deadlock.Consider using a deferred cleanup pattern:
🔎 Suggested fix
func (c *Config) ToBytes() ([]byte, error) { - // Acquire read locks for all inbounds for _, i := range c.InboundConfigs { i.mu.RLock() } + defer func() { + for _, i := range c.InboundConfigs { + i.mu.RUnlock() + } + }() b, err := json.Marshal(c) - - // Release all locks - for _, i := range c.InboundConfigs { - i.mu.RUnlock() - } - if err != nil { return nil, err } return b, nil }
🤖 Fix all issues with AI Agents
In @backend/xray/config.go:
- Around line 73-135: The functions convertClientsMapToSlice and
convertClientsSliceToMap are dead code (never called) while ToBytes() uses
json.Marshal directly and syncUsers/updateUser/removeUser expect typed slices;
either delete these two conversion functions to remove unused code, or if you
intended map-based storage, refactor ToBytes(), syncUsers, updateUser, and
removeUser to use the map strategy and call
convertClientsMapToSlice/convertClientsSliceToMap where needed (ensure ToBytes
invokes convertClientsMapToSlice before marshalling and that user management
functions read/write i.Settings["clients"] as map[string]api.Account).
- Around line 137-201: convertClientsSliceToMap is dead and incompatible with
existing code: either remove it or fully integrate maps across the flow; to fix,
delete the convertClientsSliceToMap method (and any references) so
Settings["clients"] remains the slice types expected by syncUsers, updateUser,
and removeUser, or alternatively refactor the callers
(syncUsers/updateUser/removeUser) and all code that assumes slice types to
accept the map form and use convertClientsMapToSlice for JSON serialization;
pick one approach and apply it consistently so Settings["clients"] has a single
canonical type and no unused conversion functions remain.
🧹 Nitpick comments (1)
backend/xray/config.go (1)
311-325: Potential nil slice dereference if type assertion fails.Line 313 ignores the
okresult from the type assertion. IfSettings["clients"]isnilor a different type,clientswill benil. While the subsequent binary search and append operations happen to work on nil slices in Go, the intention is unclear and the behavior is fragile.Consider adding an explicit nil check or initializing an empty slice when the type assertion fails:
🔎 Suggested fix
- clients, _ := i.Settings["clients"].([]*api.VmessAccount) + clients, ok := i.Settings["clients"].([]*api.VmessAccount) + if !ok { + clients = []*api.VmessAccount{} + }This same pattern applies to all protocol cases in
updateUser()(lines 328, 343, 358, 373) andremoveUser()(lines 404, 413, 422, 433, 441).
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
backend/xray/config.go
🔇 Additional comments (4)
backend/xray/config.go (4)
26-34: LGTM!The helper function correctly extracts the ID portion before the first dot, with a sensible fallback to the full email when no dot is present.
215-226: Good improvement: Error handling for account creation.The addition of error handling that logs and continues (rather than silently failing or panicking) is a sensible improvement. The sorting by extracted ID enables the binary search operations in
updateUser()andremoveUser().
397-450: Binary search removal logic is correct.The use of
BinarySearchFuncfor O(log n) lookups is appropriate for the sorted slice structure. The same nil slice handling suggestion fromupdateUser()applies here as well.
223-227: Implementation uses O(log n) binary search, not O(1) maps as claimed in PR.The PR description states "Replace slice-based client storage with map[string]api.Account to enable O(1) lookups," but the actual implementation:
- Stores clients as sorted slices (e.g.,
[]*api.VmessAccount)- Uses
slices.BinarySearchFuncfor O(log n) lookups- Never calls the map conversion functions
This still provides a significant improvement over the original O(n) linear scans, but the PR description should be updated to accurately reflect the O(log n) binary search approach rather than O(1) map-based lookups.
- Deleted convertClientsMapToSlice() and convertClientsSliceToMap() methods as they are no longer needed. - This cleanup reduces code complexity and improves maintainability by removing redundant logic related to client storage conversion.
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
Summary by CodeRabbit
Bug Fixes
Refactor
✏️ Tip: You can customize this high-level summary in your review settings.