Concurrency pwd generation issue #13774
Conversation
…parameter generation
-and fixed build errors
|
🚀 Dogfood this PR with:
curl -fsSL https://raw.githubusercontent.com/dotnet/aspire/main/eng/scripts/get-aspire-cli-pr.sh | bash -s -- 13774Or
iex "& { $(irm https://raw.githubusercontent.com/dotnet/aspire/main/eng/scripts/get-aspire-cli-pr.ps1) } 13774" |
dotnet-policy-service
bot
added
the
community-contribution
|
@dotnet-policy-service agree |
| Debug.WriteLine($"Failed to set value for parameter '{parameterName}' in application '{applicationName}' to user secrets."); | ||
| } | ||
| return value; | ||
| Debug.WriteLine($"UserSecretsParameterDefault: applicationName: {applicationName}, parameterName: {parameterName}"); |
There was a problem hiding this comment.
I don't think this debug is appropriate anymore. It's now being written every time GetDefaultValue is called. Before it was written only on a failure.
There was a problem hiding this comment.
You are right, but i just added it in order to use applicationName somehow since there was an error raised if left unused. And i didn't wanna change the signature of UserSecretsParameterDefault . I know this may be something trivial but what would you suggest? Thanks. @JamesNK
| /// <param name="name">The name of the secret.</param> | ||
| /// <param name="valueGenerator">Function to generate the value if it doesn't exist.</param> | ||
| /// <returns>The existing or generated secret value.</returns> | ||
| string GetOrSetSecret(string name, Func<string> valueGenerator); |
There was a problem hiding this comment.
A problem here is there isn't any indication of whether the secret was successfully set. It could be TryGetOrSetSecret and return a bool indicating whether the value was successfully set. But I've never seen the Try pattern combined with GetOrSet before.
Maybe it doesn't matter because the other thing the bool result is being used for is writing a debug message.
There was a problem hiding this comment.
This is an internal API so we could change it in the future easily if needed. Doesn't need to be solved now.
| } | ||
| } | ||
|
|
||
| public void GetOrSetSecret(IConfigurationManager configuration, string name, Func<string> valueGenerator) |
There was a problem hiding this comment.
I think this overload, which works very differently, would be confusing. Rename it to InitializeConfigSecret?
|
Thanks for the PR. I added some comments. |
Co-authored-by: James Newton-King <james@newtonking.com>
Co-authored-by: James Newton-King <james@newtonking.com>
Co-authored-by: James Newton-King <james@newtonking.com>
2 participants
Description
This PR addresses a race condition when multiple DistributedApplicationTestingBuilder instances are initialized concurrently (e.g., during parallel integration tests).
It's a pretty passive solution that uses simple mutexes to prevent that rare case.
The Problem
Previously, the logic for handling auto-generated parameters (like passwords) followed this flow:
By the time the lock was acquired, the value was already generated. This caused a "last-writer-wins" scenario where:
The Solution
I've introduced a new
GetOrSetSecret
pattern that inverts this flow to be atomic. The new flow is:
Implementation Details
Added
GetOrSetSecretto
IUserSecretsManagerImplemented a Mutex based on the file path hash in
UserSecretsManagerFactoryto ensure we coordinate writes effectively across test processes, not just threads.
Added resilience: if locking fails (timeout) or file IO fails, we degrade gracefully to the generated value to prevent the test suite from crashing, though the race condition might technically persist in that specific failure edge case.
This ensures both test instances agree on the same shared secret (e.g., Redis password) and containers start/connect successfully.
Fixes #13720
Checklist
<remarks />and<code />elements on your triple slash comments?doc-ideatemplatebreaking-changetemplatediagnostictemplate