Increase IdleConnTimeout from 120ms to 5s

[welteki]

Description

Increase the IdleConnTimeout in makeHTTPClient from 120ms to 5s. The previous value was too aggressive and could cause connections to be closed before they could be reused from the connection pool.

Also removes commented-out transport configuration (KeepAlive, MaxIdleConns, DisableKeepAlives) that was left behind.

Motivation and Context

Whilst not as important here since a new HTTP client is created for every call to fetchYAML, this is being updated for consistency across components.

[reviewfn]

AI Pull Request Overview

Summary

• Increases IdleConnTimeout in HTTP client from 120ms to 5s for better connection reuse
• Removes commented-out transport configuration lines (KeepAlive, MaxIdleConns, DisableKeepAlives)
• Change affects makeHTTPClient function used in fetchYAML for remote YAML retrieval
• Timeout increase addresses aggressive connection closure that could prevent reuse from pool
• Update is for consistency across OpenFaaS components
• No breaking changes or API modifications
• Affects network performance for YAML fetching operations

Approval rating (1-10)

8/10 - Solid improvement for connection handling, but lacks test coverage for HTTP client configuration.

Summary per file

Summary per file

File path	Summary
stack/stack.go	Modified makeHTTPClient to increase IdleConnTimeout to 5s and remove dead transport config code.

Overall Assessment

The PR makes a reasonable improvement to HTTP client configuration by extending the idle connection timeout from an overly aggressive 120ms to a more standard 5 seconds, which should enhance connection pooling efficiency for YAML fetching operations. The cleanup of commented code is beneficial for maintainability. However, the absence of tests covering the HTTP transport settings represents a risk for future regressions in network behavior.

Detailed Review

Detailed Review

stack/stack.go

• IdleConnTimeout Change: Increasing from 120ms to 5s is appropriate. The original 120ms was excessively short for connection reuse in typical network conditions. 5 seconds aligns with common HTTP client defaults and should improve performance for sequential YAML fetches. No concerns with correctness.

• Commented Code Removal: Removing the dead transport configuration lines (KeepAlive, MaxIdleConns, DisableKeepAlives) improves code cleanliness. Since they were commented out and not used, their removal poses no risk.

• Function Context: The makeHTTPClient function is only used in fetchYAML with a 120-second request timeout, so the idle timeout increase won't cause hanging connections. The change is isolated to this specific use case.

• Consistency: As noted in the PR description, this aligns with other OpenFaaS components, which is good for ecosystem uniformity.

• Testing Gap: No existing tests validate the HTTP client transport configuration. While this change is low-risk, adding unit tests for makeHTTPClient would prevent future regressions. Consider testing that the returned http.Client has the expected Timeout, IdleConnTimeout, and ExpectContinueTimeout values.

• Performance Impact: Positive - better connection reuse will reduce overhead for multiple YAML fetches. No negative performance implications identified.

• Security: No security concerns. The change only affects connection lifecycle management, not data handling or authentication.

• Migration: No migration needed for users. This is an internal implementation detail with no external API changes.

AI agent details.

Agent processing time: 38.479s
Environment preparation time: 5.499s
Total time from webhook: 46.528s