Skip to content

Increase IdleConnTimeout from 120ms to 5s#1015

Merged
alexellis merged 1 commit intoopenfaas:masterfrom
welteki:client-keep-alive
Mar 3, 2026
Merged

Increase IdleConnTimeout from 120ms to 5s#1015
alexellis merged 1 commit intoopenfaas:masterfrom
welteki:client-keep-alive

Conversation

@welteki
Copy link
Copy Markdown
Member

@welteki welteki commented Mar 3, 2026

Description

Increase the IdleConnTimeout from 120ms to 5s to allow connections to be re-used from the pool.

Motivation and Context

Whilst not as important in the CLI due to its short-lived nature, this is being updated across components for consistency.

  • I have raised an issue to propose this change (required)

How Has This Been Tested?

Tested by building and running the CLI locally.

Types of changes

  • 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.
  • I have updated the documentation accordingly.
  • I've read the CONTRIBUTION guide
  • I have signed-off my commits with git commit -s
  • I have added tests to cover my changes.
  • All new and existing tests passed.

@welteki
Increase IdleConnTimeout from 120ms to 5s
d4ca403 
Whilst not as important in the CLI due to its short-lived nature,
this is being updated across components for consistency.

Signed-off-by: Han Verstraete (OpenFaaS Ltd) <[email protected]>
@reviewfn
Copy link
Copy Markdown

reviewfn bot commented Mar 3, 2026

AI Pull Request Overview

Summary

  • Increases IdleConnTimeout from 120ms to 5s in HTTP transport configuration
  • Aims to improve connection reuse for better performance
  • Change made in commands/general.go for consistency across OpenFaaS components
  • No breaking changes or new features introduced
  • Tested by building and running CLI locally
  • Existing tests pass without modifications

Approval rating (1-10)

9 - Solid improvement for connection pooling, minimal risk, aligns with consistency goals.

Summary per file

Summary per file
File path Summary
commands/general.go Updated IdleConnTimeout from 120ms to 5s in GetDefaultCLITransport function

Overall Assessment

The change is a straightforward configuration update that enhances HTTP connection reuse without introducing complexity or risks. It aligns with the stated goal of consistency across components and should provide marginal performance benefits for CLI operations involving multiple requests.

Detailed Review

Detailed Review

commands/general.go:

  • The modification in GetDefaultCLITransport increases IdleConnTimeout from 120ms to 5s when a custom timeout is provided.
  • This allows idle connections to persist longer, enabling reuse within the 60-second command timeout window.
  • Given the CLI's potential for multiple HTTP requests (e.g., during deployment operations), this change is beneficial for reducing connection overhead.
  • No immediate risks identified: 5s is a reasonable timeout that balances reuse benefits against resource consumption.
  • The change is consistent with standard HTTP client practices and the PR's motivation for component consistency.
  • Consideration: Verify that this timeout doesn't conflict with any environment-specific connection limits or proxies in production use cases.
  • Suggestion: Consider documenting the rationale for this specific value in comments or contributing guidelines to maintain consistency in future updates.

AI agent details.

Agent processing time: 34.83s
Environment preparation time: 6.34s
Total time from webhook: 46.496s

@alexellis alexellis merged commit 30fa81c into openfaas:master Mar 3, 2026
2 checks passed
@welteki welteki deleted the client-keep-alive branch April 1, 2026 11:39
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

No reviews

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

2 participants

@welteki @alexellis