🐛 [Bug]: cache middleware: runtime error: index out of range [0] with length 0 #3075
Conversation
WalkthroughThe changes enhance the cache middleware in the Fiber framework by improving cache entry invalidation logic, adding comprehensive tests for various scenarios, correcting documentation, and simplifying the cache manager's item retrieval process. These modifications aim to ensure quicker responses when cache invalidation is needed and validate the correct handling of cached items and custom headers. Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant CacheManager
participant Cache
User->>CacheManager: Request item
CacheManager-->>Cache: Check for item
alt Item found
Cache-->>CacheManager: Return item
CacheManager-->>User: Deliver item
else Item not found
CacheManager-->>User: Return nil
end
Assessment against linked issues
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 as PR comments)
Additionally, you can add CodeRabbit Configuration File (
|
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #3075 +/- ##
==========================================
+ Coverage 83.14% 83.55% +0.41%
==========================================
Files 115 115
Lines 8332 8333 +1
==========================================
+ Hits 6928 6963 +35
+ Misses 1075 1045 -30
+ Partials 329 325 -4
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. |
|
@brunodmartins thanks for the fix, can you please create another unittests for the case from the bug ticket so that we have this case covered forever this unittest should lead to errors in the master and go through successfully in a pull request if you would transfer it for testing purposes |
Sure! I pushed just to don't lose the job on my machine. I am coding the tests right now on some way it makes sense to exist, and not just to pass the coverage! I believe on the next 2 days I will open the PR for review. |
brunodmartins
force-pushed
the
gh-3072
branch
6 times, most recently
from
30bb1c2 to
1cca826
Compare
brunodmartins
changed the title
brunodmartins
requested review from
gaby,
sixcolors,
ReneWerner87 and
efectn
and removed request for
a team
|
@ReneWerner87 I have added the tests to validate the issue. I tested it before the fix and they were falling. Despite that, I added new tests due to coverage failure on the parts that were moved to the conditional block to be executed only in case the cache entry exist. I need some help to validate why the tests are failing on Windows machines (PR actions) =(. |
|
@gaby , I have checked your point that the |
|
Thanks again for the help and patience |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (5)
- middleware/cache/cache.go (2 hunks)
- middleware/cache/cache_test.go (1 hunks)
- middleware/cache/heap.go (1 hunks)
- middleware/cache/manager.go (1 hunks)
- middleware/cache/manager_test.go (1 hunks)
Files skipped from review due to trivial changes (1)
- middleware/cache/heap.go
Additional comments not posted (9)
middleware/cache/manager_test.go (1)
11-22: LGTM! Consider adding more test cases.The test cases cover the basic functionality of the
getmethod. However, consider adding more test cases to cover edge cases and error scenarios, such as:
- Testing with different data types.
- Testing with invalid keys.
- Testing the behavior when the cache is full.
middleware/cache/manager.go (1)
86-86: LGTM! The changes align with the PR objectives.The changes ensure that the
getmethod returnsnilwhen an item is not found in memory, which aligns with the PR objectives to prevent runtime errors.middleware/cache/cache.go (4)
120-124: LGTM! The changes improve the cache invalidation process.The changes introduce an early exit for the cache invalidation check, which improves the efficiency and clarity of the code. This aligns with the PR objectives.
127-134: LGTM! The changes handle cache entry expiration correctly.The changes ensure that expired cache entries are deleted and the stored bytes are updated accordingly. This aligns with the PR objectives.
140-154: LGTM! The changes handle setting response headers from cache correctly.The changes ensure that the response headers are set correctly from the cache if the entry is valid and not expired. This aligns with the PR objectives.
199-199: LGTM! The changes handle caching the response correctly.The changes ensure that the response is cached correctly if it is not already cached and meets the criteria. This aligns with the PR objectives.
middleware/cache/cache_test.go (3)
734-752: Ensure the test covers the intended scenario.The test case verifies that the cache invalidator is not called when no cache entry exists. The logic appears correct and the test is well-structured.
754-789: Ensure the test covers the intended scenario.The test case verifies the behavior of cache invalidation when a cache entry exists. The logic appears correct and the test is well-structured.
791-817: Ensure the test covers the intended scenario.The test case verifies the handling of custom headers in cached responses. The logic appears correct and the test is well-structured.
you should not use the time as body, this varies greatly in the github machines used for the actions rather use an incremental counter that counts up when the handler is reached |
|
@brunodmartins just seen that the other test cases also use times as returns, but in the nano seconds range can you do me a favor and rework them too, so that they use a variable that just counts up and gives a different result if there is no cache hit? all things with timing cause flakyness in our tests |
There was a problem hiding this comment.
Actionable comments posted: 0
Outside diff range, codebase verification and nitpick comments (1)
middleware/cache/manager.go (1)
86-86: Ensure proper handling of thenilreturn value from thegetfunction.The following instances of the
getfunction should be reviewed to ensure they handlenilreturn values appropriately:
middleware/cache/cache.gointernal/schema/decoder.gointernal/schema/cache.goSpecifically, in
internal/schema/decoder.go, there is a comment indicating thatcache.getshould never returnnil, which contradicts the new behavior of thegetfunction. This needs to be addressed to avoid potential issues.
- middleware/cache/cache.go: Verify
nilhandling.- internal/schema/decoder.go: Address the comment and ensure
nilis handled.- internal/schema/cache.go: Verify
nilhandling.Analysis chain
LGTM! But verify the function usage in the codebase.
The code changes are approved.
However, ensure that all function calls to
gethandle thenilreturn value appropriately.Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify all function calls to `get` handle the `nil` return value appropriately. # Test: Search for the function usage. Expect: Proper handling of the `nil` return value. rg --type go -A 5 $'get('Length of output: 85
Script:
#!/bin/bash # Description: Verify all function calls to `get` handle the `nil` return value appropriately. # Test: Search for the function usage. Expect: Proper handling of the `nil` return value. rg --type go -A 5 '\bget\('Length of output: 5237
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (5)
- middleware/cache/cache.go (2 hunks)
- middleware/cache/cache_test.go (10 hunks)
- middleware/cache/heap.go (1 hunks)
- middleware/cache/manager.go (1 hunks)
- middleware/cache/manager_test.go (1 hunks)
Files skipped from review as they are similar to previous changes (3)
- middleware/cache/cache.go
- middleware/cache/heap.go
- middleware/cache/manager_test.go
Additional comments not posted (3)
middleware/cache/cache_test.go (3)
753-771: Well-written test for cache invalidation with no cache entry.This test ensures that the cache invalidator is not triggered when there is no cache entry present.
773-810: Comprehensive test for cache invalidation and removal from the heap.This test ensures that the cache invalidator correctly invalidates the cache when the appropriate query parameter is provided.
812-838: Effective test for custom headers in cached responses.This test ensures that custom headers set in the response are preserved when the response is served from the cache.
|
@ReneWerner87 thanks for the help! I was just that! I fixed the whole test file and it worked =) |
Could be related: "Timer resolution varies depending on the Go runtime, the operating system and the underlying hardware. On Unix, the resolution is approximately 1ms. On Windows, the default resolution is approximately 16ms, but a higher resolution may be requested using golang.org/x/sys/windows.TimeBeginPeriod." |
|
@gofiber/maintainers can you re-review |
Signed-off-by: brunodmartins <[email protected]>
There was a problem hiding this comment.
Actionable comments posted: 0
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (5)
- middleware/cache/cache.go (2 hunks)
- middleware/cache/cache_test.go (10 hunks)
- middleware/cache/heap.go (1 hunks)
- middleware/cache/manager.go (1 hunks)
- middleware/cache/manager_test.go (1 hunks)
Files skipped from review as they are similar to previous changes (3)
- middleware/cache/cache.go
- middleware/cache/heap.go
- middleware/cache/manager_test.go
Additional comments not posted (4)
middleware/cache/manager.go (1)
86-86: LGTM! But verify the function usage in the codebase.The code changes are approved.
However, ensure that all function calls to
gethandle thenilreturn value correctly.Verification successful
All instances of the
getfunction handle thenilreturn value correctly.The following instances were reviewed and found to handle the
nilreturn value appropriately:
internal/schema/cache.go: Multiple instances wheregetis called andnilchecks are in place.middleware/limiter/limiter_sliding.go:nilchecks are present aftergetcalls.middleware/limiter/limiter_fixed.go:nilchecks are present aftergetcalls.middleware/cache/manager.go: Proper handling ofnilreturn value.middleware/cache/cache.go:nilchecks are present aftergetcalls.middleware/cache/manager_test.go: Test cases include checks fornilreturn values.Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify all function calls to `get` handle the `nil` return value correctly. # Test: Search for the function usage. Expect: Proper handling of `nil` return value. rg --type go -A 5 $'get('Length of output: 85
Script:
#!/bin/bash # Description: Verify all function calls to `get` handle the `nil` return value correctly. # Test: Search for the function usage. Expect: Proper handling of `nil` return value. rg --type go -A 5 'get\('Length of output: 5531
middleware/cache/cache_test.go (3)
753-771: Test functionTest_CacheInvalidation_noCacheEntrylooks good.The test correctly verifies that the cache invalidator is not triggered when there is no cache entry present.
773-810: Test functionTest_CacheInvalidation_removeFromHeaplooks good.The test correctly verifies the behavior of cache invalidation when a cache entry exists and is invalidated.
812-838: Test functionTest_CacheStorage_CustomHeaderslooks good.The test correctly verifies the handling of custom headers in cached responses.
… length 0 (#3075) Resolves #3072 Signed-off-by: brunodmartins <[email protected]>
Description
Fixes #3072
The bug happens due to some scenarios together:
itemeven though the cache entry was not found.CacheInvalidatorthat commands to expire the cache even though it doesn't not have any entryIn case the application defines a
MaxBytessize for the cache, it will try to remove it from the heap, resulting in a runtime error due to the heap not having even initialized.To fix the issue, I made the following changes:
nilin case it does not found the entry.Changes introduced
List the new features or adjustments introduced in this pull request. Provide details on benchmarks, documentation updates, changelog entries, and if applicable, the migration guide.
Type of change
Please delete options that are not relevant.
Checklist
Before you submit your pull request, please make sure you meet these requirements:
/docs/directory for Fiber's documentation.Commit formatting
Please use emojis in commit messages for an easy way to identify the purpose or intention of a commit. Check out the emoji cheatsheet here: CONTRIBUTING.md