Skip to content

Allow start command to force waiting resources to start#7312

Merged
JamesNK merged 9 commits intomainfrom
jamesnk/starting-waiting-resource
Jan 29, 2025
Merged

Allow start command to force waiting resources to start#7312
JamesNK merged 9 commits intomainfrom
jamesnk/starting-waiting-resource

Conversation

@JamesNK
Copy link
Copy Markdown
Member

@JamesNK JamesNK commented Jan 29, 2025

Description

Addresses #5879

This PR makes it possible to force waiting resources to start. The important thing to consider here is waiting resources are already trying to start and they're waiting on BeforeResourceStartedEvent to finish.

I'm not happy about thread safety here. Suggestions welcome.

TODO:

  • Functional tests

Checklist

  • Is this feature complete?
    • Yes. Ready to ship.
    • No. Follow-up changes expected.
  • Are you including unit tests for the changes and scenario tests if relevant?
    • Yes
    • No
  • Did you add public API?
    • Yes
      • If yes, did you have an API Review for it?
        • Yes
        • No
      • Did you add <remarks /> and <code /> elements on your triple slash comments?
        • Yes
        • No
    • No
  • Does the change make any security assumptions or guarantees?
    • Yes
      • If yes, have you done a threat model and had a security review?
        • Yes
        • No
    • No
  • Does the change require an update in our Aspire docs?

@JamesNK JamesNK added the area-app-model Issues pertaining to the APIs in Aspire.Hosting, e.g. DistributedApplication label Jan 29, 2025
Copy link
Copy Markdown
Contributor

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.

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

Files not reviewed (1)
  • playground/Stress/Stress.AppHost/Program.cs: Evaluated as low risk
Comments suppressed due to low confidence (3)

src/Aspire.Hosting/Orchestrator/ApplicationOrchestrator.cs:214

  • The new behavior introduced by the changes, especially the handling of waiting resources, is not covered by tests. Functional tests need to be added to ensure the correctness of the new logic.
public async Task StartResourceAsync(string resourceName, CancellationToken cancellationToken)

src/Aspire.Hosting/Dcp/DcpExecutor.cs:1620

  • [nitpick] The variable name 'appResource' is ambiguous. It should be renamed to 'resourceReference' to match the method parameter.
var appResource = (AppResource)resourceReference;

src/Aspire.Hosting/Dcp/DcpExecutor.cs:1634

  • The use of ConcurrentDictionary for _startingResourceState should be reviewed for thread safety, particularly around the WaitForDependenciesTask and WaitCancellation properties.
throw new InvalidOperationException($"Unexpected resource type: {appResource.DcpResource.GetType().FullName}");

Comment thread src/Aspire.Hosting/Orchestrator/ApplicationOrchestrator.cs Outdated
@JamesNK JamesNK merged commit c3dc761 into main Jan 29, 2025
@JamesNK JamesNK deleted the jamesnk/starting-waiting-resource branch January 29, 2025 11:49
@github-actions github-actions Bot locked and limited conversation to collaborators Mar 1, 2025
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

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants