fix(middleware/session): mutex for thread safety #3049
Conversation
sixcolors
requested review from
gaby,
ReneWerner87 and
efectn
and removed request for
a team
|
Warning Rate limit exceeded@sixcolors has exceeded the limit for the number of commits or files that can be reviewed per hour. Please wait 4 minutes and 33 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. WalkthroughThe changes introduce a Changes
Sequence Diagram(s)(Skipping due to the nature of changes focusing on internal synchronization and not introducing new features or control flow complexities.) Poem
Tip Early access features: enabledWe are currently testing the following features in early access:
Note:
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 Configration 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: 0
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (3)
- middleware/session/session.go (10 hunks)
- middleware/session/session_test.go (2 hunks)
- middleware/session/store.go (3 hunks)
Files skipped from review due to trivial changes (1)
- middleware/session/store.go
Additional comments not posted (14)
middleware/session/session.go (9)
60-61: LGTM!The addition of the read lock ensures thread safety.
67-68: LGTM!The addition of the read lock ensures thread safety.
109-110: LGTM!The addition of the write lock ensures thread safety.
124-125: LGTM!The addition of the write lock ensures thread safety.
151-152: LGTM!The addition of the write lock ensures thread safety.
181-182: LGTM!The addition of the write lock ensures thread safety.
225-226: LGTM!The addition of the write lock ensures thread safety.
295-296: LGTM!The addition of the write lock ensures thread safety.
16-16: LGTM!The addition of the
sync.RWMutexfield ensures thread safety.middleware/session/session_test.go (5)
863-871: LGTM!The setup for the concurrency test is well-structured.
875-878: LGTM!The creation and execution of goroutines are well-handled.
961-967: LGTM!The wait group and error handling logic is well-implemented.
862-968: LGTM!The concurrency test function effectively tests the thread safety of session operations.
883-957: Verify the correctness of session operations.The session operations within the goroutines seem correct, but it's crucial to ensure they work as expected.
Verification successful
Verify the correctness of session operations.
The
Test_Session_Concurrencyfunction inmiddleware/session/session_test.gocorrectly tests the session operations, including setting, saving, retrieving, validating, deleting, and destroying session values. Error handling and concurrency management are also appropriately implemented.
middleware/session/session_test.golines 863-968Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify the correctness of session operations in the concurrency test. # Test: Run the concurrency test. Expect: No errors. go test -v -race -run Test_Session_Concurrency ./...Length of output: 124
Script:
#!/bin/bash # Search for the Test_Session_Concurrency function definition in the codebase ast-grep --lang go --pattern 'func Test_Session_Concurrency($_) $_'Length of output: 6664
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #3049 +/- ##
==========================================
+ Coverage 82.94% 83.06% +0.12%
==========================================
Files 115 115
Lines 8248 8260 +12
==========================================
+ Hits 6841 6861 +20
+ Misses 1076 1071 -5
+ Partials 331 328 -3
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. |
There was a problem hiding this comment.
Actionable comments posted: 0
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (1)
- middleware/session/session_test.go (2 hunks)
Files skipped from review as they are similar to previous changes (1)
- middleware/session/session_test.go
|
@sixcolors Did you try running it with |
There was a problem hiding this comment.
Actionable comments posted: 1
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (1)
- middleware/session/session.go (10 hunks)
Additional context used
GitHub Check: codecov/patch
middleware/session/session.go
[warning] 298-298: middleware/session/session.go#L298
Added line #L298 was not covered by tests
Additional comments not posted (8)
middleware/session/session.go (8)
16-16: LGTM!The addition of the
mu sync.RWMutexfield to theSessionstruct is appropriate for ensuring thread safety.
60-61: LGTM!Using
s.mu.RLock()ands.mu.RUnlock()in theFreshmethod is appropriate since it only reads thefreshfield.
67-68: LGTM!Using
s.mu.RLock()ands.mu.RUnlock()in theIDmethod is appropriate since it only reads theidfield.
109-110: LGTM!Using
s.mu.Lock()ands.mu.Unlock()in theDestroymethod is appropriate since it modifies the session data and interacts with external storage.
124-125: LGTM!Using
s.mu.Lock()ands.mu.Unlock()in theRegeneratemethod is appropriate since it modifies the session data and interacts with external storage.
151-152: LGTM!Using
s.mu.Lock()ands.mu.Unlock()in theResetmethod is appropriate since it modifies the session data and interacts with external storage.
181-182: LGTM!Using
s.mu.Lock()ands.mu.Unlock()in theSavemethod is appropriate since it modifies the session data and interacts with external storage.
225-226: LGTM!Using
s.mu.Lock()ands.mu.Unlock()in theSetExpirymethod is appropriate since it modifies theexpfield.
yes |
|
@coderabbitai full review |
Actions performedFull review triggered. |
|
I still see some data-races. Maybe it's caused because we acquired a session and not released somewhere? ╰─$ go test . -race -count=20 1 ↵
==================
WARNING: DATA RACE
Write at 0x00c000688318 by goroutine 167:
github.com/gofiber/fiber/v3/middleware/session.(*Store).Get()
/home/efectn/Devel/fiber-v3-constraint/middleware/session/store.go:82 +0x384
github.com/gofiber/fiber/v3/middleware/session.Test_Session_Concurrency.func1()
/home/efectn/Devel/fiber-v3-constraint/middleware/session/session_test.go:880 +0x14d
Previous read at 0x00c000688318 by goroutine 147:
github.com/gofiber/fiber/v3/middleware/session.(*Session).ID()
/home/efectn/Devel/fiber-v3-constraint/middleware/session/session.go:69 +0x8f
github.com/gofiber/fiber/v3/middleware/session.Test_Session_Store_Reset()
/home/efectn/Devel/fiber-v3-constraint/middleware/session/session_test.go:305 +0x2d1
testing.tRunner()
/usr/lib/go/src/testing/testing.go:1689 +0x21e
testing.(*T).Run.gowrap1()
/usr/lib/go/src/testing/testing.go:1742 +0x44
Goroutine 167 (running) created at:
github.com/gofiber/fiber/v3/middleware/session.Test_Session_Concurrency()
/home/efectn/Devel/fiber-v3-constraint/middleware/session/session_test.go:875 +0xab
testing.tRunner()
/usr/lib/go/src/testing/testing.go:1689 +0x21e
testing.(*T).Run.gowrap1()
/usr/lib/go/src/testing/testing.go:1742 +0x44
Goroutine 147 (running) created at:
testing.(*T).Run()
/usr/lib/go/src/testing/testing.go:1742 +0x825
testing.runTests.func1()
/usr/lib/go/src/testing/testing.go:2161 +0x85
testing.tRunner()
/usr/lib/go/src/testing/testing.go:1689 +0x21e
testing.runTests()
/usr/lib/go/src/testing/testing.go:2159 +0x8be
testing.(*M).Run()
/usr/lib/go/src/testing/testing.go:2027 +0xf17
main.main()
_testmain.go:85 +0x2bd
==================
--- FAIL: Test_Session_Cookie_In_Middleware_Chain (0.00s)
testing.go:1398: race detected during execution of test
--- FAIL: Test_Session_Custom_Config (0.00s)
testing.go:1398: race detected during execution of test
--- FAIL: Test_Session_Cookie (0.00s)
testing.go:1398: race detected during execution of test
--- FAIL: Test_Session_Store_Reset (0.00s)
testing.go:1398: race detected during execution of test
--- FAIL: Test_Session (0.00s)
testing.go:1398: race detected during execution of test
--- FAIL: Test_Session_Regenerate (0.00s)
--- FAIL: Test_Session_Regenerate/set_fresh_to_be_true_when_regenerating_a_session (0.00s)
testing.go:1398: race detected during execution of test
--- FAIL: Test_Store_Get (0.00s)
--- FAIL: Test_Store_Get/session_should_be_re-generated_if_it_is_invalid (0.00s)
testing.go:1398: race detected during execution of test
--- FAIL: Test_Session_Concurrency (0.00s)
testing.go:1398: race detected during execution of test
--- FAIL: Test_Session_Save (0.00s)
testing.go:1398: race detected during execution of test
--- FAIL: Test_Store_getSessionID (0.00s)
testing.go:1398: race detected during execution of test
--- FAIL: Test_Session_Deletes_Single_Key (0.00s)
testing.go:1398: race detected during execution of test
--- FAIL: Test_Session_Destroy (0.00s)
testing.go:1398: race detected during execution of test
--- FAIL: Test_Session_Types (0.01s)
testing.go:1398: race detected during execution of test
--- FAIL: Test_Session_Reset (0.00s)
testing.go:1398: race detected during execution of test
--- FAIL: Test_Session_Save_Expiration (0.00s)
--- FAIL: Test_Session_Save_Expiration/save_to_cookie (5.02s)
testing.go:1398: race detected during execution of test
==================
WARNING: DATA RACE
Write at 0x00c00041b578 by goroutine 766:
github.com/gofiber/fiber/v3/middleware/session.(*Store).Get()
/home/efectn/Devel/fiber-v3-constraint/middleware/session/store.go:82 +0x384
github.com/gofiber/fiber/v3/middleware/session.Test_Session_Concurrency.func1()
/home/efectn/Devel/fiber-v3-constraint/middleware/session/session_test.go:915 +0x3db
Previous read at 0x00c00041b578 by goroutine 733:
github.com/gofiber/fiber/v3/middleware/session.(*Session).ID()
/home/efectn/Devel/fiber-v3-constraint/middleware/session/session.go:69 +0x8f
github.com/gofiber/fiber/v3/middleware/session.Test_Session_Store_Reset()
/home/efectn/Devel/fiber-v3-constraint/middleware/session/session_test.go:305 +0x2d1
testing.tRunner()
/usr/lib/go/src/testing/testing.go:1689 +0x21e
testing.(*T).Run.gowrap1()
/usr/lib/go/src/testing/testing.go:1742 +0x44
Goroutine 766 (running) created at:
github.com/gofiber/fiber/v3/middleware/session.Test_Session_Concurrency()
/home/efectn/Devel/fiber-v3-constraint/middleware/session/session_test.go:875 +0xab
testing.tRunner()
/usr/lib/go/src/testing/testing.go:1689 +0x21e
testing.(*T).Run.gowrap1()
/usr/lib/go/src/testing/testing.go:1742 +0x44
Goroutine 733 (running) created at:
testing.(*T).Run()
/usr/lib/go/src/testing/testing.go:1742 +0x825
testing.runTests.func1()
/usr/lib/go/src/testing/testing.go:2161 +0x85
testing.tRunner()
/usr/lib/go/src/testing/testing.go:1689 +0x21e
testing.runTests()
/usr/lib/go/src/testing/testing.go:2159 +0x8be
testing.(*M).Run()
/usr/lib/go/src/testing/testing.go:2027 +0xf17
main.main()
_testmain.go:85 +0x2bd
==================
--- FAIL: Test_Store_Get (0.00s)
testing.go:1398: race detected during execution of test
--- FAIL: Test_Session_Store_Reset (0.00s)
testing.go:1398: race detected during execution of test
--- FAIL: Test_Session_Save (0.00s)
--- FAIL: Test_Session_Save/save_to_header (0.00s)
testing.go:1398: race detected during execution of test
--- FAIL: Test_Session_Save/save_to_cookie (0.00s)
testing.go:1398: race detected during execution of test
--- FAIL: Test_Session_Regenerate (0.00s)
--- FAIL: Test_Session_Regenerate/set_fresh_to_be_true_when_regenerating_a_session (0.00s)
testing.go:1398: race detected during execution of test
--- FAIL: Test_Store_getSessionID (0.00s)
testing.go:1398: race detected during execution of test
--- FAIL: Test_Session_Deletes_Single_Key (0.00s)
testing.go:1398: race detected during execution of test
--- FAIL: Test_Session_Concurrency (0.00s)
testing.go:1398: race detected during execution of test
--- FAIL: Test_Session (0.00s)
testing.go:1398: race detected during execution of test
--- FAIL: Test_Session_Cookie_In_Middleware_Chain (0.00s)
testing.go:1398: race detected during execution of test
--- FAIL: Test_Session_Destroy (0.00s)
testing.go:1398: race detected during execution of test
--- FAIL: Test_Session_Reset (0.00s)
testing.go:1398: race detected during execution of test
--- FAIL: Test_Session_Types (0.00s)
testing.go:1398: race detected during execution of test
--- FAIL: Test_Session_Save_Expiration (0.00s)
--- FAIL: Test_Session_Save_Expiration/save_to_cookie (5.01s)
testing.go:1398: race detected during execution of test
FAIL
FAIL github.com/gofiber/fiber/v3/middleware/session 100.314s
FAIL |
This pull request adds a mutex to the session middleware to ensure thread safety when accessing session data. The mutex is used to protect non-data fields and prevent race conditions. This improves the reliability and stability of the session middleware.