Fix: make Engine.Routes() concurrency-safe (#4457)#4459
Fix: make Engine.Routes() concurrency-safe (#4457)#4459S-Sarim wants to merge 1 commit intogin-gonic:masterfrom
Conversation
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## master #4459 +/- ##
==========================================
- Coverage 99.21% 98.68% -0.53%
==========================================
Files 42 44 +2
Lines 3182 2976 -206
==========================================
- Hits 3157 2937 -220
- Misses 17 26 +9
- Partials 8 13 +5
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
|
Why is the route registration needed for concurrency-safe? |
Route registration modifies the route tree, and Routes() reads from it, this causes data race, to avoid it we need concurrency safe access, that is why route registration is part of the test to ensure reads and writes don’t conflict. |
|
What I mean is: why would we ever need to write code like this? In what situation would your example be used? r := gin.New()
go r.Routes() // concurrent read
r.GET("/", handler) // concurrent writeWe wouldn’t normally expect this kind of usage. |
fix: make Engine.Routes() concurrent-safe (#4457)
Description
This PR addresses a data race in
Engine.Routes()that occurs when routes are read concurrently with route registration.Problem
Engine.Routes()while registering new routes could cause simultaneous reads and writes to the internaltreesstructure.go test -race.Solution
-Introduced a sync.RWMutex in Engine to protect access to the route trees.
-Reads (Routes()) acquire a read lock, while route registrations acquire a write lock.
-Ensures thread-safe access to route trees without blocking unrelated reads.
-Added a unit test (TestRoutesConcurrent) to reproduce the race condition and verify the fix.
Testing
-Run go test ./... -race locally; the previous race warnings no longer appear.
-The test confirms that concurrent reads and writes to routes are handled safely.
References
-Fixes issue: #4457