app/vmauth: remove readTrackingBody pool (VictoriaMetrics#8104)

Sync.Pool for readTrackingBody was added in order to reduce potential
load on garbage collector. But golang net/http standard library does not
allow to reuse request body, since it closes body asynchronously after
return. Related issue: golang/go#51907

This commit removes sync.Pool in order to fix potential panic and data
race at requests processing.

 Affected releases:
- all releases after v1.97.7

 Related issue:
VictoriaMetrics#8051

Signed-off-by: f41gh7 <[email protected]>
Co-authored-by: Roman Khavronenko <[email protected]>

Author: f41gh7

app/vmauth/main.go

@@ -222,8 +222,7 @@ func processRequest(w http.ResponseWriter, r *http.Request, ui *UserInfo) {
 		isDefault = true
 	}
 
-	rtb := getReadTrackingBody(r.Body, maxRequestBodySizeToRetry.IntN())
-	defer putReadTrackingBody(rtb)
+	rtb := newReadTrackingBody(r.Body, maxRequestBodySizeToRetry.IntN())
 	r.Body = rtb
 
 	maxAttempts := up.getBackendsCount()
@@ -559,22 +558,11 @@ type readTrackingBody struct {
 	bufComplete bool
 }
 
-func (rtb *readTrackingBody) reset() {
-	rtb.maxBodySize = 0
-	rtb.r = nil
-	rtb.buf = rtb.buf[:0]
-	rtb.readBuf = nil
-	rtb.cannotRetry = false
-	rtb.bufComplete = false
-}
-
-func getReadTrackingBody(r io.ReadCloser, maxBodySize int) *readTrackingBody {
-	v := readTrackingBodyPool.Get()
-	if v == nil {
-		v = &readTrackingBody{}
-	}
-	rtb := v.(*readTrackingBody)
-
+func newReadTrackingBody(r io.ReadCloser, maxBodySize int) *readTrackingBody {
+	// do not use sync.Pool there
+	// since http.RoundTrip may still use request body after return
+	// See this issue for details https://github.com/VictoriaMetrics/VictoriaMetrics/issues/8051
+	rtb := &readTrackingBody{}
 	if maxBodySize < 0 {
 		maxBodySize = 0
 	}
@@ -597,13 +585,6 @@ func (r *zeroReader) Close() error {
 	return nil
 }
 
-func putReadTrackingBody(rtb *readTrackingBody) {
-	rtb.reset()
-	readTrackingBodyPool.Put(rtb)
-}
-
-var readTrackingBodyPool sync.Pool
-
 // Read implements io.Reader interface.
 func (rtb *readTrackingBody) Read(p []byte) (int, error) {
 	if len(rtb.readBuf) > 0 {

app/vmauth/main_test.go

@@ -545,8 +545,7 @@ func TestReadTrackingBody_RetrySuccess(t *testing.T) {
 	f := func(s string, maxBodySize int) {
 		t.Helper()
 
-		rtb := getReadTrackingBody(io.NopCloser(bytes.NewBufferString(s)), maxBodySize)
-		defer putReadTrackingBody(rtb)
+		rtb := newReadTrackingBody(io.NopCloser(bytes.NewBufferString(s)), maxBodySize)
 
 		if !rtb.canRetry() {
 			t.Fatalf("canRetry() must return true before reading anything")
@@ -581,8 +580,7 @@ func TestReadTrackingBody_RetrySuccessPartialRead(t *testing.T) {
 		t.Helper()
 
 		// Check the case with partial read
-		rtb := getReadTrackingBody(io.NopCloser(bytes.NewBufferString(s)), maxBodySize)
-		defer putReadTrackingBody(rtb)
+		rtb := newReadTrackingBody(io.NopCloser(bytes.NewBufferString(s)), maxBodySize)
 
 		for i := 0; i < len(s); i++ {
 			buf := make([]byte, i)
@@ -631,8 +629,7 @@ func TestReadTrackingBody_RetryFailureTooBigBody(t *testing.T) {
 	f := func(s string, maxBodySize int) {
 		t.Helper()
 
-		rtb := getReadTrackingBody(io.NopCloser(bytes.NewBufferString(s)), maxBodySize)
-		defer putReadTrackingBody(rtb)
+		rtb := newReadTrackingBody(io.NopCloser(bytes.NewBufferString(s)), maxBodySize)
 
 		if !rtb.canRetry() {
 			t.Fatalf("canRetry() must return true before reading anything")
@@ -681,8 +678,7 @@ func TestReadTrackingBody_RetryFailureZeroOrNegativeMaxBodySize(t *testing.T) {
 	f := func(s string, maxBodySize int) {
 		t.Helper()
 
-		rtb := getReadTrackingBody(io.NopCloser(bytes.NewBufferString(s)), maxBodySize)
-		defer putReadTrackingBody(rtb)
+		rtb := newReadTrackingBody(io.NopCloser(bytes.NewBufferString(s)), maxBodySize)
 
 		if !rtb.canRetry() {
 			t.Fatalf("canRetry() must return true before reading anything")

docs/changelog/CHANGELOG.md

@@ -25,6 +25,7 @@ See also [LTS releases](https://docs.victoriametrics.com/lts-releases/).
 
 * BUGFIX: [vmsingle](https://docs.victoriametrics.com/single-server-victoriametrics/), `vminsert` in [VictoriaMetrics cluster](https://docs.victoriametrics.com/cluster-victoriametrics/) and [vmagent](https://docs.victoriametrics.com/vmagent/): allow ingesting histograms with missing `_sum` metric via [OpenTelemetry ingestion protocol](https://docs.victoriametrics.com/#sending-data-via-opentelemetry) in the same way as Prometheus does.
 * BUGFIX: all VictoriaMetrics [enterprise](https://docs.victoriametrics.com/enterprise/) components: properly trim whitespaces at the end of license provided via `-license` and `-licenseFile` command-line flags. Previously, the trailing whitespaces could cause the license verification to fail.
+* BUGFIX: [vmauth](https://docs.victoriametrics.com/vmauth/): fix possible runtime panic during requests processing under heavy load. See [this issue](https://github.com/VictoriaMetrics/VictoriaMetrics/issues/8051) for details.
 
 ## [v1.109.1](https://github.com/VictoriaMetrics/VictoriaMetrics/releases/tag/v1.109.1)