Skip to content

Conversation

JamesNK
Copy link
Member

@JamesNK JamesNK commented Feb 11, 2025

Use WaitAsync again. Also, avoid throwing DistributedApplicationException when restarting resource.

@JamesNK JamesNK added the area-app-model Issues pertaining to the APIs in Aspire.Hosting, e.g. DistributedApplication label Feb 11, 2025
@JamesNK JamesNK requested a review from mitchdenny as a code owner February 11, 2025 10:23
@davidfowl
Copy link
Member

backport this plz

@JamesNK
Copy link
Member Author

JamesNK commented Feb 13, 2025

The code in the 9.1 branch already doesn't throw errors, it just does it in a different way.

(not throwing the app host exception isn't in 9.1, but it is one message when restarting a resource so is low impact compared to the TimeoutException every few seconds)

What is the reason to backport?

@davidfowl
Copy link
Member

Is the error showing up in the console output? That will make people report bugs (especially if it wasn't happening in 9.0).

@JamesNK
Copy link
Member Author

JamesNK commented Feb 13, 2025

The DistributedApplicationException error on restart appears on the dev console, but that isn't a regression.

The regression in 9.1 was many TimeoutException instances being thrown in the dev console, and that's already fixed in 9.1.

@davidfowl
Copy link
Member

The DistributedApplicationException error on restart appears on the dev console, but that isn't a regression.

Dev console, are you talking about the debug window? Does it show up in the dashboard anywhere or in the app host console?

@JamesNK
Copy link
Member Author

JamesNK commented Feb 13, 2025

Dev console, are you talking about the debug window?

Yes. See screenshots here: #7496

Does it show up in the dashboard anywhere or in the app host console?

They just show up in the IDE debug console. These exceptions are all caught and ignored, but that doesn't stop them from appearing in the debug console.

I created an issue about reducing how many times we throw exceptions to improve this situation: #7528

{
BackoffType = DelayBackoffType.Exponential,
Delay = TimeSpan.FromMilliseconds(200),
UseJitter = true,
MaxRetryAttempts = 10, // Cumulative time for all attempts amounts to about 15 seconds
MaxDelay = TimeSpan.FromSeconds(3),
ShouldHandle = new PredicateBuilder().Handle<Exception>(),
ShouldHandle = args => ValueTask.FromResult(!args.Outcome.Result),
Copy link
Member

Choose a reason for hiding this comment

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

What about when an exception occurs that isn't Http NotFound? Should we retry on that?

Copy link
Member Author

Choose a reason for hiding this comment

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

No, the retry here is specifically just for not found.

Copy link
Member

@eerhardt eerhardt left a comment

Choose a reason for hiding this comment

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

I just had one question, the rest of the changes look fine to me.

@JamesNK JamesNK force-pushed the jamesnk/healthmonitoring-errorhandling-2 branch from 8a2b28d to 6dd96ca Compare February 28, 2025 05:32
@JamesNK JamesNK merged commit c36342c into main Feb 28, 2025
71 checks passed
@JamesNK JamesNK deleted the jamesnk/healthmonitoring-errorhandling-2 branch February 28, 2025 09:46
@github-actions github-actions bot locked and limited conversation to collaborators Mar 31, 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.

4 participants