Skip to content

Conversation

@S-Sarim
Copy link

@S-Sarim S-Sarim commented Dec 2, 2025

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

  • Concurrent calls to Engine.Routes() while registering new routes could cause simultaneous reads and writes to the internal trees structure.
  • This leads to race conditions, which are detectable with go test -race.
  • Example scenario:
r := gin.New()
go r.Routes()          // concurrent read
r.GET("/", handler)    // concurrent write

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

@codecov
Copy link

codecov bot commented Dec 2, 2025

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 98.68%. Comparing base (3dc1cd6) to head (a41520e).
⚠️ Report is 220 commits behind head on master.

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     
Flag Coverage Δ
?
--ldflags="-checklinkname=0" -tags sonic 98.67% <100.00%> (?)
-tags go_json 98.61% <100.00%> (?)
-tags nomsgpack 98.67% <100.00%> (?)
go-1.18 ?
go-1.19 ?
go-1.20 ?
go-1.21 ?
go-1.24 98.68% <100.00%> (?)
go-1.25 98.68% <100.00%> (?)
macos-latest 98.68% <100.00%> (-0.53%) ⬇️
ubuntu-latest 98.68% <100.00%> (-0.53%) ⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

@S-Sarim S-Sarim marked this pull request as draft December 2, 2025 08:09
@S-Sarim S-Sarim marked this pull request as ready for review December 2, 2025 08:14
@appleboy
Copy link
Member

appleboy commented Dec 5, 2025

Why is the route registration needed for concurrency-safe?

@S-Sarim
Copy link
Author

S-Sarim commented Dec 8, 2025

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants