dotnet / aspnetcore

ASP.NET Core is a cross-platform .NET framework for building modern cloud-based web applications on Windows, Mac, or Linux.
https://asp.net
MIT License
35.16k stars 9.92k forks source link

DeferredHostBuilder starts the host during the build action #38335

Open Blackclaws opened 2 years ago

Blackclaws commented 2 years ago

Describe the bug

This issue is about a very hidden behaviour when DeferredHostBuilder is being used.

I came across this issue when fixing an issue with databases being seeded twice during tests:

https://github.com/ardalis/CleanArchitecture/pull/277/files

The reason is that in contrast to other IHostBuilder implementations DeferredHostBuilder actually does start the Host when calling:

IHostBuilder builder;
builder.Build();

Since this is also done on a separate thread this leads to race conditions in code that previously didn't have them. The issue comes up when switching to the new template for aspnetcore servers that don't use a Startup class

To Reproduce

  1. Create an aspnetcore 6.0 project without a Startup class
  2. Create a CustomWebApplicationFactory extending WebApplicationFactory
  3. Override CreateHost(IHostBuilder builder)
  4. Call builder.Build() then perform other things with the host's services
  5. Notice that the code in Program.cs is running in parallel and you might be getting race conditions now

Exceptions (if any)

Further technical details

Blackclaws commented 2 years ago

Repro is here:

https://github.com/Blackclaws/deferred-host-repro

I see a couple of solutions to this:

  1. Defer the host start until the actual Start action. This however will mean that none of the service registrations that are done in Program.cs are available, probably not what is expected
  2. Defer any access to _host.Services in the DeferredHost until the host has actually finished startup (that way you can't interact with the host until it is done.
halter73 commented 2 years ago

Notice that the code in Program.cs is running in parallel and you might be getting race conditions now

This is by design. With the new templates, we run Program.Main just to get the IHostBuilder from a DiagnosticListener since there's no more Program.CreateHostBuilder or Program.CreateWebHostBuilder. See https://github.com/dotnet/runtime/blob/8574ce9a9a195cac371e3a2cfe5d36ec754aa94c/src/libraries/Microsoft.Extensions.HostFactoryResolver/src/HostFactoryResolver.cs#L42-L47

Blackclaws commented 2 years ago

I know its working as intended. The problem is that is hides a race condition that didn't use to be there and suddenly you can get intermittent test failures that don't seem to make a lot of sense.

Maybe builder.Build() should just block until the host is ready. That would also eliminate further race conditions. Its simply unclear to the person writing the tests that moving from Startup class to Program.cs top level statements will completely break their tests as this is hidden in the details of WebApplicaitonFactory.

davidfowl commented 2 years ago

Its also a question about how common customers will run into this scenario.

Blackclaws commented 2 years ago

I expect that it might be a rare occurrence in general. But it can introduce very subtle errors into tests if it does happen.

I'm not sure it actually plays a role outside of running tests at all. That being said, I think the current behavior also does kind of violate the documentation (https://github.com/dotnet/runtime/blob/8574ce9a9a195cac371e3a2cfe5d36ec754aa94c/src/libraries/Microsoft.Extensions.Hosting.Abstractions/src/IHostBuilder.cs#L76) on

 /// <summary>
 /// Run the given actions to initialize the host. This can only be called once.
 /// </summary>
 /// <returns>An initialized <see cref="IHost"/>.</returns>
IHost Build();

This isn't an async method, nor does it state that the host wouldn't be finished initializing after it returns.

Thinking about this further I think the two best solutions would indeed be:

  1. Introduce BuildAsync to IHostBuilder though this is probably overkill. I think there isn't any other host that has this kind of behaviour and would need it.
  2. Block on Build() until the host is actually done initializing

If you'd like I can prepare a pull request for solution 2 and we can continue the discussion there as well.

adityamandaleeka commented 1 year ago

@captainsafia Just found another old issue. Any update on this? Not sure if there was another discussion elsewhere about it.

adityamandaleeka commented 1 year ago

@captainsafia Can you please re-milestone this?

captainsafia commented 1 year ago

@adityamandaleeka There hasn't been any work on this. I'm inclined to backlog considering priorities.

ghost commented 1 year ago

We've moved this issue to the Backlog milestone. This means that it is not going to be worked on for the coming release. We will reassess the backlog following the current release and consider this item at that time. To learn more about our issue management process and to have better expectation regarding different types of issues you can read our Triage Process.

Taha-cmd commented 1 year ago

We had some flaky tests that we were able to track down to this.

In Program.cs

var app = builder.Build();
app.Run(); // expected to throw due to startup validation of invalid options. 

Then this line https://github.com/dotnet/aspnetcore/blob/main/src/Mvc/Mvc.Testing/src/DeferredHostBuilder.cs#L152 will throw an ObjectDisposedException: "System.ObjectDisposedException: Cannot access a disposed object. Object name: 'IServiceProvider'.

Tested with ASP.NET Core 7.0.9

Edit: this is happening with the DelegatedWebApplicationFactory that is returned by WebApplicationFactory.WithWebHostBuilder

fschmied commented 1 year ago

We had some flaky tests that we were able to track down to this.

... and, seeing this has been a known issue since 2021: it required a few hours of very intense debugging that I'd rather have spent otherwise, tbh. 🙂

MichaelHochriegl commented 1 year ago

I too run into the same problem and I have a quick reproduction repo here: https://github.com/MichaelHochriegl/WebAppFactoryHostedService

I have a HostedService that is accessing the DbContext and due the app already running the overriden service is not used. If I delay the Program.cs the setup in my custom WebApplicationFactory is already done by the time the app is started and it works.

Currently I can't switch any of my work projects to minimal apis as every test-setup that I have is borked.

Sonic198 commented 9 months ago

We had some flaky tests that we were able to track down to this.

In Program.cs

var app = builder.Build();
app.Run(); // expected to throw due to startup validation of invalid options. 

Then this line https://github.com/dotnet/aspnetcore/blob/main/src/Mvc/Mvc.Testing/src/DeferredHostBuilder.cs#L152 will throw an ObjectDisposedException: "System.ObjectDisposedException: Cannot access a disposed object. Object name: 'IServiceProvider'.

Tested with ASP.NET Core 7.0.9

Edit: this is happening with the DelegatedWebApplicationFactory that is returned by WebApplicationFactory.WithWebHostBuilder

Hi, Did you find any solution to this?

Taha-cmd commented 9 months ago

We added resiliency (retries) to our tests