Skip to content
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.

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

Backport of database: Avoid race condition in connection creation into release/1.16.x #26163

Conversation

hc-github-team-secure-vault-core
Copy link
Collaborator

Backport

This PR is auto-generated from #26147 to be assessed for backporting due to the inclusion of the label backport/1.16.x.

The below text is copied from the body of the original PR.


When creating database connections, there is a race condition when multiple goroutines try to create the connection at the same time. This happens, for example, on leadership changes in a cluster.

Normally, the extra database connections are cleaned up when this is detected. However, some database
implementations, notably Postgres, do not seem to clean up in a timely manner, and can leak in these scenarios.

To fix this, we create a global lock when creating database connections to prevent multiple connections from being created at the same time.

We also clean up the logic at the end so that if (somehow) we ended up creating an additional connection, we use the existing one rather than the new one. This by itself would solve our problem long-term, however, would still involve many transient database connections being created and immediately killed on leadership changes.

It's not ideal to have a single global lock for database connection creation. Some potential alternatives:

  • a map of locks from the connection name to the lock. The biggest downside is the we probably will want to garbage collect this map so that we don't have an unbounded number of locks.
  • a small pool of locks, where we hash the connection names to pick the lock. Using such a pool generally is a good way to introduce deadlock, but since we will only use it in a specific case, and the purpose is to improve performance for concurrent connection creation, this is probably acceptable.

Overview of commits

@hc-github-team-secure-vault-core hc-github-team-secure-vault-core force-pushed the backport/vault-25323/postgres-leak/conversely-vast-tortoise branch from b7f9579 to d451952 Compare March 26, 2024 16:58
@github-actions github-actions bot added the hashicorp-contributed-pr If the PR is HashiCorp (i.e. not-community) contributed label Mar 26, 2024
@swenson swenson enabled auto-merge (squash) March 26, 2024 17:00
Copy link

CI Results:
All Go tests succeeded! ✅

Copy link

Build Results:
All builds succeeded! ✅

@swenson swenson merged commit 5d4c989 into release/1.16.x Mar 26, 2024
74 of 76 checks passed
@swenson swenson deleted the backport/vault-25323/postgres-leak/conversely-vast-tortoise branch March 26, 2024 17:18
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
hashicorp-contributed-pr If the PR is HashiCorp (i.e. not-community) contributed
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants