Skip to content

Add a delay on unhandled failure in initial load to slow down crash loops - #79

Merged
AA (abhilasharora) merged 6 commits into
masterfrom
abarora/prevent-fast-fail
Sep 17, 2019
Merged

AA (abhilasharora) merged 6 commits into
masterfrom
abarora/prevent-fast-fail

Conversation

@abhilasharora

Copy link
Copy Markdown
Contributor

An unhandled exception encountered during the initial configuration load in the provider can cause an Azure function or web application to crash. In such a case, it is possible that the application is restarted repeatedly if the error persists and result in a large number of calls made to the server. This change adds a delay in throwing an exception that is likely to cause the app to fail in order to slow down any further attempts of re-loading the application.

@zhenlan

Zhenlan Wang (zhenlan) commented Sep 16, 2019

Copy link
Copy Markdown
Member

I'm not sure this is the right thing to do. It will cost a developer 1 minute to receive an error.

The particular issue that leads to this PR is because of the authentication failure, but we are slowing down for any failures. Do we know how exactly Azure function can cause issues this PR is trying to resolve? Are there any other alternative solutions we can pinpoint fix the issue for Azure function? #Resolved

@jimmyca15

Jimmy Campbell (jimmyca15) commented Sep 16, 2019

Copy link
Copy Markdown
Member

I'm thinking we should limit it to known troublesome errors like 401 for instance, which is the one that originally brought this to our attention. I don't see any reason to slow down "unable to parse connection string" type errors. #Resolved

@abhilasharora

Copy link
Copy Markdown
Contributor Author

I have updated the code to add delay only in case of 401 errors. For Azure functions, depending upon the trigger, it is a simple configuration change to set how many times the developer would like the function instance to be retried in case of a failure. Default value of the retry count is under 10 for most triggers, but it is common for developer to update it. However, we don't seem to have enough data to confirm that this is what happened with the requests during the spike. I'm trying out a few things with Azure functions to see if we can provide a better way for developers to consume app config than is currently detailed in the quick start guide.


In reply to: 531895812 [](ancestors = 531895812)

@jimmyca15

Jimmy Campbell (jimmyca15) commented Sep 16, 2019

Copy link
Copy Markdown
Member

This is a bit of a special scenario. Can we add a detailed comment on it?

Something like:

// This method is normally called in the application's startup code path
// Unhandled exceptions cause application crash which can result in crash loops as orchestrators attempt to restart the application
// Knowing the intended usage of the provider in startup code path, we mitigate back-to-back crash loops from overloading the server with requests by waiting a minimum time to propogate fatal errors
``` #Resolved

@zhenlan

Zhenlan Wang (zhenlan) commented Sep 16, 2019

Copy link
Copy Markdown
Member

Should we limit this to Azure function only? Like to see thoughts from others. #Resolved

@jimmyca15

Jimmy Campbell (jimmyca15) commented Sep 16, 2019

Copy link
Copy Markdown
Member

Should we limit this to Azure function only? Like to see thoughts from others.

In this scenario Azure Function is just one orchestrator that we know how to identify. There are other orchestrators like AKS that can cause the same scenario by continuously restarting the application. #Resolved

@jimmyca15

Jimmy Campbell (jimmyca15) commented Sep 17, 2019

Copy link
Copy Markdown
Member

Discussed offline with AA (@abhilasharora) and Drago Draganov (@drago-draganov). To prevent missing exceptions in the future which can also trigger a server overload crash loop such as KeyVaultReferenceException it may be a good idea to delay exception propogation by 5 seconds by default and whitelist known exceptions such as ArgumentException that we know should be thrown instantly. #Resolved


/// <summary>
/// Loads (or reloads) the data for this provider.
/// This method is normally called in the application's startup code path.

@jimmyca15 Jimmy Campbell (jimmyca15) Sep 17, 2019

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

We shouldn't put this in the summary sorry for the confusion. Then it will show up in a developers intellisense (this type is public). I meant we should put it where you had the previous comment so that we could remember why we were doing this. #Resolved

@jimmyca15 Jimmy Campbell (jimmyca15) Sep 17, 2019

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Oh wait, this type isn't public. But do you think we should put it in the summary? #Resolved

@jimmyca15

Jimmy Campbell (jimmyca15) commented Sep 17, 2019

Copy link
Copy Markdown
Member

Code looks good to me. Will await response on previous comment. #Resolved

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

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants