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 Context.Next() - recheck len of handlers every iteration #1745

Merged
merged 4 commits into from Jan 18, 2019

Conversation

vkd
Copy link
Contributor

@vkd vkd commented Jan 17, 2019

If gin sometimes reset Context.index and .handlers then needs to recheck len of handlers every iteration on func Context.Next():

gin/context.go

Lines 103 to 111 in 29a145c

// Next should be used only inside middleware.
// It executes the pending handlers in the chain inside the calling handler.
// See example in GitHub.
func (c *Context) Next() {
c.index++
for s := int8(len(c.handlers)); c.index < s; c.index++ {
c.handlers[c.index](c)
}
}


Fix #1678, because gin change c.handlers when error on open file.

gin/routergroup.go

Lines 195 to 202 in 29a145c

// Check if file exists and/or if we have permission to access it
if _, err := fs.Open(file); err != nil {
c.Writer.WriteHeader(http.StatusNotFound)
c.handlers = group.engine.allNoRoute
// Reset index
c.index = -1
return
}


Fix #1744, because gin reset c.handlers slice on HandleContext() func.

gin/gin.go

Lines 354 to 360 in 29a145c

// HandleContext re-enter a context that has been rewritten.
// This can be done by setting c.Request.URL.Path to your new target.
// Disclaimer: You can loop yourself to death with this, use wisely.
func (engine *Engine) HandleContext(c *Context) {
c.reset()
engine.handleHTTPRequest(c)
}

gin/context.go

Lines 67 to 75 in 29a145c

func (c *Context) reset() {
c.Writer = &c.writermem
c.Params = c.Params[0:0]
c.handlers = nil
c.index = -1
c.Keys = nil
c.Errors = c.Errors[0:0]
c.Accepted = nil
}

@codecov
Copy link

codecov bot commented Jan 17, 2019

Codecov Report

Merging #1745 into master will increase coverage by <.01%.
The diff coverage is 100%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #1745      +/-   ##
==========================================
+ Coverage   98.48%   98.48%   +<.01%     
==========================================
  Files          41       41              
  Lines        2041     2042       +1     
==========================================
+ Hits         2010     2011       +1     
  Misses         19       19              
  Partials       12       12
Impacted Files Coverage Δ
context.go 98.3% <100%> (ø) ⬆️

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 b056a34...676c7b2. Read the comment docs.

@thinkerou
Copy link
Member

thanks @vkd can you add test case?

context.go Outdated
@@ -105,7 +105,7 @@ func (c *Context) Handler() HandlerFunc {
// See example in GitHub.
func (c *Context) Next() {
c.index++
for s := int8(len(c.handlers)); c.index < s; c.index++ {
for ; c.index < int8(len(c.handlers)); c.index++ {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

use while style?

TestEngineHandleContext
TestContextResetInHandler
TestRouterStaticFSFileNotFound
Copy link
Member

@thinkerou thinkerou left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, thanks!

@appleboy appleboy added this to the 1.4 milestone Jan 18, 2019
@thinkerou thinkerou merged commit 4867ff9 into gin-gonic:master Jan 18, 2019
@vkd vkd deleted the fix_context_next branch March 26, 2019 18:27
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants