fix(tree): correctly expand the capacity of params #3502

georgijd-form3 · 2023-02-16T13:30:23Z

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 #2690 (comment)). 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 · 2023-02-18T06:54:14Z

Codecov Report

All modified and coverable lines are covered by tests ✅

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%)`	⬆️
go-1.18	`99.11% <100.00%> (+<0.01%)`	⬆️
go-1.19	`99.21% <100.00%> (+<0.01%)`	⬆️
go-1.20	`99.21% <100.00%> (+<0.01%)`	⬆️
macos-latest	`99.21% <100.00%> (+0.09%)`	⬆️
ubuntu-latest	`99.21% <100.00%> (+<0.01%)`	⬆️

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.

georgijd-form3 · 2023-02-21T09:51:46Z

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?

cyryl-form3 · 2023-10-10T09:53:27Z

Hello dear folks, hope you're having a great day !
Is this PR something you would be willing to consider including/merging ? :)

tomas-deml-form3 · 2023-12-06T13:37:08Z

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.

appleboy · 2023-12-06T15:33:42Z

@georgijd-form3 Can you add a performance report?

georgijd-form3 · 2023-12-06T21:52:51Z

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

appleboy · 2023-12-07T00:39:21Z

@georgijd-form3 Thanks for your PR. :)

fix(tree): correctly expand the capacity of params

f949602

appleboy added the bug label Feb 24, 2023

appleboy added this to the v1.10 milestone Feb 24, 2023

a-h mentioned this pull request Nov 16, 2023

golang.org/x/net < 0.17.0 security vulnerability #3755

Closed

Merge branch 'gin-gonic:master' into fix-missing-params

bb52068

appleboy approved these changes Dec 7, 2023

View reviewed changes

appleboy merged commit 386d244 into gin-gonic:master Dec 7, 2023
29 checks passed

appleboy mentioned this pull request Dec 7, 2023

Using gin.CreateTestContext, and then engine.HandleContext causes panic if params are not filled in #2690

Closed

georgijd-form3 deleted the fix-missing-params branch December 7, 2023 09:15

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix(tree): correctly expand the capacity of params #3502

fix(tree): correctly expand the capacity of params #3502

georgijd-form3 commented Feb 16, 2023

codecov bot commented Feb 18, 2023 •

edited

Loading

georgijd-form3 commented Feb 21, 2023

cyryl-form3 commented Oct 10, 2023

tomas-deml-form3 commented Dec 6, 2023

appleboy commented Dec 6, 2023

georgijd-form3 commented Dec 6, 2023 •

edited

Loading

appleboy commented Dec 7, 2023

fix(tree): correctly expand the capacity of params #3502

fix(tree): correctly expand the capacity of params #3502

Conversation

georgijd-form3 commented Feb 16, 2023

codecov bot commented Feb 18, 2023 • edited Loading

Codecov Report

georgijd-form3 commented Feb 21, 2023

cyryl-form3 commented Oct 10, 2023

tomas-deml-form3 commented Dec 6, 2023

appleboy commented Dec 6, 2023

georgijd-form3 commented Dec 6, 2023 • edited Loading

appleboy commented Dec 7, 2023

codecov bot commented Feb 18, 2023 •

edited

Loading

georgijd-form3 commented Dec 6, 2023 •

edited

Loading