Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Reduce thrown exceptions #7527

Open
wants to merge 3 commits into
base: main
Choose a base branch
from

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

@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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
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