database: Avoid race condition in connection creation #26147
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
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 leaderhip 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 conenction creation, this is probably acceptable.
github-actions
bot
added
the
hashicorp-contributed-pr
swenson
commented
Mar 25, 2024
| dbi := b.connections.Get(name) | ||
| if dbi != nil { | ||
| return dbi, nil | ||
| } | ||
|
|
||
| // slow path, create a new connection | ||
| // if we don't lock the rest of the operation, there is a race condition for multiple callers of this function | ||
| b.createConnectionLock.Lock() |
There was a problem hiding this comment.
If we delete this block, the fix will still work; we still see a spike in open file descriptors on leadership transfers under load, but the file descriptors are closed correctly (because we switch to PutIfEmpty below).
|
CI Results: |
|
Build Results: |
Co-authored-by: Jason O'Donnell <[email protected]>
benashz
approved these changes
Mar 26, 2024
| for i := 0; i < goroutines; i++ { | ||
| go func(i int) { | ||
| dbis[i], errs[i] = b.GetConnection(ctx, s, "mockv5") | ||
| wg.Done() |
There was a problem hiding this comment.
nit: probably not necessary, but deferring this call just before calling b.GetConnection() feels a bit better.
| errs := make([]error, goroutines) | ||
| for i := 0; i < goroutines; i++ { | ||
| go func(i int) { | ||
| dbis[i], errs[i] = b.GetConnection(ctx, s, "mockv5") |
There was a problem hiding this comment.
nit: might want to factor mockv5 out to a constant to avoid duplication.
builtin/logical/database/backend.go
Outdated
| conn, ok := b.connections.PutIfEmpty(name, dbi) | ||
| if !ok { | ||
| // this is a bug | ||
| b.Logger().Error("Error: there was a race condition adding to the database connection map") |
There was a problem hiding this comment.
Should we mention that we hit a bug here? A user could then report it.
Should this be Warn() like line 355 below?
|
|
||
| // TestGetConnectionRaceCondition checks that GetConnection always returns the same instance, even when asked | ||
| // by multiple goroutines in parallel. | ||
| func TestGetConnectionRaceCondition(t *testing.T) { |
jasonodonnell
approved these changes
Mar 26, 2024
|
Thanks! |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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: