fix tag deletion issue #11
Conversation
|
Warning Rate limit exceeded@mdaliyan has exceeded the limit for the number of commits or files that can be reviewed per hour. Please wait 14 minutes and 48 seconds before requesting another review. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. 📒 Files selected for processing (1)
WalkthroughThe pull request introduces enhancements to the Changes
Sequence DiagramsequenceDiagram
participant Client
participant Pot
participant Shard
participant Tags
Client->>Pot: GetByTag("B")
Pot->>Shard: GetEntry
Shard-->>Pot: Return entries
Pot->>Tags: getEntriesWithTags
Tags-->>Pot: Return matching entries
Pot-->>Client: Return entries or error
Poem
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 6
🧹 Nitpick comments (3)
shard.go (1)
60-65: Unnecessary assignment before deletion inDropEntryIn the
DropEntrymethod, settings.entries[key] = nilbefore deleting the key from the map is unnecessary.You can simplify the code by removing the assignment:
func (s *shard[T]) DropEntry(key uint64) { s.rw.Lock() defer s.rw.Unlock() - s.entries[key] = nil delete(s.entries, key) }icache.go (1)
40-41: Fix documentation typos and improve clarityThe documentation comment has typos and could be clearer:
-// Get restores ann T entries previously stored with the given tag +// GetByTag retrieves all entries previously stored with the given tag.github/workflows/test.yml (1)
Line range hint
17-17: Consider updating minimum Go version requirementThe test matrix includes Go 1.18.1 which is quite old. Given the new features and improvements in newer versions, consider:
- Updating the minimum supported version to at least 1.19 or 1.20
- Documenting the minimum required Go version in the README
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
.github/workflows/test.yml(1 hunks)icache.go(1 hunks)pot.go(6 hunks)shard.go(3 hunks)tags.go(1 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (16)
- GitHub Check: lint-and-test (1.22.3, windows-latest)
- GitHub Check: lint-and-test (1.22.3, macos-latest)
- GitHub Check: lint-and-test (1.22.3, ubuntu-latest)
- GitHub Check: lint-and-test (1.21.10, windows-latest)
- GitHub Check: lint-and-test (1.21.10, macos-latest)
- GitHub Check: lint-and-test (1.21.10, ubuntu-latest)
- GitHub Check: lint-and-test (1.20.14, windows-latest)
- GitHub Check: lint-and-test (1.20.14, macos-latest)
- GitHub Check: lint-and-test (1.20.14, ubuntu-latest)
- GitHub Check: lint-and-test (1.19.13, windows-latest)
- GitHub Check: lint-and-test (1.19.13, macos-latest)
- GitHub Check: lint-and-test (1.19.13, ubuntu-latest)
- GitHub Check: lint-and-test (1.18.1, windows-latest)
- GitHub Check: lint-and-test (1.18.1, macos-latest)
- GitHub Check: lint-and-test (1.18.1, ubuntu-latest)
- GitHub Check: coverage
🔇 Additional comments (2)
pot.go (1)
101-107: Ensure thread safety when accessing entries fromGetByTagThe
GetByTagmethod retrieves entries usingp.tags.getEntriesWithTags, which may return entries that could be modified concurrently.Confirm that the entries returned by
GetByTagare accessed safely. You may need to lock the entries or return copies to prevent concurrent modification issues..github/workflows/test.yml (1)
12-12: Verify the need for 'test' branch in workflow triggersAdding the 'test' branch to PR triggers seems unusual. Typically, feature branches or development branches are used for testing before merging to master/main. Consider if this aligns with your branching strategy.
Run this script to check the branch usage patterns:
✅ Verification successful
'test' branch in workflow triggers appears intentional
The workflow configuration shows a well-structured branch strategy where:
- Feature development occurs in
feature/*branches- Pull requests can target
master,v1, ortestbranches- Direct pushes to
masterare explicitly disabledThe inclusion of the
testbranch alongsidemasterandv1in PR triggers appears to be part of a deliberate branch protection strategy.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Check branch usage patterns in the repository git branch -r | grep -E 'test|feature|dev' | sort git log --format="%H %d %s" --graph --all | grep -E 'test|feature|dev'Length of output: 51
Script:
#!/bin/bash # Check workflow configurations and branch patterns fd -e yml -e yaml . .github/workflows/ --exec cat {} cat .github/workflows/test.ymlLength of output: 2953
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Nitpick comments (1)
pot.go (1)
177-200: Optimize expired entries handlingThe current implementation has potential issues:
- Unnecessary allocation of expiredEntries slice
- Double iteration over expired entries
Consider this more efficient approach:
func (p *pot[T]) dropExpiredEntries(at time.Time) { now := at.UnixNano() p.windowRW.Lock() defer p.windowRW.Unlock() var expiredWindows int - var expiredEntries entrySlice[T] for _, entry := range p.window { if entry == nil { expiredWindows++ continue } entry.rw.Lock() if now < entry.expiresAt { entry.rw.Unlock() break } entry.deleted = true - expiredEntries = append(expiredEntries, entry) + p.dropEntry(entry) expiredWindows++ entry.rw.Unlock() } - if len(expiredEntries) > 0 { + if expiredWindows > 0 { p.window = append(entrySlice[T]{}, p.window[expiredWindows:]...) - for _, e := range expiredEntries { - p.dropEntry(e) - } } }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
icache_test.go(1 hunks)pot.go(5 hunks)shard.go(3 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- shard.go
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: lint-and-test (1.20.14, windows-latest)
🔇 Additional comments (1)
pot.go (1)
55-56: Verify proper mutex ordering to prevent deadlocksThe addition of deferred mutex unlocks is good, but we need to ensure proper mutex ordering when multiple locks are acquired.
Let's verify the mutex acquisition order across the codebase:
Also applies to: 63-65, 70-71, 77-87, 110-112, 129-131, 155-160, 164-165
✅ Verification successful
Mutex ordering is correct and safe
The codebase follows a consistent lock hierarchy (pot -> entry) with proper deferred unlocks, preventing potential deadlocks.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Check for potential deadlock scenarios by analyzing mutex lock ordering # Look for methods that acquire multiple locks rg -A 5 'Lock|RLock' --type go # Look for nested mutex operations ast-grep --pattern 'Lock() { $$$ Lock() $$$ }'Length of output: 2969
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (2)
pot.go (2)
132-134:⚠️ Potential issueRace condition in tags.add still needs attention.
While the pot-level locking is good, there's still a potential race condition in
p.tags.add(e)as mentioned in the past review.Ensure that the
addmethod in thetagsstruct properly synchronizes access tot.pairs.
158-163:⚠️ Potential issueRace condition in getEntriesWithTags still needs attention.
While the pot-level locking is good, there's still a potential race condition in
getEntriesWithTagsas mentioned in the past review.Ensure that
getEntriesWithTagsis thread-safe by adding proper synchronization when accessingt.pairs.
🧹 Nitpick comments (1)
internal_test.go (1)
Line range hint
74-86: Consider enhancing the failAssertion functionTwo suggestions to improve error reporting:
- Add
t.Helper()tofailAssertionas it's also a helper function- Use string formatting for better error messages
Here's the suggested implementation:
func failAssertion(t *testing.T, not bool, expected, actual interface{}, msg ...string) { + t.Helper() if len(msg) != 0 { - fmt.Println("assertion: ", msg) + fmt.Printf("assertion: %v\n", msg) } - if not { - fmt.Print("not expected:") - } else { - fmt.Print("expected:") - } - fmt.Printf("%v\n", expected) - fmt.Printf("actual: %v\n", actual) + expectation := "expected" + if not { + expectation = "not expected" + } + fmt.Printf("%s: %v\nactual: %v\n", expectation, expected, actual) t.Fail() }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
icache.go(1 hunks)icache_test.go(1 hunks)internal_test.go(1 hunks)pot.go(4 hunks)shard.go(3 hunks)
🚧 Files skipped from review as they are similar to previous changes (3)
- icache.go
- icache_test.go
- shard.go
⏰ Context from checks skipped due to timeout of 90000ms (2)
- GitHub Check: lint-and-test (1.22.3, windows-latest)
- GitHub Check: lint-and-test (1.20.14, windows-latest)
🔇 Additional comments (9)
internal_test.go (3)
27-27: Great addition of t.Helper()!Adding
t.Helper()improves test failure reporting by showing the actual test line number instead of the helper function, making it easier to debug failed tests.
34-34: Consistent implementation across all assertion functions!The addition of
t.Helper()has been consistently applied to all assertion functions, which is great for maintaining a uniform approach to test failure reporting.Also applies to: 41-41, 46-46, 51-51, 56-56, 61-61, 68-68
Line range hint
1-86: Verify impact of removing random seed initializationThe removal of the
init()function that seeded the random number generator could affect tests that rely on randomness.Let's verify if there are any tests depending on random number generation:
✅ Verification successful
Random seed initialization removal is safe
The random number generator is only used in benchmarks to simulate workload by generating random test data and accessing it randomly. Since benchmarks don't make assertions on random values, the default seed (1) provided by
math/randis sufficient.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Search for usage of math/rand or crypto/rand in test files echo "Searching for random number usage in tests..." rg -t go --glob '*_test.go' 'rand\.'Length of output: 288
Script:
#!/bin/bash # Check the content of benchmark_test.go echo "Content of benchmark_test.go:" cat benchmark_test.goLength of output: 1146
pot.go (6)
54-55: LGTM! Good improvement in mutex handling.Adding
deferfor mutex unlock is a good practice as it ensures the mutex is always released, even in case of panics.
62-87: Well-structured concurrent access patterns!Good improvements:
- Using RLock for read-only operations
- Proper defer usage for unlocks
- Entry-level locking in ExpireTime
- Abstraction of entry retrieval logic
113-128: LGTM! Good concurrent access handling.The method correctly handles:
- Pot-level read locking
- Entry-level read locking
- Proper cleanup of deleted entries
180-204: LGTM! Good improvements in expired entries handling.The changes improve efficiency by:
- Using proper synchronization
- Optimizing the expired entries cleanup process
- Maintaining proper locking order
209-211: LGTM! Good cleanup of tag management.The changes improve the tag cleanup process and remove the ineffective nil assignment as suggested in the past review.
101-110: 🛠️ Refactor suggestionAdd entry-level locking to prevent races.
While the pot-level RLock is good, the returned entries should be protected against concurrent modifications.
Apply this diff:
func (p *pot[T]) GetByTag(tag string) ([]*entry[T], error) { p.windowRW.RLock() defer p.windowRW.RUnlock() entries := p.tags.getEntriesWithTags(tagKeyGen(tag)...) + // Lock entries to prevent races + for _, e := range entries { + e.rw.RLock() + if e.deleted { + e.rw.RUnlock() + continue + } + // Caller must unlock entries after use + } if len(entries) == 0 { return nil, ErrNotFound } return entries, nil }Likely invalid or redundant comment.
|
@coderabbitai full review |
✅ Actions performedFull review triggered. |
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (1)
shard.go (1)
34-38:⚠️ Potential issuePossible nil pointer dereference in
EntryExistsmethodIn the
EntryExistsmethod, after retrieving the entrye, there is a potential for a nil pointer dereference ifeisnil. Accessinge.deletedwheneisnilwould cause a panic.Apply this diff to fix the issue:
func (s *shard[T]) EntryExists(key uint64) bool { s.rw.RLock() defer s.rw.RUnlock() e, ok := s.entries[key] + if !ok || e == nil { + return false + } return !e.deleted }This ensures that you only access
e.deletedifeis notnil.
🧹 Nitpick comments (4)
internal_test.go (1)
Line range hint
74-85: Add t.Helper() to failAssertion functionFor consistency with other helper functions and improved error reporting, consider adding
t.Helper()to thefailAssertionfunction as well.func failAssertion(t *testing.T, not bool, expected, actual interface{}, msg ...string) { + t.Helper() if len(msg) != 0 { fmt.Println("assertion: ", msg) }tags.go (1)
9-9: [Nitpick] Suggest aligning struct fields for readabilityFor improved readability, consider aligning the struct fields in
tags[T].Apply this diff:
type tags[T any] struct { rw sync.RWMutex pairs map[uint64]entries[T] }icache.go (1)
40-42: Enhance method documentation for GetByTagWhile the documentation covers the error case, it would be helpful to add:
- A brief description of what constitutes a tag
- Whether the order of returned entries is guaranteed
- Whether the returned slice can be modified by the caller
Consider expanding the documentation like this:
-// GetByTag restores ann T entries previously stored with the given tag -// returns ErrNotFound if there is no entries to return. +// GetByTag returns all entries that were stored with the given tag. +// The order of returned entries is not guaranteed. +// Returns ErrNotFound if no entries exist with the given tag. +// The returned slice can be safely modified by the caller.pot.go (1)
186-205: Optimize memory usage in expired entries handlingThe current implementation might hold onto memory longer than necessary:
- The expired entries slice creates an additional allocation
- The window slice copy could be more efficient
Consider this optimization:
- var expiredEntries entrySlice[T] + expiredEntries := make([]*entry[T], 0, len(p.window)) for _, entry := range p.window { if entry == nil { expiredWindows++ continue } entry.rw.Lock() if now < entry.expiresAt { entry.rw.Unlock() break } entry.deleted = true expiredEntries = append(expiredEntries, entry) expiredWindows++ entry.rw.Unlock() } if len(expiredEntries) > 0 { - p.window = append(entrySlice[T]{}, p.window[expiredWindows:]...) + if expiredWindows < len(p.window) { + p.window = p.window[expiredWindows:] + p.window = p.window[:len(p.window):len(p.window)] // trim capacity + } else { + p.window = nil + } for _, e := range expiredEntries { p.dropEntry(e) } }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
icache.go(1 hunks)icache_test.go(1 hunks)internal_test.go(1 hunks)pot.go(4 hunks)shard.go(3 hunks)tags.go(2 hunks)
🔇 Additional comments (11)
internal_test.go (1)
27-27: Great addition of t.Helper() calls!Adding
t.Helper()to all assertion functions is a good practice. It improves test failure reporting by ensuring that error messages point to the test file line number rather than the helper function implementation.Also applies to: 34-34, 41-41, 46-46, 51-51, 56-56, 61-61, 68-68
tags.go (2)
19-20: Proper synchronization inaddmethodThe
addmethod now correctly acquires a write lock ont.rwbefore modifyingt.pairs. This ensures thread-safe operation when adding entries with their associated tags.
47-65: Verify locking order to prevent potential deadlocksIn
getEntriesWithTags, you acquire a read lock ont.rwand then, within the loop, acquire read locks one.rw. Ensure that elsewhere in the code, there are no instances wheree.rwis locked beforet.rw, as this could lead to a deadlock due to inconsistent lock ordering.Consider reviewing the codebase to confirm that all locks are acquired in a consistent order.
shard.go (2)
28-31: Proper use of read lock inLenmethodThe
Lenmethod now correctly uses a read lock withdeferto ensure thread-safe access tos.entries.
53-54: Proper synchronization inSetEntrymethodThe
SetEntrymethod now acquires a write lock before modifyings.entries, ensuring thread safety when setting entries.icache_test.go (4)
91-99: Test data consistency corrected inmultiple by tagsub-testThe test case now correctly sets the item with key
"3"and valuenewItem("3"), ensuring consistency between the key and the item's ID. This addresses the previous inconsistency where the item's ID did not match its key.
98-99: Additional assertion improves test coverageAdding
assertIsTrue(t, p.Exists("3"))after dropping tags enhances the test by verifying that the item with key"3"still exists as expected.
100-106: Verification ofGetByTagafter tag dropThe added assertions check the behavior of
GetByTagafter tags have been dropped, ensuring that the method returns the correct items and errors. This strengthens the test suite by validating the tag-based retrieval functionality.
110-131: Addition ofTestGetByTagsfunction enhances test suiteThe new
TestGetByTagsfunction effectively tests theGetByTagmethod, verifying correct retrieval of items by tags and proper error handling when querying a non-existent tag. This addition improves the overall test coverage for tag-based operations.pot.go (2)
53-54: LGTM! Improved thread safetyGood addition of
deferfor mutex unlock, ensuring the lock is always released even if the method panics.
212-214: LGTM! Improved tag cleanupGood implementation of tag cleanup that removes tags only when no other entries exist.
|
https://github.com/coderabbitai full review |
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
pot.go (2)
78-88: Consider using time.UnixNano for consistent precision.The conversion between UnixNano (used in
Set) and Unix (used here) might lead to precision loss.- ti := time.Unix(e.expiresAt, 0) + ti := time.Unix(0, e.expiresAt)
193-212: Optimize slice operations in dropExpiredEntries.Consider pre-allocating the new slice with the correct capacity to avoid potential reallocation.
if expiredWindows > 0 { - p.window = append(entrySlice[T]{}, p.window[expiredWindows:]...) + remaining := len(p.window) - expiredWindows + newWindow := make(entrySlice[T], remaining) + copy(newWindow, p.window[expiredWindows:]) + p.window = newWindow }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
icache.go(1 hunks)icache_test.go(1 hunks)internal_test.go(1 hunks)pot.go(4 hunks)shard.go(3 hunks)
🚧 Files skipped from review as they are similar to previous changes (4)
- icache_test.go
- icache.go
- shard.go
- internal_test.go
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: lint-and-test (1.20.14, windows-latest)
🔇 Additional comments (6)
pot.go (6)
53-54: LGTM! Good use of deferred mutex unlock.The addition of
deferfor mutex unlock is a good practice that ensures proper cleanup even in case of panics.
61-63: LGTM! Good addition of read locks for thread safety.The addition of RLock protection for read-only operations is correct and necessary for concurrent access safety.
Also applies to: 68-74, 78-88
125-127: LGTM! Consistent use of read lock.The addition of RLock protection is correct and maintains consistency with other read operations.
144-147: LGTM! Race condition fixed.The addition of proper Lock protection fixes the previously identified race condition in the
Setmethod.
171-176: LGTM! Thread safety improved.The addition of proper Lock protection fixes the previously identified race condition in the
DropTagsmethod.
218-220: LGTM! Good memory management.The addition of tag cleanup logic helps prevent memory leaks by removing unused tags.
|
https://github.com/coderabbitai full review |
Changelog
Code Quality Assurance
Summary by CodeRabbit
New Features
GetByTagmethod to retrieve cache entries by tag.Bug Fixes
Tests
Refactor