Fire diagnostic source events from IHostBuilder.Build #53757

davidfowl · 2021-06-05T03:46:45Z

We want to enable getting access to the IHostBuilder and IHost during the call to IHostBuilder.Build so that we can access the application's IServiceProvider from various tools (like the EntityFramework migrations tool or scaffolding). Usually, this is enabled by exposing a different entry point from the application (CreateHostBuilder) but this technique allows us to let the application execute normally and hook important events via a diagnostic source.
Updated HostFactoryResolver and added new APIs for resolving the IHostBuilder from an assembly with an entry point.
Added tests

~~TODO: I need to update the HostFactoryResolver to support this new pattern.~~

~~TODO: Add tests for various scenarios:~~

~~Exception thrown before IHostBuilder.Build is called where the exception propagates~~
~~Exception thrown before IHostBuilder.Build is called where the exception does not propagate~~
Crazy scenarios where multiple host builders are fired up at during the call to main (not sure this is important)

- We want to enable getting access to the IHostBuilder and IHost during the call to IHostBuilder.Build so that we can access the application's IServiceProvider from various tools. Usually, this is enabled by exposing a different entry point from the application but this technique allows us to let the application execute normally and hook important events via a diagnostic source.

ghost · 2021-06-05T03:46:51Z

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

Issue Details

We want to enable getting access to the IHostBuilder and IHost during the call to IHostBuilder.Build so that we can access the application's IServiceProvider from various tools. Usually, this is enabled by exposing a different entry point from the application but this technique allows us to let the application execute normally and hook important events via a diagnostic source.

Author:	davidfowl
Assignees:	-
Labels:	`area-Extensions-Hosting`
Milestone:	-

- Added support for resolving an IServiceProvider using the new diagnostics source pattern (which doesn't require a different entrypoint)

davidfowl · 2021-06-06T03:48:50Z

src/libraries/Microsoft.Extensions.HostFactoryResolver/src/HostFactoryResolver.cs

+
+            public void OnNext(DiagnosticListener value)
+            {
+                if (value.Name == "Microsoft.Extensions.Hosting")


This is where we wire up to the new event to intercept calls to build.

src/libraries/Microsoft.Extensions.HostFactoryResolver/src/HostFactoryResolver.cs

- DiagnosticSources will publish events globally and we need to make sure there's no cross talk between tests

martincostello · 2021-06-07T12:55:05Z

Would this light-up the ability to use WebApplicationFactory<T> (and maybe top-level programs) with the new minimal hosting APIs?

davidfowl · 2021-06-07T14:45:12Z

Yes that's the intent:

EF migrations
WebApplicationFactory
NSwag
Scaffolding

Etc

martincostello · 2021-06-07T14:50:44Z

Nice 👍 - I've been holding off in using it in a bunch of stuff as so much of our tests use WebApplicationFactory to do integration-level tests.

src/libraries/Microsoft.Extensions.Hosting/src/HostBuilder.cs

eerhardt · 2021-06-07T15:40:08Z

src/libraries/Microsoft.Extensions.Hosting/src/HostBuilder.cs

+        }
+
+        [UnconditionalSuppressMessage("ReflectionAnalysis", "IL2026:UnrecognizedReflectionPattern",
+            Justification = "The values being passed into Write are being consumed by the application already.")]


It isn't the "The values being passed into Write" that are the issue, but instead the recursive properties of the values.

DiagnosticSource has an EventSource bridge that allows end-users to set up "filter specs" that specify which properties of the objects passed here should be written to the EventSource. If those properties aren't used in the app, they could be trimmed.

I think that's true in general but in the use case for these events, I'm not worried:

There won't be any trimming happening in development. You don't publish an application trimmed and then run tools in it to grab the IHost and IHostBuilder.

In the 90% case, the relevant properties/methods are going to be used as part of application bootstrapping.

Of course, if we don't expect people to be using the EventSource bridge, that could be a "good enough" reason to suppress.

There won't be any trimming happening in development. You don't publish an application trimmed and then run tools in it to grab the IHost and IHostBuilder.

What about someone else who takes a dependency on this new DiagnosticSource?

Of course, if we don't expect people to be using the EventSource bridge, that could be a "good enough" reason to suppress.

Yes this isn't a use case. There's nothing serializable on there that can be sent out of process.

What about someone else who takes a dependency on this new DiagnosticSource?

Then we'll learn about the scenario then because it won't work for that scenario we don't currently understand.

I think anyone else who used the DiagnosticSource would either:
a) cast the received object to its correct type which would make the linker aware of the usage - everything works nicely here
b) private reflection - I trust this puts the onus on the new DiagnosticSource consumer to ensure the fields aren't removed

src/libraries/Microsoft.Extensions.HostFactoryResolver/src/HostFactoryResolver.cs

eerhardt

Looks fine to me. Just some random nits.

src/libraries/Microsoft.Extensions.HostFactoryResolver/src/HostFactoryResolver.cs

eerhardt · 2021-06-07T19:52:42Z

src/libraries/Microsoft.Extensions.HostFactoryResolver/src/HostFactoryResolver.cs

+                    // Wait before throwing an exception
+                    if (!_hostTcs.Task.Wait(_waitTimeout))
+                    {
+                        throw new InvalidOperationException("Unable to build IHost");


Maybe use a different message here - specifically call out that it timed out. That way we can tell the difference between the entrypoint returning gracefully vs. timing out.

I want the timeout to be an implementation detail. I'm also thinking this could also return null.

Who do you expect to see the message? If the message is primarily shown to people who are trying to resolve the issue (or users will relay it to someone else who resolves the issue) then it might help to make it more descriptive.

I need to check the callers but its possible null is a better return than throwing.

.../Microsoft.Extensions.HostFactoryResolver/tests/CreateHostBuilderInvalidSignature/Program.cs

bricelam

LGTM. I really like this approach--it's the magic I've been looking for since the beginning of ASP.NET Core.

As discussed offline, we should try hard to avoid the compile-time dependency on Microsoft.Extensions.Hosting. We had a lot of pushback against this in the 1.1 days since it adds it to all app models (see dotnet/efcore#7835 from a WPF user for example)

/cc @ajcvickers

src/libraries/Microsoft.Extensions.HostFactoryResolver/src/HostFactoryResolver.cs

src/libraries/Microsoft.Extensions.Hosting/src/HostBuilder.cs

… events to fire - We want to fail fast if we know this version of hosting will never fire events of if the hosting assembly fails to load. Added a version check. - Allow the caller to specify a wait timeout.

noahfalk · 2021-06-08T13:18:25Z

src/libraries/Microsoft.Extensions.HostFactoryResolver/src/HostFactoryResolver.cs

+            try
+            {
+                // Attempt to load hosting and check the version to make sure the events
+                // even have a change of firing (they were adding in .NET >= 6)


Nit:

Suggested change

// even have a change of firing (they were adding in .NET >= 6)

// even have a chance of firing (they were added in .NET >= 6)

noahfalk · 2021-06-08T13:29:26Z

src/libraries/Microsoft.Extensions.HostFactoryResolver/src/HostFactoryResolver.cs

+                    // Wait before throwing an exception
+                    if (!_hostTcs.Task.Wait(_waitTimeout))
+                    {
+                        throw new InvalidOperationException("Unable to build IHost");


Who do you expect to see the message? If the message is primarily shown to people who are trying to resolve the issue (or users will relay it to someone else who resolves the issue) then it might help to make it more descriptive.

noahfalk · 2021-06-08T13:32:38Z

src/libraries/Microsoft.Extensions.HostFactoryResolver/src/HostFactoryResolver.cs

+                    if (_stopApplication)
+                    {
+                        // Stop the host from running further
+                        throw new StopTheHostException();


The prize for dirty hackery this week : D

Is it important to guarantee that the thread really does stop? It would be easy enough for an app to throw a try/catch around the part of their code where this gets raised so that it never gets back to the entrypoint. Your tool code will be running in parallel with the user's unknown error handling app code. It doesn't seem obviously harmful to me but figured I mention it.

Disposal is one of the things I'm worried about and why I think throwing to stop execution makes the most sense here. The main app never gets a handle on the application here.

It would be easy enough for an app to throw a try/catch around the part of their code where this gets raised so that it never gets back to the entrypoint.

The thing that stops this from happening in the throw case is the fact that the application never gets access to the IHost instance. They can't call build again on it because double building throws. Even if they catch the exception and do something else, the IHost isntance that came out of Build is never observed by the application.

Yeah, I wasn't imagining they continue normally, more like they have some complex error handling code that will be running in parallel.

the application never gets access to the IHost instance

[Joking] What do you mean? This PR just added the official mechanism that lets everyone access IHost without the pesky inconvenience of needing Build() to return it to you ;p

eerhardt · 2021-06-08T15:38:05Z

src/libraries/Microsoft.Extensions.HostFactoryResolver/src/HostFactoryResolver.cs

@@ -31,6 +40,35 @@ internal sealed class HostFactoryResolver
            return ResolveFactory<THostBuilder>(assembly, CreateHostBuilder);
        }

+        public static Func<string[], object>? ResolveHostFactory(Assembly assembly, TimeSpan? waitTimeout = null, bool stopApplication = true, Action<object>? configureHostBuilder = null)


I don't see a test that uses configureHostBuilder. Do we need this parameter?

Yes, we will need it for https://github.com/dotnet/aspnetcore/blob/main/src/Mvc/Mvc.Testing/src/WebApplicationFactory.cs

Ok, then it would be great if we had a test ensuring it doesn't break.

2 things:

As of right now, ASP.NET Core will need to use it so it kinda has to work or tests will fail regardless 😄.

I agree with you and will add a test.

davidfowl · 2021-06-08T15:48:32Z

@eerhardt and @noahfalk I need to get this merged so I can react in several places down stream. I will react to your feedback in another PR so keep commenting.

dotnet-issue-labeler bot added the area-Extensions-Hosting label Jun 5, 2021

davidfowl requested review from eerhardt, maryamariyan and noahfalk June 5, 2021 03:50

davidfowl added 3 commits June 4, 2021 23:46

Handle linker warnings

ec7831b

Remove DAM annotations

d6cded5

Add support for the new pattern in HostFactoryResolver

1c7b509

- Added support for resolving an IServiceProvider using the new diagnostics source pattern (which doesn't require a different entrypoint)

davidfowl commented Jun 6, 2021

View reviewed changes

src/libraries/Microsoft.Extensions.HostFactoryResolver/src/HostFactoryResolver.cs Outdated Show resolved Hide resolved

davidfowl commented Jun 6, 2021

View reviewed changes

src/libraries/Microsoft.Extensions.HostFactoryResolver/src/HostFactoryResolver.cs Outdated Show resolved Hide resolved

davidfowl added 5 commits June 5, 2021 23:18

Made fixes and added more tests

d2b5678

Handle main with no args

99d2a77

Make sure we copy properties over

250c87d

Store the entrypoint directly

08729fe

Added tests for top level statements

e592f10

davidfowl marked this pull request as ready for review June 6, 2021 07:36

davidfowl added 3 commits June 6, 2021 08:34

Mark tests that require threading

e56785a

Run tests with RemoteExecutor since they rely on a global event

19e412e

- DiagnosticSources will publish events globally and we need to make sure there's no cross talk between tests

Increase the timeout

12ae4e9

davidfowl requested a review from halter73 June 7, 2021 04:29

eerhardt reviewed Jun 7, 2021

View reviewed changes

src/libraries/Microsoft.Extensions.Hosting/src/HostBuilder.cs Show resolved Hide resolved

eerhardt reviewed Jun 7, 2021

View reviewed changes

Added test for events

ad5c27e

eerhardt reviewed Jun 7, 2021

View reviewed changes

src/libraries/Microsoft.Extensions.HostFactoryResolver/src/HostFactoryResolver.cs Outdated Show resolved Hide resolved

eerhardt approved these changes Jun 7, 2021

View reviewed changes

bricelam approved these changes Jun 7, 2021

View reviewed changes

halter73 reviewed Jun 7, 2021

View reviewed changes

src/libraries/Microsoft.Extensions.HostFactoryResolver/src/HostFactoryResolver.cs Show resolved Hide resolved

halter73 reviewed Jun 7, 2021

View reviewed changes

src/libraries/Microsoft.Extensions.Hosting/src/HostBuilder.cs Show resolved Hide resolved

halter73 approved these changes Jun 7, 2021

View reviewed changes

davidfowl added 4 commits June 7, 2021 16:00

Remove dependency on hosting in shared file

7b01d0b

Actually remove hosting dependency

f8f2a69

Detect the version of Microsoft.Extensions.Hosting before waiting for…

5625ba5

… events to fire - We want to fail fast if we know this version of hosting will never fire events of if the hosting assembly fails to load. Added a version check. - Allow the caller to specify a wait timeout.

Added a flag to allow running the application to completion

315575d

noahfalk reviewed Jun 8, 2021

View reviewed changes

eerhardt reviewed Jun 8, 2021

View reviewed changes

davidfowl merged commit 543a983 into main Jun 8, 2021

eerhardt deleted the davidfowl/fire-hosting-events branch June 8, 2021 15:52

davidfowl added this to the 6.0.0 milestone Jun 9, 2021

This was referenced Jun 9, 2021

Remove unused usings #53938

Merged

Follow up on HostFactoryResolver changes #54052

Merged

ResolveServiceProviderFactory should work with Minimal Hosting dotnet/aspnetcore#31861

Closed

ghost locked as resolved and limited conversation to collaborators Jul 11, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fire diagnostic source events from IHostBuilder.Build #53757

Fire diagnostic source events from IHostBuilder.Build #53757

davidfowl commented Jun 5, 2021 •

edited

Loading

ghost commented Jun 5, 2021

davidfowl Jun 6, 2021

martincostello commented Jun 7, 2021

davidfowl commented Jun 7, 2021

martincostello commented Jun 7, 2021

eerhardt Jun 7, 2021

davidfowl Jun 7, 2021

eerhardt Jun 7, 2021

eerhardt Jun 7, 2021

davidfowl Jun 7, 2021 •

edited

Loading

noahfalk Jun 8, 2021

eerhardt left a comment

eerhardt Jun 7, 2021

davidfowl Jun 7, 2021

noahfalk Jun 8, 2021

davidfowl Jun 8, 2021

bricelam left a comment

noahfalk Jun 8, 2021

noahfalk Jun 8, 2021

noahfalk Jun 8, 2021

davidfowl Jun 8, 2021 •

edited

Loading

davidfowl Jun 11, 2021

noahfalk Jun 11, 2021

eerhardt Jun 8, 2021

davidfowl Jun 8, 2021

eerhardt Jun 8, 2021

davidfowl Jun 8, 2021

davidfowl commented Jun 8, 2021

	// even have a change of firing (they were adding in .NET >= 6)
	// even have a chance of firing (they were added in .NET >= 6)

Fire diagnostic source events from IHostBuilder.Build #53757

Fire diagnostic source events from IHostBuilder.Build #53757

Conversation

davidfowl commented Jun 5, 2021 • edited Loading

ghost commented Jun 5, 2021

Choose a reason for hiding this comment

martincostello commented Jun 7, 2021

davidfowl commented Jun 7, 2021

martincostello commented Jun 7, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

davidfowl Jun 7, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

eerhardt left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bricelam left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

davidfowl Jun 8, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

davidfowl commented Jun 8, 2021

davidfowl commented Jun 5, 2021 •

edited

Loading

davidfowl Jun 7, 2021 •

edited

Loading

davidfowl Jun 8, 2021 •

edited

Loading