Lock the credentials store when reading or writing#15610
Merged
zanieb merged 1 commit intofeature/authfrom Sep 2, 2025
Merged
Conversation
zanieb
added a commit
that referenced
this pull request
Sep 2, 2025
Adds locking of the credentials store for concurrency safety. It's important to hold the lock from read -> write so credentials are not dropped during concurrent writes. I opted not to attach the lock to the store itself. Instead, I return the lock on read and require it on write to encourage safe use. Maybe attaching the source path to the store struct and adding a `lock(&self)` method would make sense? but then you can forget to take the lock at the right time. The main problem with the interface here is to write a _new_ store you have to take the lock yourself, and you could make a mistake by taking a lock for the wrong path or something. The fix for that would be to introduce a new `CredentialStoreHandle` type or something, but that seems overzealous rn. We also don't eagerly drop the lock on token read, although we could.
Merged
This file contains hidden or 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
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.
Adds locking of the credentials store for concurrency safety. It's important to hold the lock from read -> write so credentials are not dropped during concurrent writes.
I opted not to attach the lock to the store itself. Instead, I return the lock on read and require it on write to encourage safe use. Maybe attaching the source path to the store struct and adding a
lock(&self)method would make sense? but then you can forget to take the lock at the right time. The main problem with the interface here is to write a new store you have to take the lock yourself, and you could make a mistake by taking a lock for the wrong path or something. The fix for that would be to introduce a newCredentialStoreHandletype or something, but that seems overzealous rn. We also don't eagerly drop the lock on token read, although we could.