Skip to content

HOTFIX: Reuse http client#60

Merged
prasadlohakpure merged 4 commits intomainfrom
hf_reuse_http_client
Apr 24, 2026
Merged

HOTFIX: Reuse http client#60
prasadlohakpure merged 4 commits intomainfrom
hf_reuse_http_client

Conversation

@prasadlohakpure
Copy link
Copy Markdown
Contributor

@prasadlohakpure prasadlohakpure commented Apr 22, 2026

Description

This pull request refactors the HTTP client handling in internal/pkg/pipeline/task/http/http.go to improve connection management, reuse clients, and set sensible defaults for connection limits. The changes introduce a reusable HTTP client with default connection settings, ensure proper client initialization, and adjust how the client is constructed when using proxies.

HTTP Client Management Improvements:

  • Added a client field to the httpCore struct and a getClient() method to initialize and reuse an http.Client with default connection limits (MaxConnsPerHost and MaxIdleConnsPerHost set to 100). [1] [2] [3]
  • Updated the newFromInput method to ensure cloned httpCore instances share the properly initialized HTTP client.

Connection Handling and Proxy Support:

  • Refactored the call method to use the reusable client from getClient(), and to create a new client only when a proxy is specified, preserving custom transport settings. [1] [2]

Resource Management:

  • Changed the response body handling to close the body immediately after reading, ensuring resources are released promptly.

Testing:

Tested using /test/pipelines/context_test_with_storage_class.yaml

Types of changes

  • Docs change / refactoring / dependency upgrade
  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to change)

Checklist

  • My code follows the code style of this project.
  • My change requires a change to the documentation and I have updated the documentation accordingly.
  • I have added tests to cover my changes.

Copilot AI review requested due to automatic review settings April 22, 2026 06:38
@prasadlohakpure prasadlohakpure requested a review from a team as a code owner April 22, 2026 06:38
@wiz-55ccc8b716
Copy link
Copy Markdown

wiz-55ccc8b716 Bot commented Apr 22, 2026

Wiz Scan Summary

Scanner Findings
Vulnerability Finding Vulnerabilities -
Data Finding Sensitive Data -
Secret Finding Secrets -
IaC Misconfiguration IaC Misconfigurations -
SAST Finding SAST Findings -
Software Management Finding Software Management Findings -
Total -

View scan details in Wiz

To detect these findings earlier in the dev lifecycle, try using Wiz Code VS Code Extension.

Copy link
Copy Markdown
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull request overview

Refactors the HTTP task to reuse an http.Client across requests to improve connection reuse and reduce per-request client creation overhead.

Changes:

  • Added a cached *http.Client on httpCore with a getClient() initializer and default per-host connection limits.
  • Updated newFromInput() to copy the client into cloned task instances.
  • Updated call() to use the cached client by default and construct a per-call client when a proxy is configured.
Comments suppressed due to low confidence (2)

internal/pkg/pipeline/task/http/http.go:340

  • defer response.Body.Close() is inside the retry loop. On retry paths (e.g., unexpected status), the body won't be closed until call() returns, which can keep connections in-use and starve the transport. Close the body explicitly before any continue and avoid defer inside the loop so each attempt releases resources promptly.
		}
		defer response.Body.Close()

		body, err := io.ReadAll(response.Body)
		if err != nil {

internal/pkg/pipeline/task/http/http.go:316

  • For proxied requests, the transport comes from h.Proxy.getTransport(), which (today) sets only proxy/TLS fields. If the goal is consistent connection management, consider basing the proxy transport on the same default/limited transport used elsewhere (clone base transport, then apply proxy/TLS), so idle timeouts/HTTP2/dialer defaults and connection limits are preserved.
		if h.Proxy != nil {
			transport, err := h.Proxy.getTransport()
			if err != nil {
				lastErr = err

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Comment thread internal/pkg/pipeline/task/http/http.go Outdated
Comment on lines +92 to +97
h.client = &http.Client{
Timeout: time.Duration(h.Timeout),
Transport: &http.Transport{
MaxConnsPerHost: defaultMaxConnsPerHost,
MaxIdleConnsPerHost: defaultMaxConnsPerHost,
},
Copy link

Copilot AI Apr 22, 2026

Choose a reason for hiding this comment

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

getClient() constructs a new http.Transport with only MaxConnsPerHost/MaxIdleConnsPerHost set. This drops important defaults from http.DefaultTransport (e.g., ProxyFromEnvironment, HTTP/2 enablement, IdleConnTimeout/TLSHandshakeTimeout, dialer keep-alives), potentially regressing connectivity and reuse. Prefer cloning http.DefaultTransport.(*http.Transport).Clone() and then overriding the connection-limit fields.

Suggested change
h.client = &http.Client{
Timeout: time.Duration(h.Timeout),
Transport: &http.Transport{
MaxConnsPerHost: defaultMaxConnsPerHost,
MaxIdleConnsPerHost: defaultMaxConnsPerHost,
},
transport := http.DefaultTransport.(*http.Transport).Clone()
transport.MaxConnsPerHost = defaultMaxConnsPerHost
transport.MaxIdleConnsPerHost = defaultMaxConnsPerHost
h.client = &http.Client{
Timeout: time.Duration(h.Timeout),
Transport: transport,

Copilot uses AI. Check for mistakes.
Comment on lines 120 to 124
RetryDelay: h.RetryDelay,
client: h.getClient(),
}

if err := json.Unmarshal(data, newHttp); err != nil {
Copy link

Copilot AI Apr 22, 2026

Choose a reason for hiding this comment

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

newFromInput assigns client: h.getClient() before json.Unmarshal runs. If the payload overrides timeout, the new httpCore will still reuse a client initialized with the old timeout, so the configured timeout is ignored. Initialize/rebind the client after unmarshalling (or reuse only a shared Transport and create a per-instance client using newHttp.Timeout).

Copilot uses AI. Check for mistakes.
Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

I think we wont have use cases where timeout will change with every input record. In that case we will need to keep on changing transport object as well, which would be costly. Hence we will continue using the timeout configured previously. WDUT Divyanshu Tiwari (@divyanshu-tiwari)

Comment thread internal/pkg/pipeline/task/http/http.go Outdated
@prasadlohakpure prasadlohakpure merged commit f9443da into main Apr 24, 2026
7 checks passed
@prasadlohakpure prasadlohakpure deleted the hf_reuse_http_client branch April 24, 2026 06:52
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.

4 participants