fix Context.Next() - recheck len of handlers every iteration #1745

vkd · 2019-01-17T08:48:38Z

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 · 2019-01-17T08:55:49Z

Codecov Report

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

@@            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 · 2019-01-17T12:18:01Z

thanks @vkd can you add test case?

thinkerou · 2019-01-17T12:19:12Z

context.go

 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++ {


use while style?

TestEngineHandleContext TestContextResetInHandler TestRouterStaticFSFileNotFound

thinkerou

LGTM, thanks!

fix Context.Next() - recheck len of handlers every iteration

5c26a13

thinkerou reviewed Jan 17, 2019

View reviewed changes

thinkerou requested review from appleboy, javierprovecho and thinkerou January 17, 2019 13:53

vkd added 2 commits January 17, 2019 18:11

add tests when Context.reset() can be called inside of handler

53263e4

TestEngineHandleContext TestContextResetInHandler TestRouterStaticFSFileNotFound

Context.Next() - format to while style

7c5e99b

thinkerou approved these changes Jan 18, 2019

View reviewed changes

appleboy approved these changes Jan 18, 2019

View reviewed changes

appleboy added the enhancement label Jan 18, 2019

appleboy added this to the 1.4 milestone Jan 18, 2019

Merge branch 'master' into fix_context_next

676c7b2

thinkerou merged commit 4867ff9 into gin-gonic:master Jan 18, 2019

vkd deleted the fix_context_next branch March 26, 2019 18:27

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

fix Context.Next() - recheck len of handlers every iteration #1745

fix Context.Next() - recheck len of handlers every iteration #1745

vkd commented Jan 17, 2019

Uh oh!

codecov bot commented Jan 17, 2019 •

edited

Loading

Uh oh!

thinkerou commented Jan 17, 2019

Uh oh!

thinkerou Jan 17, 2019

Uh oh!

thinkerou left a comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

	// 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)
	}
	}

	// 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
	}

	// 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)
	}

	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
	}

fix Context.Next() - recheck len of handlers every iteration #1745

fix Context.Next() - recheck len of handlers every iteration #1745

Conversation

vkd commented Jan 17, 2019

Uh oh!

codecov bot commented Jan 17, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

thinkerou commented Jan 17, 2019

Uh oh!

thinkerou Jan 17, 2019

Choose a reason for hiding this comment

Uh oh!

thinkerou left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

codecov bot commented Jan 17, 2019 •

edited

Loading