Skip to content

fix goroutine leak#113

Open
jarvis-u wants to merge 1 commit intohashicorp:mainfrom
jarvis-u:main
Open

fix goroutine leak#113
jarvis-u wants to merge 1 commit intohashicorp:mainfrom
jarvis-u:main

Conversation

@jarvis-u
Copy link

@jarvis-u jarvis-u commented Oct 16, 2025

Overview

I discovered through grafana monitoring that there was a goroutine leakage issue in vault. Later, after analyzing it with pprof, I found that the usage of the http connection pool in the elasticsearch plugin was incorrect

Design of Change

How was this change implemented?
Fixed the goroutine leakage issue

Related Issues/Pull Requests

[ ] Issue #110

PCI review checklist

  • I have documented a clear reason for, and description of, the change I am making.

  • If applicable, I've documented a plan to revert these changes if they require more than reverting the pull request.

  • If applicable, I've documented the impact of any changes to security controls.

    Examples of changes to security controls include using new access control methods, adding or removing logging pipelines, etc.

@jarvis-u jarvis-u requested a review from a team as a code owner October 16, 2025 09:40
@hashicorp-cla-app
Copy link

hashicorp-cla-app bot commented Oct 16, 2025

CLA assistant check
All committers have signed the CLA.

@hashicorp-cla-app
Copy link

CLA assistant check

Thank you for your submission! We require that all contributors sign our Contributor License Agreement ("CLA") before we can accept the contribution. Read and sign the agreement

Learn more about why HashiCorp requires a CLA and what the CLA includes

Have you signed the CLA already but the status is still pending? Recheck it.

Copy link
Contributor

@fairclothjm fairclothjm left a comment

Choose a reason for hiding this comment

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

Hi @jarvis-u ! Thanks for the contribution! I left some questions

}

client.HTTPClient.Transport = &http.Transport{TLSClientConfig: conf}
transport := cleanhttp.DefaultTransport()
Copy link
Contributor

Choose a reason for hiding this comment

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

I believe this turns off connection pooling? How will this perform under heavy load?

What is the root cause of the goroutine leak you are seeing? Is it indeed http.Transport or is it that this plugin creates a new client on each request? Should we instead re-use the client and enable pooling?

Copy link
Author

@jarvis-u jarvis-u Nov 4, 2025

Choose a reason for hiding this comment

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

Thank you for your comments, more detailed analysis goroutine leak I recorded in the issue of #110

Your insight is very correct. It is precisely because this plugin creates a new client every time a request is made.

What we need to do is precisely to close the connection pool. An unclosed connection pool is the fundamental cause of goroutine leakage, as a new connection pool is created each time this function is called, while the old connection pool is ignored by us.

This change of mine will not reduce performance because the connection pool was not reused in the previous code either.

Reusing the connection pool is a good idea, but in my opinion, it is rather difficult to implement because the configuration of the es instance that requests this interface each time is not fixed, and we need to use this configuration in the connection pool.

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.

2 participants