Skip to content

Conversation

davidfowl
Copy link
Member

@davidfowl davidfowl commented Jul 12, 2025

Description

This was found while dogfooding the latest build with async parameter resolution. We were using ParameterResource.Value is many places and as a result those places did not properly wait for values to be resolved before continuing, they just failed. Now ParameterResource.Value calls ParameterResource.GetValueAsync(...).GetAwaiter().GetResult(), which is not great, but is consistent. We need the sync and async versions to work exactly the same way to avoid inconsistent state.

There's a risk with introducing deadlocks with this change for people that were using .Value before. I'm working through tests now to make sure that does not happen in practice. 👍🏾 If the Value is accessed before the TCS is set then it will throw an exception like it did before.

  • Value now blocks (sync over async) waiting for value resolution if WaitForValueTcs is set.
  • Made GetValueAsync on ParameterResource public.
  • Changed most code outside of tests to use GetValueAsync instead of Value

Fixes #10352

Checklist

  • Is this feature complete?
    • Yes. Ready to ship.
  • Are you including unit tests for the changes and scenario tests if relevant?
    • Yes
  • Did you add public API?
    • Yes
  • Does the change make any security assumptions or guarantees?
    • No
  • Does the change require an update in our Aspire docs?
    • No

- Value now blocks (sync over async) waiting for value resolution if WaitForValueTcs is set.
- Made GetValueAsync on ParameterResource public.
- Changed most code outside of tests to use GetValueAsync instead of Value
@davidfowl davidfowl added this to the 9.4 milestone Jul 12, 2025
@davidfowl davidfowl requested review from Copilot, eerhardt and mitchdenny and removed request for Copilot July 12, 2025 09:17
@github-actions github-actions bot added the area-app-model Issues pertaining to the APIs in Aspire.Hosting, e.g. DistributedApplication label Jul 12, 2025
Copy link
Contributor

@Copilot Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull Request Overview

This PR ensures that parameter values are resolved consistently by making ParameterResource.Value block on async resolution, exposing GetValueAsync, and updating callers to use the async API.

  • ParameterResource.Value now synchronously awaits the async resolution path
  • GetValueAsync is made public and most callers switch from .Value to .GetValueAsync(...)
  • Call sites in various builder extensions are updated to await parameter values

Reviewed Changes

Copilot reviewed 9 out of 9 changed files in this pull request and generated 5 comments.

Show a summary per file
File Description
src/Aspire.Hosting/ResourceBuilderExtensions.cs Await URL parameter resolution in WithEnvironment
src/Aspire.Hosting/Orchestrator/ParameterProcessor.cs Switched from Value to ValueInternal in parameter processing
src/Aspire.Hosting/ExternalServiceBuilderExtensions.cs Expose async URL retrieval and adjust initial state
src/Aspire.Hosting/ApplicationModel/ParameterResource.cs Made GetValueAsync public and unified Value implementation
src/Aspire.Hosting.Redis/RedisBuilderExtensions.cs Use async resolution for Redis password parameter
src/Aspire.Hosting.MySql/MySqlBuilderExtensions.cs Await MySQL password parameter in PhpMyAdmin setup
src/Aspire.Hosting.Azure/Provisioning/Provisioners/BicepProvisioner.cs Use GetValueAsync for parameter-based resource group names
Comments suppressed due to low confidence (1)

src/Aspire.Hosting/ApplicationModel/ParameterResource.cs:89

  • New public API GetValueAsync was introduced but there are no unit tests for it. Consider adding tests to cover waiting on WaitForValueTcs and the fallback to ValueInternal.
    public async ValueTask<string?> GetValueAsync(CancellationToken cancellationToken)

@davidfowl
Copy link
Member Author

/backport to release/9.4

Copy link
Contributor

Started backporting to release/9.4: https://github.com/dotnet/aspire/actions/runs/16240431419

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

area-app-model Issues pertaining to the APIs in Aspire.Hosting, e.g. DistributedApplication breaking-change Issue or PR that represents a breaking API or functional change over a prerelease.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

ExternalServiceResource does not work if the parameter value is not set

2 participants