Repository: megaease/easegress
Pull request #614 (2022-05-05 08:39:57)

sync.Pool should be passed by reference
The sync.Pool used in filter/proxy/pool.go should be passed by reference instead of by value.
use go vet command `go vet pkg/filter/proxy/*.go `, then get the warnings.

note: objects in sync.Pool that are not explicitly cleaned up before call `Put` may cause dirty data to be obtained the next time  call `Get`. I'm guessing `httpStatResultPool` might be the case, but I need experiments to prove it. EG:When the first request succeeds, `httpstat.Result`'s some properties will be set, and when the second request fails (failed to resolve the domain name, failed to establish a tcp link and etc.), some properties (NameLookup\TLSHandshake\serverDone) may use the last one. value，that may be leads to a bias in the statistics

localvar: Thanks, I think this PR fixes an important bug.

Patch:
diff --git a/pkg/filter/proxy/pool.go b/pkg/filter/proxy/pool.go
index fdfa444e..19007724 100644
--- a/pkg/filter/proxy/pool.go
+++ b/pkg/filter/proxy/pool.go
@@ -127,13 +127,13 @@ func (p *pool) status() *PoolStatus {
 	return s
 }
 
-var requestPool = sync.Pool{
+var requestPool = &sync.Pool{
 	New: func() interface{} {
 		return &request{}
 	},
 }
 
-var httpstatResultPool = sync.Pool{
+var httpStatResultPool = &sync.Pool{
 	New: func() interface{} {
 		return &gohttpstat.Result{}
 	},
@@ -170,7 +170,7 @@ func (p *pool) handle(ctx context.HTTPContext, reqBody io.Reader, client *Client
 	}
 	addLazyTag("addr", server.URL, -1)
 
-	req, err := p.prepareRequest(ctx, server, reqBody, requestPool, httpstatResultPool)
+	req, err := p.prepareRequest(ctx, server, reqBody, requestPool, httpStatResultPool)
 	if err != nil {
 		msg := stringtool.Cat("prepare request failed: ", err.Error())
 		logger.Errorf("BUG: %s", msg)
@@ -226,9 +226,9 @@ func (p *pool) prepareRequest(
 	ctx context.HTTPContext,
 	server *Server,
 	reqBody io.Reader,
-	requestPool sync.Pool,
-	httpstatResultPool sync.Pool) (req *request, err error) {
-	return p.newRequest(ctx, server, reqBody, requestPool, httpstatResultPool)
+	requestPool *sync.Pool,
+	httpStatResultPool *sync.Pool) (req *request, err error) {
+	return p.newRequest(ctx, server, reqBody, requestPool, httpStatResultPool)
 }
 
 func (p *pool) doRequest(ctx context.HTTPContext, req *request, client *Client) (*http.Response, error) {
@@ -246,7 +246,7 @@ func (p *pool) doRequest(ctx context.HTTPContext, req *request, client *Client)
 	return resp, nil
 }
 
-var httpstatMetricPool = sync.Pool{
+var httpStatMetricPool = &sync.Pool{
 	New: func() interface{} {
 		return &httpstat.Metric{}
 	},
@@ -276,7 +276,7 @@ func (p *pool) statRequestResponse(ctx context.HTTPContext,
 			return stringtool.Cat(p.tagPrefix, "#duration: ", duration.String())
 		})
 		// use recycled object
-		metric := httpstatMetricPool.Get().(*httpstat.Metric)
+		metric := httpStatMetricPool.Get().(*httpstat.Metric)
 		metric.StatusCode = resp.StatusCode
 		metric.Duration = duration
 		metric.ReqSize = ctx.Request().Size()
@@ -287,8 +287,8 @@ func (p *pool) statRequestResponse(ctx context.HTTPContext,
 		}
 		p.httpStat.Stat(metric)
 		// recycle struct instances
-		httpstatMetricPool.Put(metric)
-		httpstatResultPool.Put(req.statResult)
+		httpStatMetricPool.Put(metric)
+		httpStatResultPool.Put(req.statResult)
 		requestPool.Put(req)
 	})
 
diff --git a/pkg/filter/proxy/request.go b/pkg/filter/proxy/request.go
index 24e13548..bbf25c94 100644
--- a/pkg/filter/proxy/request.go
+++ b/pkg/filter/proxy/request.go
@@ -51,9 +51,9 @@ func (p *pool) newRequest(
 	ctx context.HTTPContext,
 	server *Server,
 	reqBody io.Reader,
-	requestPool sync.Pool,
-	httpstatResultPool sync.Pool) (*request, error) {
-	statResult := httpstatResultPool.Get().(*httpstat.Result)
+	requestPool *sync.Pool,
+	httpStatResultPool *sync.Pool) (*request, error) {
+	statResult := httpStatResultPool.Get().(*httpstat.Result)
 	req := requestPool.Get().(*request)
 	req.createTime = fasttime.Now()
 	req.server = server


Test patch:
diff --git a/pkg/filter/proxy/request_test.go b/pkg/filter/proxy/request_test.go
index d2623e6c..f1e398d8 100644
--- a/pkg/filter/proxy/request_test.go
+++ b/pkg/filter/proxy/request_test.go
@@ -57,7 +57,7 @@ func TestRequest(t *testing.T) {
 
 	p := pool{}
 	sr := strings.NewReader("this is the raw body")
-	req, _ := p.newRequest(ctx, &server, sr, requestPool, httpstatResultPool)
+	req, _ := p.newRequest(ctx, &server, sr, requestPool, httpStatResultPool)
 	defer requestPool.Put(req) // recycle request
 
 	req.start()
@@ -83,7 +83,7 @@ func TestRequest(t *testing.T) {
 	ctx.MockedRequest.MockedMethod = func() string {
 		return "ééé" // not tokenable, should fail
 	}
-	req, err := p.newRequest(ctx, &server, nil, requestPool, httpstatResultPool)
+	req, err := p.newRequest(ctx, &server, nil, requestPool, httpStatResultPool)
 	assert.Nil(req)
 	assert.NotNil(err)
 }
@@ -102,7 +102,7 @@ func TestResultState(t *testing.T) {
 }
 
 func TestRequestStatus(t *testing.T) {
-	statResult := httpstatResultPool.Get().(*httpstat.Result)
+	statResult := httpStatResultPool.Get().(*httpstat.Result)
 	req := requestPool.Get().(*request)
 	req.createTime = fasttime.Now()
 	req._startTime = time.Time{}
@@ -129,7 +129,7 @@ func TestRequestStatus(t *testing.T) {
 		t.Error("endtime should be _endtime after finish()")
 	}
 
-	httpstatResultPool.Put(req.statResult)
+	httpStatResultPool.Put(req.statResult)
 	requestPool.Put(req)
 }
 
@@ -153,7 +153,7 @@ func TestAddB3PropagationHeaders(t *testing.T) {
 
 	p := pool{}
 	sr := strings.NewReader("this is the raw body")
-	req, _ := p.newRequest(ctx, &server, sr, requestPool, httpstatResultPool)
+	req, _ := p.newRequest(ctx, &server, sr, requestPool, httpStatResultPool)
 	defer requestPool.Put(req) // recycle request
 
 	header := req.std.Header