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 resp body close #1195

Merged
merged 2 commits into from
Jul 8, 2022
Merged

fix resp body close #1195

merged 2 commits into from
Jul 8, 2022

Conversation

jiuker
Copy link
Contributor

@jiuker jiuker commented Jul 5, 2022

resp.Body is not closed. It may cause memory leaks.

Copy link
Member

@harshavardhana harshavardhana left a comment

Choose a reason for hiding this comment

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

Thanks for the PR however please make the following change instead

diff --git a/pkg/apis/minio.min.io/v1/helper.go b/pkg/apis/minio.min.io/v1/helper.go
index 83301220..2f5de612 100644
--- a/pkg/apis/minio.min.io/v1/helper.go
+++ b/pkg/apis/minio.min.io/v1/helper.go
@@ -465,53 +465,6 @@ func (t *Tenant) MinIOServerEndpoint() string {
        return u.String()
 }
 
-// MinIOHealthCheck check MinIO cluster health
-func (t *Tenant) MinIOHealthCheck() bool {
-       // Keep TLS config.
-       tlsConfig := &tls.Config{
-               // Can't use SSLv3 because of POODLE and BEAST
-               // Can't use TLSv1.0 because of POODLE and BEAST using CBC cipher
-               // Can't use TLSv1.1 because of RC4 cipher usage
-               MinVersion:         tls.VersionTLS12,
-               InsecureSkipVerify: true, // FIXME: use trusted CA
-       }
-
-       req, err := http.NewRequest(http.MethodGet, t.MinIOServerEndpoint()+"/minio/health/cluster", nil)
-       if err != nil {
-               return false
-       }
-
-       httpClient := &http.Client{
-               Transport:
-               // For more details about various values used here refer
-               // https://golang.org/pkg/net/http/#Transport documentation
-               &http.Transport{
-                       Proxy: http.ProxyFromEnvironment,
-                       DialContext: (&net.Dialer{
-                               Timeout:   2 * time.Second,
-                               KeepAlive: 10 * time.Second,
-                       }).DialContext,
-                       ResponseHeaderTimeout: 5 * time.Second,
-                       TLSHandshakeTimeout:   5 * time.Second,
-                       ExpectContinueTimeout: 5 * time.Second,
-                       TLSClientConfig:       tlsConfig,
-                       // Go net/http automatically unzip if content-type is
-                       // Go net/http automatically unzip if content-type is
-                       // gzip disable this feature, as we are always interested
-                       // in raw stream.
-                       DisableCompression: true,
-               },
-       }
-       defer httpClient.CloseIdleConnections()
-
-       resp, err := httpClient.Do(req)
-       if err != nil {
-               return false
-       }
-
-       return resp.StatusCode == http.StatusOK
-}
-
 // NewMinIOAdmin initializes a new madmin.Client for operator interaction
 func (t *Tenant) NewMinIOAdmin(minioSecret map[string][]byte) (*madmin.AdminClient, error) {
        host := t.MinIOServerHostAddress()
diff --git a/pkg/apis/minio.min.io/v2/helper.go b/pkg/apis/minio.min.io/v2/helper.go
index d5061054..3ec14a60 100644
--- a/pkg/apis/minio.min.io/v2/helper.go
+++ b/pkg/apis/minio.min.io/v2/helper.go
@@ -648,6 +648,26 @@ func (t *Tenant) MinIOServerEndpoint() string {
        return u.String()
 }
 
+// drainBody close non nil response with any response Body.
+// convenient wrapper to drain any remaining data on response body.
+//
+// Subsequently this allows golang http RoundTripper
+// to re-use the same connection for future requests.
+func drainBody(respBody io.ReadCloser) {
+       // Callers should close resp.Body when done reading from it.
+       // If resp.Body is not closed, the Client's underlying RoundTripper
+       // (typically Transport) may not be able to re-use a persistent TCP
+       // connection to the server for a subsequent "keep-alive" request.
+       if respBody != nil {
+               // Drain any remaining Body and then close the connection.
+               // Without this closing connection would disallow re-using
+               // the same connection for future uses.
+               //  - http://stackoverflow.com/a/17961593/4465767
+               defer respBody.Close()
+               io.Copy(ioutil.Discard, respBody)
+       }
+}
+
 // MinIOHealthCheck check MinIO cluster health
 func (t *Tenant) MinIOHealthCheck(tr *http.Transport) bool {
        if tr.TLSClientConfig != nil {
@@ -668,6 +688,8 @@ func (t *Tenant) MinIOHealthCheck(tr *http.Transport) bool {
                return false
        }
 
+       drainBody(resp.Body)
+
        return resp.StatusCode == http.StatusOK
 }

Copy link
Member

@vadmeste vadmeste left a comment

Choose a reason for hiding this comment

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

LGTM

@harshavardhana harshavardhana merged commit 944c7c3 into minio:master Jul 8, 2022
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

Successfully merging this pull request may close these issues.

None yet

3 participants