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

HostFactoryResolver - Increase default timeout to thirty seconds #61621

Merged
merged 8 commits into from
Nov 17, 2021

Conversation

maryamariyan
Copy link
Member

@maryamariyan maryamariyan commented Nov 15, 2021

Customer Impact

When customer has debugger attached or when using tools like EF, host startup path using WebApplicationBuilder for hosting and WebApplicationFactory for testing, may fail to build and throw an InvalidOperationException with the message “Unable to build IHost.”, because the default timeout is only 5 seconds.

We are proposing to use a higher timeout (1 minute) to be used so that when an application is taking longer to build, we could allow startup code to complete, and the host can start successfully.

Increasing the timeout should not have a negative impact on experience.

TODO

  • Add test for this scenario to ensure it works.
  • Add environment variable
  • improve exception message

Risk

Very low.

Regression

No, new 6.0 feature/code flow for discovering a host.
Even though this is not a regression (uses WebApplicationBuilder which is new), but since this code is in the ASP.NET Core templates, it regresses the scenario when using default templates.

Fixes #60891, dotnet/aspnetcore#33886

@ghost
Copy link

ghost commented Nov 15, 2021

Tagging subscribers to this area: @eerhardt, @maryamariyan
See info in area-owners.md if you want to be subscribed.

Issue Details
  • Add test
Author: maryamariyan
Assignees: maryamariyan
Labels:

area-Extensions-Hosting

Milestone: -

@@ -22,7 +22,7 @@ internal sealed class HostFactoryResolver
public const string CreateHostBuilder = nameof(CreateHostBuilder);

// The amount of time we wait for the diagnostic source events to fire
private static readonly TimeSpan s_defaultWaitTimeout = Debugger.IsAttached ? Timeout.InfiniteTimeSpan : TimeSpan.FromSeconds(5);
private static readonly TimeSpan s_defaultWaitTimeout = Debugger.IsAttached ? Timeout.InfiniteTimeSpan : TimeSpan.FromMinutes(1);
Copy link
Member

Choose a reason for hiding this comment

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

Assuming we improve the error message to make it clear that there was a timeout, are we worried that people will just kill the migration before seeing the error if we make this a full minute?

Copy link
Member Author

@maryamariyan maryamariyan Nov 16, 2021

Choose a reason for hiding this comment

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

updates exception messaging and changed it to 30 seconds, assuming we would keep the environment variable option.

@halter73 halter73 changed the title HostFactoryResolver - Increase default timeout to one minute HostFactoryResolver - Increase default timeout to thirty seconds Nov 16, 2021
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.

LGTM

@davidfowl
Copy link
Member

How did we land on 30 seconds? Just FYI, the reason the keyvault configuration provider tends to be the one that times out is because it makes an HTTP request per key, value pair to get the initial set of configuration values. The default should be like 5 minutes to just cover those scenarios.

@maryamariyan
Copy link
Member Author

How did we land on 30 seconds?

My thought was, since we are allowing to accept environment variables then it's fair to reduce to 30 seconds. Also because of @halter73 's comment below:

are we worried that people will just kill the migration before seeing the error if we make this a full minute?

@davidfowl
Copy link
Member

It should be more than 30 seconds because of the keyvault scenario (if you want some concrete data, look for keyvault configuration provider issues). My vote is 5 minutes

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

Successfully merging this pull request may close these issues.

System.InvalidOperationException: Unable to build IHost when creating EF Migration with Minimal API dotnet 6
4 participants