Skip to content

fix: Fix the problem that some request connections are not released #8329

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.

Sign up for GitHub

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

Merged
f2c-ci-robot merged 1 commit into from
Apr 7, 2025
Merged

fix: Fix the problem that some request connections are not released #8329

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 2 additions & 0 deletions agent/utils/cloud_storage/client/ali.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -403,6 +403,7 @@ func (a aliClient) uploadPart(uri string, reader io.Reader) error {
return err
}
client := &http.Client{}
defer client.CloseIdleConnections()
response, err := client.Do(req)
if err != nil {
return err
Expand All @@ -428,6 +429,7 @@ func (a aliClient) handleDownload(uri string, target string) error {
req.Header.Add("origin", "https://www.aliyundrive.com")
req.Header.Add("referer", "https://www.aliyundrive.com/")
client := &http.Client{}
defer client.CloseIdleConnections()
response, err := client.Do(req)
if err != nil {
return err
Copy link
Copy Markdown
Member

@wanghe-fit2cloud wanghe-fit2cloud Apr 7, 2025

Choose a reason for hiding this comment

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

The code appears to include several areas for improvement, primarily related to resource management and best practices:

  1. Resource Management:

    • The client is created only once at the beginning of the functions and should be retained throughout their execution scope rather than being re-created on each request. This will help manage resources better.

  2. Closing Idle Connections:

    • It's important to close idle connections immediately after use to free up system resources. However, in this case, it seems redundant because both functions close the client using defer client.CloseIdleConnections(). This can be removed as they are already cleaning up properly.

  3. Error Handling:

    • Ensure that the handleDownload function checks if response.Body is not nil before closing it to avoid errors when attempting to read from an unexpected response body closure.
    defer response.Body.Close() // Ensure Body is closed to release resources

Here's a revised version of the code with these improvements:

func (a aliClient) uploadPart(uri string, reader io.Reader) error {
    client := &http.Client{}
    defer func() { _ = client.CloseIdleConnections() }()

    req, err := http.NewRequestWithContext(context.Background(), "PUT", uri, reader)
    if err != nil {
        return err
    }

    resp, err := client.Do(req)
    if err != nil {
        return err
    }
    defer func() { _ = resp.Body.Close() }()
}

func (a aliClient) handleDownload(uri string, target string) error {
    client := &http.Client{Transport: make(http.Transport)} // Use non-default transport for better control over connection pooling
    defer func() { _ = client.CloseIdleConnections() }()
    
    req, err := http.NewRequestWithContext(context.Background(), "GET", uri, nil)
    req.Header.Set("Origin", "https://www.aliyundrive.com")
    req.Header.Set("Referer", "https://www.aliyundrive.com/")

    resp, err := client.Do(req)
    if err != nil {
        return err
    }
    defer func() { _ = resp.Body.Close()}()

    if resp.StatusCode < 200 || resp.StatusCode >= 300 {
        return fmt.Errorf("unexpected status code: %d", resp.StatusCode)
    }

    out, err := os.OpenFile(target, os.O_WRONLY|os.O_CREATE|os.O_TRUNC, 0644)
    if err != nil {
        return err
    }
    defer func() { _ = out.Close() }()

    _, err = io.Copy(out, resp.Body)
    if err != nil {
        return err
    }

    return nil
}

Key Changes:

  • Reuse Client: The HTTP client (client) is reused across multiple requests within each function to improve efficiency.
  • Connection Pooling: The default HTTP client uses a connection pool, so explicitly setting a custom transport might not be necessary but good practice.
  • Cleanup: Added error handling when closing resp.Body.
  • Status Code Check: Added logic to check for unexpected HTTP status codes in the download function.

Expand Down
1 change: 1 addition & 0 deletions agent/utils/cloud_storage/client/google_drive.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -226,6 +226,7 @@ func (g *googleDriveClient) handleDownload(urlItem string, target string) error
Proxy: http.ProxyURL(proxyURL),
},
}
defer client.CloseIdleConnections()
response, err := client.Do(req)
if err != nil {
return err
Copy link
Copy Markdown
Member

@wanghe-fit2cloud wanghe-fit2cloud Apr 7, 2025

Choose a reason for hiding this comment

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

The defer client.CloseIdleConnections() line is correct and recommended to ensure that idle connections are closed properly after all requests have been processed.

There are no other issues or optimizations in this code snippet.

Expand Down
1 change: 1 addition & 0 deletions agent/utils/cloud_storage/client/onedrive.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -213,6 +213,7 @@ func RefreshToken(grantType string, tokenType string, varMap map[string]interfac
}
data.Set("redirect_uri", loadParamFromVars("redirect_uri", varMap))
client := &http.Client{}
defer client.CloseIdleConnections()
url := "https://login.microsoftonline.com/common/oauth2/v2.0/token"
if isCN == "true" {
url = "https://login.chinacloudapi.cn/common/oauth2/v2.0/token"
Expand Down
1 change: 1 addition & 0 deletions agent/utils/files/file_op.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -274,6 +274,7 @@ func (f FileOp) DownloadFileWithProcess(url, dst, key string, ignoreCertificate
TLSClientConfig: &tls.Config{InsecureSkipVerify: true},
}
}
defer client.CloseIdleConnections()
request, err := http.NewRequest("GET", url, nil)
if err != nil {
return nil
Expand Down
1 change: 1 addition & 0 deletions agent/utils/req_helper/core.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -30,6 +30,7 @@ func PostLocalCore(url string) error {
return err
}
client := &http.Client{}
defer client.CloseIdleConnections()
resp, err := client.Do(req)
if err != nil {
return err
Expand Down
1 change: 1 addition & 0 deletions agent/utils/req_helper/request.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -19,6 +19,7 @@ func HandleGet(url string) (*http.Response, error) {
Timeout: time.Second * 300,
}
client.Transport = loadRequestTransport()
defer client.CloseIdleConnections()

req, err := http.NewRequestWithContext(context.Background(), "GET", url, nil)
if err != nil {
Expand Down
1 change: 1 addition & 0 deletions agent/utils/ssl/acme.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -181,6 +181,7 @@ func getZeroSSLEabCredentials(email string) (*zeroSSLRes, error) {
req.Header.Set("Content-Type", "application/json")

client := &http.Client{}
defer client.CloseIdleConnections()
resp, err := client.Do(req)
if err != nil {
return nil, err
Expand Down
1 change: 1 addition & 0 deletions core/utils/cloud_storage/client/ali.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -301,6 +301,7 @@ func (a aliClient) uploadPart(uri string, reader io.Reader) error {
return err
}
client := &http.Client{}
defer client.CloseIdleConnections()
response, err := client.Do(req)
if err != nil {
return err
Expand Down
1 change: 1 addition & 0 deletions core/utils/cloud_storage/client/onedrive.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -123,6 +123,7 @@ func RefreshToken(grantType string, tokenType string, varMap map[string]interfac
}
data.Set("redirect_uri", loadParamFromVars("redirect_uri", varMap))
client := &http.Client{}
defer client.CloseIdleConnections()
url := "https://login.microsoftonline.com/common/oauth2/v2.0/token"
if isCN == "true" {
url = "https://login.chinacloudapi.cn/common/oauth2/v2.0/token"
Expand Down
6 changes: 4 additions & 2 deletions core/utils/req_helper/proxy_local/req_to_local.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,14 +5,15 @@ import (
"encoding/json"
"errors"
"fmt"
"github.com/1Panel-dev/1Panel/core/app/dto"
"github.com/1Panel-dev/1Panel/core/i18n"
"io"
"net"
"net/http"
"net/url"
"os"
"strings"

"github.com/1Panel-dev/1Panel/core/app/dto"
"github.com/1Panel-dev/1Panel/core/i18n"
)

func NewLocalClient(reqUrl, reqMethod string, body io.Reader) (interface{}, error) {
Expand All @@ -31,6 +32,7 @@ func NewLocalClient(reqUrl, reqMethod string, body io.Reader) (interface{}, erro
client := &http.Client{
Transport: transport,
}
defer client.CloseIdleConnections()
parsedURL, err := url.Parse("http://unix")
if err != nil {
return nil, fmt.Errorf("handle url Parse failed, err: %v \n", err)
Copy link
Copy Markdown
Member

@wanghe-fit2cloud wanghe-fit2cloud Apr 7, 2025

Choose a reason for hiding this comment

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

The provided code seems to be part of a function that creates an HTTP client for local operations. There are a few areas where improvements can be made:

  1. Resource Cleanup: The defer statement should call client.CloseIdleConnections() to release idle connections.

  2. URL Parsing: The URL is set to "http://unix" regardless of whether the input reqUrl specifies 'local' or not. This might lead to incorrect behavior if used with different types of requests.

  3. Error Handling: Although the current implementation returns errors as formatted strings, this could make debugging more difficult in cases with complex error messages.

Here’s an optimized version of the code:

import (
	"encoding/json"
	"errors"
	"fmt"
	"io"
	"net"
	"net/http"
	"os"
	"strings"

	dto "github.com/1Panel-dev/1Panel/core/app/dto"
	i18n "github.com/1Panel-dev/1Panel/core/i18n"
)

func NewLocalClient(reqUrl, reqMethod string, body io.Reader) (interface{}, error) {
	client := &http.Client{}

	// Adjust transport based on requirements
	transport := &http.Transport{
		DialContext: (&net.Dialer{}).DialContext,
		TLSHandshakeTimeout: 15 * time.Second,
	}

	switch reqUrl {
	case dto.LocalRequestType:
		host := getUnixSocketHost()
		if host == "" {
			return nil, i18n.GetMessage(i18n.MissingConfigKey)
		}
		transport.Dial = funcctx context.Context, _, _ string) (conn net.Conn, err error) {
			conn, err = net.DialContext(ctx, "unix", host)
			return
		}
	default:
		// Use default TCP connection for other methods
		transport.Dial = (&net.Dialer{}).Dial
	}

	client.Transport = transport

	defer client.CloseIdleConnections()

	request, err := http.NewRequestWithContext(context.Background(), reqMethod, reqUrl, body)
	if err != nil {
		return nil, fmt.Errorf("failed to create request: %w", err)
	}

	responseBody, err := client.Do(request)
	if err != nil {
		return nil, fmt.Errorf("request execution failed: %w", err)
	}
	defer responseBody.Body.Close()

	var result interface{}
	switch responseType {
	case respText:
		bodyContent, err := ioutil.ReadAll(responseBody)
		if err != nil {
			return nil, fmt.Errorf("reading response content failed: %w", err)
		}
		result = string(bodyContent)
	case respJSON:
		err := json.NewDecoder(responseBody).Decode(&result)
		if err != nil && !errors.As(err, new(UnexpectedEOF)) {
			return nil, fmt.Errorf("decoding JSON response failed: %w", err)
		}
	default:
		log.Println(fmt.Sprintf("Unsupported response type: %+v\n\n", responseType))
		return map[string]interface{}{
			"statusCode": int(responseBody.StatusCode),
		}, nil // Return a generic status code map until proper handling implemented.
		panic(errors.New("unreachable")) // Just to satisfy compile-time checks
	}

	return result, nil
}

// Utility functions omitted for brevity

Key Changes:

  1. Switch Statement: Added a switch case for dto.LocalRequestType to handle specific URLs related to Unix sockets.
  2. Default Transport Configuration: Moved the default TCP connection setup outside the switch.
  3. Connection Context: Used a background context when creating the request.
  4. Response Encoding: Decoupled the logic to parse different response types (text vs JSON).

These changes make the code cleaner and more robust, especially considering it's intended for local server interactions.

Expand Down
1 change: 1 addition & 0 deletions core/utils/req_helper/requset.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -87,6 +87,7 @@ func handleGetWithTransport(url string, transport *http.Transport) (*http.Respon
Timeout: time.Second * 300,
}
client.Transport = transport
defer client.CloseIdleConnections()

req, err := http.NewRequestWithContext(context.Background(), "GET", url, nil)
if err != nil {
Expand Down
Loading