-
Notifications
You must be signed in to change notification settings - Fork 8.5k
Fix: Literal colon routes not working with engine.Handler() and direct http.Handler usage #4415
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: master
Are you sure you want to change the base?
Conversation
…teral colon routes in all usage scenarios (gin-gonic#4413)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull Request Overview
This PR implements lazy initialization of route trees using sync.Once to ensure the updateRouteTrees() function is called exactly once when routes are first accessed, rather than being called explicitly in every Run* method. This approach handles literal colons in route paths (escaped as \:) more consistently across different server startup patterns.
- Added
routeTreesUpdated sync.Oncefield to Engine struct to ensure single initialization - Modified
ServeHTTPto callupdateRouteTrees()lazily on first request usingsync.Once - Added comprehensive tests covering literal colon handling in various scenarios
Reviewed Changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
| gin.go | Added routeTreesUpdated sync.Once field and modified ServeHTTP to lazily initialize route trees on first request |
| literal_colon_test.go | Added test suite covering literal colon functionality with different server initialization patterns and testing that route tree update happens only once |
Comments suppressed due to low confidence (1)
gin.go:542
- The
Run()method still explicitly callsupdateRouteTrees()before starting the server. With the new lazy initialization inServeHTTP, this meansupdateRouteTrees()could be called twice: once here and once by thesync.OnceinServeHTTP. SinceRun()callsengine.Handler()which returns the engine itself (when UseH2C is false), requests will trigger thesync.Oncecall. This creates inconsistent behavior whereRun()eagerly updates but directServeHTTPusage updates lazily. Consider usingengine.routeTreesUpdated.Do(func() { engine.updateRouteTrees() })here instead.
engine.updateRouteTrees()
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| // routeTreesUpdated ensures that the initialization or update of the route trees | ||
| // (used for routing HTTP requests) happens only once, even if called multiple times | ||
| // concurrently. It helps prevent race conditions and redundant setup operations. |
Copilot
AI
Nov 3, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The documentation states this prevents race conditions, but sync.Once alone doesn't prevent race conditions on the route trees themselves - it only ensures the function is called once. If routes can be added after the first ServeHTTP call, there could still be concurrent access issues. Consider clarifying that this ensures initialization happens once, and note any assumptions about route registration happening before serving requests.
| // routeTreesUpdated ensures that the initialization or update of the route trees | |
| // (used for routing HTTP requests) happens only once, even if called multiple times | |
| // concurrently. It helps prevent race conditions and redundant setup operations. | |
| // routeTreesUpdated ensures that initialization of the route trees (used for routing HTTP requests) | |
| // happens only once, even if called multiple times concurrently. Note: sync.Once only ensures | |
| // the initialization function is called once; it does not prevent race conditions on the route trees | |
| // themselves if routes are added after the first ServeHTTP call. It is assumed that all route | |
| // registration happens before serving requests to avoid concurrent access issues. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
sync.OnceensuresupdateRouteTrees()is called exactly once, not that it prevents all race conditions.- This implementation assumes routes are registered before serving begins, which is consistent with Gin's existing design. The
engine.Run()method already callsupdateRouteTrees()once with this same assumption - this PR simply extends that pattern toHandler()and directhttp.Handlerusage. - Adding routes dynamically after serving begins is not supported in Gin's current architecture (this is existing behavior, not introduced by this change).
|
Hi @appleboy , The golangci-lint was failing, I used |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull Request Overview
Copilot reviewed 2 out of 2 changed files in this pull request and generated 1 comment.
Comments suppressed due to low confidence (1)
gin.go:511
- Corrected grammar: 'do update to' should be 'does update to' or 'updates'.
// updateRouteTrees do update to the route trees
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| assert.Equal(t, http.StatusOK, w.Code) | ||
| } | ||
|
|
||
| _ = originalUpdate |
Copilot
AI
Nov 3, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This blank identifier assignment serves no purpose and suggests incomplete test implementation. Either remove this line or implement the intended verification logic for the test.
| _ = originalUpdate |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is a test case that checks whether updateRouteTrees is called only once.
Problem
Literal colon routes (e.g.,
/api:v1) currently fail when using:engine.Handler()http.Handlerusage (e.g.,&http.Server{Handler: engine})They only work when using engine.Run()
Related
Reproduction
Output Before Fix
Root Cause
The
updateRouteTrees()method converts escaped paths (/api\:v1) to actual colon paths (/api:v1), but it's only called insideengine.Run(). Other usage patterns never trigger this conversion.Fixes #4413
Solution
Add a
sync.Oncemechanism inServeHTTP()to ensureupdateRouteTrees()is called exactly once before processing the first request, regardless of how the engine is used.Changes
Modified Files:
gin.go: AddedrouteTreesUpdated sync.Oncefield to Engine struct.gin.go: UpdatedServeHTTP()to callupdateRouteTrees()once on first request.Testing
engine.Run()engine.Handler()ServeHTTP()usageupdateRouteTrees()is called only once across multiple requestsOutput after the Fix:
Result: Request to
/api:v1now returns 200 OK with the correct response!Breaking Changes
None. This is a pure bug fix with no API changes.
Pull Request Checklist
Please ensure your pull request meets the following requirements:
masterbranch.Thank you for contributing!