gin
gin copied to clipboard
gin-gonic
fix(tree): correctly expand the capacity of params
Even though #2690 was fixed in #2755, the solution fixes the panic but doesn't actually expand the capacity of the params slice which means that the handler never gets those parameters in context.Params (as pointed out in https://github.com/gin-gonic/gin/issues/2690#issuecomment-870207672). I have also added a router test to illustrate this - without the changes in tree.go it fails:
=== RUN TestRouteParamsNotEmpty
routes_test.go:326:
Error Trace: /repos/gin/routes_test.go:326
Error: Not equal:
expected: "john"
actual : ""
Diff:
--- Expected
+++ Actual
@@ -1 +1 @@
-john
+
Test: TestRouteParamsNotEmpty
routes_test.go:327:
Error Trace: /repos/gin/routes_test.go:327
Error: Not equal:
expected: "smith"
actual : ""
Diff:
--- Expected
+++ Actual
@@ -1 +1 @@
-smith
+
Test: TestRouteParamsNotEmpty
--- FAIL: TestRouteParamsNotEmpty (0.00s)
Codecov Report
All modified and coverable lines are covered by tests :white_check_mark:
Comparison is base (
44d0dd7) 99.20% compared to head (bb52068) 99.21%.
Additional details and impacted files
@@ Coverage Diff @@
## master #3502 +/- ##
=======================================
Coverage 99.20% 99.21%
=======================================
Files 42 42
Lines 3163 3175 +12
=======================================
+ Hits 3138 3150 +12
Misses 17 17
Partials 8 8
| Flag | Coverage Δ | |
|---|---|---|
99.21% <100.00%> (+<0.01%) |
:arrow_up: | |
| go-1.18 | 99.11% <100.00%> (+<0.01%) |
:arrow_up: |
| go-1.19 | 99.21% <100.00%> (+<0.01%) |
:arrow_up: |
| go-1.20 | 99.21% <100.00%> (+<0.01%) |
:arrow_up: |
| macos-latest | 99.21% <100.00%> (+0.09%) |
:arrow_up: |
| ubuntu-latest | 99.21% <100.00%> (+<0.01%) |
:arrow_up: |
Flags with carried forward coverage won't be shown. Click here to find out more.
:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Have feedback on the report? Share it here.
![codecov[bot] avatar](https://avatars.githubusercontent.com/in/254?v=4 "Published by a pub.dev verified publisher"){kind=small}
Hi,
I noticed the Run Tests / macos-latest @ Go 1.19 -tags "sonic avx" (pull_request) CI job failed. It appears that it expects fewer mallocs:
=== RUN TestPathCleanMallocs
path_test.go:85:
Error Trace: /Users/runner/work/gin/gin/path_test.go:85
Error: Not equal:
expected: float64(1683)
actual : int(0)
Test: TestPathCleanMallocs
--- FAIL: TestPathCleanMallocs (0.07s)
Not sure why it doesn't fail on older Go version or Ubuntu. I don't have a macOS machine to reproduce and it also doesn't look related to my changes. Would you mind having a look?
Hello dear folks, hope you're having a great day ! Is this PR something you would be willing to consider including/merging ? :)
Hello @appleboy 👋🏻, is there something we could do for this PR to land in master? 🙏🏻
We maintain our own fork with the patch in production, and we think the fix is important for others as well.
Sure.
Here are the results of running the following command:
go test -bench=. -run '^Benchmark.*$' -count 10
and the comparison:
$ benchstat master.txt fix-missing-params.txt
goos: darwin
goarch: arm64
pkg: github.com/gin-gonic/gin
│ master.txt │ fix-missing-params.txt │
│ sec/op │ sec/op vs base │
OneRoute-10 26.87n ± 1% 26.74n ± 0% ~ (p=0.059 n=10)
RecoveryMiddleware-10 31.45n ± 1% 31.77n ± 1% +1.02% (p=0.001 n=10)
LoggerMiddleware-10 789.4n ± 0% 791.7n ± 1% ~ (p=0.128 n=10)
ManyHandlers-10 821.5n ± 0% 821.2n ± 1% ~ (p=0.643 n=10)
5Params-10 62.91n ± 0% 65.23n ± 1% +3.69% (p=0.000 n=10)
OneRouteJSON-10 162.8n ± 0% 165.7n ± 1% +1.78% (p=0.000 n=10)
OneRouteHTML-10 608.1n ± 1% 614.4n ± 1% +1.02% (p=0.001 n=10)
OneRouteSet-10 150.0n ± 0% 149.8n ± 0% ~ (p=0.252 n=10)
OneRouteString-10 69.95n ± 0% 69.74n ± 0% -0.30% (p=0.002 n=10)
ManyRoutesFist-10 26.77n ± 0% 26.78n ± 0% ~ (p=0.643 n=10)
ManyRoutesLast-10 26.28n ± 0% 26.43n ± 0% +0.55% (p=0.012 n=10)
404-10 37.12n ± 1% 37.31n ± 1% +0.51% (p=0.028 n=10)
404Many-10 42.13n ± 1% 42.15n ± 0% ~ (p=0.516 n=10)
Github-10 1.164µ ± 1% 1.159µ ± 0% ~ (p=0.169 n=10)
ParallelGithub-10 728.0n ± 2% 725.6n ± 2% ~ (p=0.739 n=10)
ParallelGithubDefault-10 717.2n ± 2% 711.5n ± 2% ~ (p=1.000 n=10)
PathClean-10 877.3n ± 0% 879.0n ± 1% ~ (p=0.072 n=10)
PathCleanLong-10 2.694m ± 1% 2.678m ± 0% ~ (p=0.123 n=10)
ParseAccept-10 130.4n ± 0% 130.1n ± 1% ~ (p=0.538 n=10)
geomean 259.5n 260.3n +0.30%
│ master.txt │ fix-missing-params.txt │
│ B/op │ B/op vs base │
OneRoute-10 0.000 ± 0% 0.000 ± 0% ~ (p=1.000 n=10) ¹
RecoveryMiddleware-10 0.000 ± 0% 0.000 ± 0% ~ (p=1.000 n=10) ¹
LoggerMiddleware-10 268.0 ± 0% 268.0 ± 0% ~ (p=1.000 n=10) ¹
ManyHandlers-10 268.0 ± 0% 268.0 ± 0% ~ (p=1.000 n=10) ¹
5Params-10 0.000 ± 0% 0.000 ± 0% ~ (p=1.000 n=10) ¹
OneRouteJSON-10 48.00 ± 0% 48.00 ± 0% ~ (p=1.000 n=10) ¹
OneRouteHTML-10 256.0 ± 0% 256.0 ± 0% ~ (p=1.000 n=10) ¹
OneRouteSet-10 336.0 ± 0% 336.0 ± 0% ~ (p=1.000 n=10) ¹
OneRouteString-10 48.00 ± 0% 48.00 ± 0% ~ (p=1.000 n=10) ¹
ManyRoutesFist-10 0.000 ± 0% 0.000 ± 0% ~ (p=1.000 n=10) ¹
ManyRoutesLast-10 0.000 ± 0% 0.000 ± 0% ~ (p=1.000 n=10) ¹
404-10 0.000 ± 0% 0.000 ± 0% ~ (p=1.000 n=10) ¹
404Many-10 0.000 ± 0% 0.000 ± 0% ~ (p=1.000 n=10) ¹
Github-10 1.056Ki ± 0% 1.056Ki ± 0% ~ (p=1.000 n=10) ¹
PathClean-10 112.0 ± 0% 112.0 ± 0% ~ (p=1.000 n=10) ¹
PathCleanLong-10 3.068Mi ± 0% 3.068Mi ± 0% ~ (p=0.372 n=10)
geomean ² -0.00% ²
¹ all samples are equal
² summaries must be >0 to compute geomean
│ master.txt │ fix-missing-params.txt │
│ allocs/op │ allocs/op vs base │
OneRoute-10 0.000 ± 0% 0.000 ± 0% ~ (p=1.000 n=10) ¹
RecoveryMiddleware-10 0.000 ± 0% 0.000 ± 0% ~ (p=1.000 n=10) ¹
LoggerMiddleware-10 8.000 ± 0% 8.000 ± 0% ~ (p=1.000 n=10) ¹
ManyHandlers-10 8.000 ± 0% 8.000 ± 0% ~ (p=1.000 n=10) ¹
5Params-10 0.000 ± 0% 0.000 ± 0% ~ (p=1.000 n=10) ¹
OneRouteJSON-10 3.000 ± 0% 3.000 ± 0% ~ (p=1.000 n=10) ¹
OneRouteHTML-10 9.000 ± 0% 9.000 ± 0% ~ (p=1.000 n=10) ¹
OneRouteSet-10 2.000 ± 0% 2.000 ± 0% ~ (p=1.000 n=10) ¹
OneRouteString-10 1.000 ± 0% 1.000 ± 0% ~ (p=1.000 n=10) ¹
ManyRoutesFist-10 0.000 ± 0% 0.000 ± 0% ~ (p=1.000 n=10) ¹
ManyRoutesLast-10 0.000 ± 0% 0.000 ± 0% ~ (p=1.000 n=10) ¹
404-10 0.000 ± 0% 0.000 ± 0% ~ (p=1.000 n=10) ¹
404Many-10 0.000 ± 0% 0.000 ± 0% ~ (p=1.000 n=10) ¹
Github-10 18.00 ± 0% 18.00 ± 0% ~ (p=1.000 n=10) ¹
PathClean-10 17.00 ± 0% 17.00 ± 0% ~ (p=1.000 n=10) ¹
PathCleanLong-10 4.683k ± 0% 4.683k ± 0% ~ (p=1.000 n=10) ¹
geomean ² +0.00% ²
¹ all samples are equal
² summaries must be >0 to compute geomean