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

Is there a race condition #88

Open
sdemjanenko opened this issue Sep 20, 2024 · 1 comment
Open

Is there a race condition #88

sdemjanenko opened this issue Sep 20, 2024 · 1 comment

Comments

@sdemjanenko
Copy link

sdemjanenko commented Sep 20, 2024

I was looking at the func (g *gzipHandler) Handle(c *gin.Context) method and it appears to me that there could be a case where gz := g.gzPool.Get().(*gzip.Writer) is used after being returned to sync.Pool.

Specifically in this code

gz := g.gzPool.Get().(*gzip.Writer)
defer g.gzPool.Put(gz)
defer gz.Reset(io.Discard)
gz.Reset(c.Writer)

c.Header("Content-Encoding", "gzip")
c.Header("Vary", "Accept-Encoding")
c.Writer = &gzipWriter{c.Writer, gz}
defer func() {
	gz.Close()
	c.Header("Content-Length", fmt.Sprint(c.Writer.Size()))
}()
c.Next()

It seems like it would be safer if the code was something like

gz := g.gzPool.Get().(*gzip.Writer)
defer g.gzPool.Put(gz)

// NOTE: There may be a better way than creating a func here
// My goal is to run gz.Reset(io.Discard) before g.gzPool.Put(gz)
// I also want gz.Close() to run before g.gzPool.Put(gz)
// I am not sure if gz.Close() should run after gz.Reset(io.Discard)
// I believe it currently runs after.
func() {
	defer gz.Reset(io.Discard)
	gz.Reset(c.Writer)

	c.Header("Content-Encoding", "gzip")
	c.Header("Vary", "Accept-Encoding")
	c.Writer = &gzipWriter{c.Writer, gz}
	defer func() {
		gz.Close()
		c.Header("Content-Length", fmt.Sprint(c.Writer.Size()))
	}()
        c.Next()
}()

It is possible that I am missing something that prevents a race condition like this from happening. I would love to learn more about this.

@sdemjanenko
Copy link
Author

I think i got the defer order wrong. Its a last-in-first-out stack. I think that means the order of the defer execution is

gz.Close()
c.Header("Content-Length", fmt.Sprint(c.Writer.Size()))

gz.Reset(io.Discard)

g.gzPool.Put(gz)

Which I interpret to be correct.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

1 participant