Skip to content
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

Fix the path params slice out of range with the wrong latest node #2820

Closed
wants to merge 2 commits into from

Conversation

git-hulk
Copy link

@git-hulk git-hulk commented Aug 13, 2021

getValue didn't update the latest node after walking through
the path and may regard the static route as the param type,
but the params cap was preallocated and cause out of range.

For Example:

the route was "/:a/:b/:c/d" and we send the request path "/a/b/c/d1",
the params cap was 3 but the d would also be regarded as param then
would be out of range.

getValue didn't update the latest node after walking through
the path and may regard the static route as the param type,
but the params cap was prealloc and cause out of range.

For Example:

the route was "/a/b/:c/d" and we send the request path "/a/b/c/d1",
the params cap was 1 but the `d` would also be regarded as param then
would be out of range.
@maximerety
Copy link

Hi @git-hulk,

We encountered the same issue (while trying to upgrade to gin 1.7.3) but I wasn't sure how to tackle it.

So thank you for the fix and the explanations!

I guess you could add a test case to gin_integration_test.go too, since it seems to mirror all tests from tree_test.go, e.g.:

diff --git a/gin_integration_test.go b/gin_integration_test.go
index a4149de..cb05f3a 100644
--- a/gin_integration_test.go
+++ b/gin_integration_test.go
@@ -463,4 +463,5 @@ func TestTreeRunDynamicRouting(t *testing.T) {
        testRequest(t, ts.URL+"/a/dd", "404 Not Found")
        testRequest(t, ts.URL+"/addr/dd/aa", "404 Not Found")
        testRequest(t, ts.URL+"/something/secondthing/121", "404 Not Found")
+       testRequest(t, ts.URL+"/cc/dd/ee/ff/gg/hh1", "404 Not Found")
 }

I hope this fix will be merged/released soon so that we'll be able to upgrade gin beyond 1.7.2.

@codecov
Copy link

codecov bot commented Aug 24, 2021

Codecov Report

Merging #2820 (a1f62a5) into master (b463b1c) will increase coverage by 0.04%.
The diff coverage is 100.00%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #2820      +/-   ##
==========================================
+ Coverage   98.71%   98.75%   +0.04%     
==========================================
  Files          41       41              
  Lines        2100     3055     +955     
==========================================
+ Hits         2073     3017     +944     
- Misses         15       26      +11     
  Partials       12       12              
Flag Coverage Δ
go-1.13 98.75% <100.00%> (?)
go-1.14 98.59% <100.00%> (?)
go-1.15 98.59% <100.00%> (?)
go-1.16 98.59% <100.00%> (?)
macos-latest 98.75% <100.00%> (?)
nomsgpack 98.73% <100.00%> (?)
ubuntu-latest 98.75% <100.00%> (?)

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

Impacted Files Coverage Δ
tree.go 100.00% <100.00%> (ø)
render/json.go 83.14% <0.00%> (-2.15%) ⬇️
gin.go 99.09% <0.00%> (-0.09%) ⬇️
utils.go 96.80% <0.00%> (-0.02%) ⬇️
fs.go 100.00% <0.00%> (ø)
auth.go 100.00% <0.00%> (ø)
mode.go 100.00% <0.00%> (ø)
path.go 100.00% <0.00%> (ø)
debug.go 100.00% <0.00%> (ø)
... and 32 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update b463b1c...a1f62a5. Read the comment docs.

@git-hulk git-hulk changed the title Fix params would out of range with the wrong latest node Fix the path params slice out of range with the wrong latest node Aug 25, 2021
@git-hulk
Copy link
Author

@appleboy anyone can have a look at this PR?

@appleboy
Copy link
Member

@git-hulk Thanks for your effort. Can you also provide the benchmark result like this comment: #2767 (comment)

Hi @qm012 @rw-access, please also take a look if you have free time.

@appleboy appleboy added the bug label Aug 25, 2021
@appleboy appleboy added this to the v1.7.5 milestone Aug 25, 2021
@qm012
Copy link
Contributor

qm012 commented Aug 26, 2021

@git-hulk Thanks for your effort. Can you also provide the benchmark result like this comment: #2767 (comment)

Hi @qm012 @rw-access, please also take a look if you have free time.

Thanks I have been working late these two days. I will do a test on the weekend and give you feedback:eyes:

@qm012
Copy link
Contributor

qm012 commented Aug 29, 2021

hi @appleboy lgtm. thanks for @git-hulk

@appleboy
Copy link
Member

@git-hulk Post the benchmark result?

@git-hulk
Copy link
Author

@appleboy ok, I’ll post the benchmark result later, sorry for missing your previous comment.

@git-hulk
Copy link
Author

git-hulk commented Aug 29, 2021

The benchmarks:
master https://app.travis-ci.com/github/git-hulk/go-http-routing-benchmark/builds/236444329 - 632s
this PR https://app.travis-ci.com/github/git-hulk/go-http-routing-benchmark/builds/236442891 - 636s

The benchstat report:

BenchmarkGin_Param        	58612060	        61.18 ns/op	       0 B/op	       0 allocs/op
BenchmarkGin_Param5       	31930453	       117.8 ns/op	       0 B/op	       0 allocs/op
BenchmarkGin_Param20      	11869882	       302.4 ns/op	       0 B/op	       0 allocs/op
BenchmarkGin_ParamWrite   	31239873	       117.7 ns/op	       0 B/op	       0 allocs/op
BenchmarkGin_GithubStatic 	44474041	        80.83 ns/op	       0 B/op	       0 allocs/op
BenchmarkGin_GithubParam  	27028000	       135.0 ns/op	       0 B/op	       0 allocs/op
BenchmarkGin_GithubParam  	27133978	       132.6 ns/op	       0 B/op	       0 allocs/op
BenchmarkGin_GithubAll    	  135966	     26346 ns/op	       0 B/op	       0 allocs/op
BenchmarkGin_GPlusStatic  	59091340	        60.47 ns/op	       0 B/op	       0 allocs/op
BenchmarkGin_GPlusParam   	39117842	        92.22 ns/op	       0 B/op	       0 allocs/op
BenchmarkGin_GPlus2Params 	31236037	       115.7 ns/op	       0 B/op	       0 allocs/op
BenchmarkGin_GPlusAll     	 3110154	      1176 ns/op	       0 B/op	       0 allocs/op
BenchmarkGin_ParseStatic  	61985156	        59.47 ns/op	       0 B/op	       0 allocs/op
BenchmarkGin_ParseParam   	55725441	        65.23 ns/op	       0 B/op	       0 allocs/op
BenchmarkGin_Parse2Params 	44601469	        80.90 ns/op	       0 B/op	       0 allocs/op
BenchmarkGin_ParseAll     	 1842114	      2007 ns/op	       0 B/op	       0 allocs/op
BenchmarkGin_StaticAll    	  191962	     18623 ns/op	       0 B/op	       0 allocs/op

@appleboy
Copy link
Member

appleboy commented Aug 29, 2021

benchmark result between master and PR branch:

$ benchstat a.txt b.txt 
name              old time/op    new time/op    delta
Gin_Param           57.9ns ± 1%    60.5ns ± 5%   +4.46%  (p=0.000 n=9+10)
Gin_Param5           102ns ± 3%     115ns ± 3%  +13.64%  (p=0.000 n=10+10)
Gin_Param20          254ns ± 2%     304ns ± 1%  +19.90%  (p=0.000 n=10+10)
Gin_ParamWrite       117ns ± 2%     116ns ± 4%     ~     (p=0.150 n=9+10)
Gin_GithubStatic    72.0ns ± 0%    81.0ns ± 1%  +12.48%  (p=0.000 n=10+10)
Gin_GithubParam      122ns ± 1%     133ns ± 0%   +8.55%  (p=0.000 n=10+9)
Gin_GithubAll       25.3µs ± 0%    26.3µs ± 0%   +4.14%  (p=0.000 n=9+10)
Gin_GPlusStatic     57.9ns ± 0%    60.7ns ± 0%   +4.80%  (p=0.000 n=10+10)
Gin_GPlusParam      76.4ns ± 0%    91.9ns ± 1%  +20.31%  (p=0.000 n=10+10)
Gin_GPlus2Params    95.7ns ± 0%   116.1ns ± 2%  +21.30%  (p=0.000 n=9+9)
Gin_GPlusAll        1.07µs ± 1%    1.17µs ± 1%   +9.03%  (p=0.000 n=9+10)
Gin_ParseStatic     57.5ns ± 0%    58.0ns ± 1%   +0.87%  (p=0.000 n=10+8)
Gin_ParseParam      63.4ns ± 0%    64.9ns ± 1%   +2.36%  (p=0.000 n=10+10)
Gin_Parse2Params    76.2ns ± 0%    80.9ns ± 0%   +6.11%  (p=0.000 n=10+10)
Gin_ParseAll        1.82µs ± 1%    1.98µs ± 1%   +8.83%  (p=0.000 n=10+10)
Gin_StaticAll       18.3µs ± 1%    18.6µs ± 0%   +1.81%  (p=0.000 n=10+10)

@git-hulk
Copy link
Author

@appleboy so should I worry about the performance degrade?

@appleboy
Copy link
Member

@git-hulk Yes, we need to care about the performance of the router. @qm012 Do you have any comments about the benchmark result?

@qm012
Copy link
Contributor

qm012 commented Aug 30, 2021

@git-hulk Yes, we need to care about the performance of the router. @qm012 Do you have any comments about the benchmark result?

hi @appleboy Migration is done because it affects route matching performance and makes multiple circular calls.If fixing this bug results in such a large performance difference, I think it is not worth the cost, so we can avoid this bug in route definition for the time being until we have a better solution.

Test report after migration #2796 (comment)

Before the migration
image
After the migration
image

@git-hulk
Copy link
Author

git-hulk commented Aug 30, 2021

@qm012 We should NOT sacrifice the correctness to promise the performance, right?

@qm012
Copy link
Contributor

qm012 commented Aug 30, 2021

@qm012 I don't think we should NOT sacrifice the correctness to promise the performance, right?

They are compatible, so we need to talk about them.
@appleboy @thinkerou @rw-access What do you think

@qm012
Copy link
Contributor

qm012 commented Aug 30, 2021

@qm012 We should NOT sacrifice the correctness to promise the performance, right?

There are many ways to solve a problem.
Like this #2767 (comment) can fix problem but the test report is not good, we can optimize until we get a better test report #2767 (comment)

@jjba23
Copy link
Contributor

jjba23 commented Sep 27, 2021

Issue still exists, thus i am frozen at gin 1.7.2... see #2878

zhuxi0511 added a commit to zhuxi0511/gin that referenced this pull request Oct 8, 2021
@boindil
Copy link

boindil commented Oct 14, 2021

Issue still exists, thus i am frozen at gin 1.7.2... see #2878

I'll have to downgrade, too ... weird how this takes 2 months ... :(

@git-hulk
Copy link
Author

IMHO, this PR only intended to fix the bug, the performance data didn't mean this PR cause performance to degrade(I mean the old performance should be wrong since the logic has a bug), so we should fix the bug first then optimize the performance if need. HDYT @appleboy

@appleboy
Copy link
Member

@git-hulk We can also take a look PR #2897

@appleboy
Copy link
Member

See solution #2897 already merged in the master branch.

@appleboy appleboy closed this Oct 23, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants