-
Notifications
You must be signed in to change notification settings - Fork 4.9k
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
Fire diagnostic source events from IHostBuilder.Build #53757
Changes from all commits
42b36a1
ec7831b
d6cded5
1c7b509
d2b5678
99d2a77
250c87d
08729fe
e592f10
e56785a
19e412e
12ae4e9
ad5c27e
7b01d0b
f8f2a69
5625ba5
315575d
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change | ||||
---|---|---|---|---|---|---|
|
@@ -2,7 +2,13 @@ | |||||
// The .NET Foundation licenses this file to you under the MIT license. | ||||||
|
||||||
using System; | ||||||
using System.Collections.Generic; | ||||||
using System.Diagnostics; | ||||||
using System.Reflection; | ||||||
using System.Threading; | ||||||
using System.Threading.Tasks; | ||||||
using Microsoft.Extensions.Configuration; | ||||||
using Microsoft.Extensions.DependencyInjection; | ||||||
|
||||||
#nullable enable | ||||||
|
||||||
|
@@ -16,6 +22,9 @@ internal sealed class HostFactoryResolver | |||||
public const string CreateWebHostBuilder = nameof(CreateWebHostBuilder); | ||||||
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 = TimeSpan.FromSeconds(5); | ||||||
|
||||||
public static Func<string[], TWebHost>? ResolveWebHostFactory<TWebHost>(Assembly assembly) | ||||||
{ | ||||||
return ResolveFactory<TWebHost>(assembly, BuildWebHost); | ||||||
|
@@ -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) | ||||||
{ | ||||||
if (assembly.EntryPoint is null) | ||||||
{ | ||||||
return null; | ||||||
} | ||||||
|
||||||
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) | ||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Nit:
Suggested change
|
||||||
var hostingAssembly = Assembly.Load("Microsoft.Extensions.Hosting"); | ||||||
if (hostingAssembly.GetName().Version is Version version && version.Major < 6) | ||||||
{ | ||||||
return null; | ||||||
} | ||||||
|
||||||
// We're using a version >= 6 so the events can fire. If they don't fire | ||||||
// then it's because the application isn't using the hosting APIs | ||||||
} | ||||||
catch | ||||||
{ | ||||||
// There was an error loading the extensions assembly, return null. | ||||||
return null; | ||||||
} | ||||||
|
||||||
return args => new HostingListener(args, assembly.EntryPoint, waitTimeout ?? s_defaultWaitTimeout, stopApplication, configureHostBuilder).CreateHost(); | ||||||
} | ||||||
|
||||||
private static Func<string[], T>? ResolveFactory<T>(Assembly assembly, string name) | ||||||
{ | ||||||
var programType = assembly?.EntryPoint?.DeclaringType; | ||||||
|
@@ -58,7 +96,7 @@ private static bool IsFactory<TReturn>(MethodInfo? factory) | |||||
} | ||||||
|
||||||
// Used by EF tooling without any Hosting references. Looses some return type safety checks. | ||||||
public static Func<string[], IServiceProvider?>? ResolveServiceProviderFactory(Assembly assembly) | ||||||
public static Func<string[], IServiceProvider?>? ResolveServiceProviderFactory(Assembly assembly, TimeSpan? waitTimeout = null) | ||||||
{ | ||||||
// Prefer the older patterns by default for back compat. | ||||||
var webHostFactory = ResolveWebHostFactory<object>(assembly); | ||||||
|
@@ -93,6 +131,16 @@ private static bool IsFactory<TReturn>(MethodInfo? factory) | |||||
}; | ||||||
} | ||||||
|
||||||
var hostFactory = ResolveHostFactory(assembly, waitTimeout: waitTimeout); | ||||||
if (hostFactory != null) | ||||||
{ | ||||||
return args => | ||||||
{ | ||||||
var host = hostFactory(args); | ||||||
return GetServiceProvider(host); | ||||||
}; | ||||||
} | ||||||
|
||||||
return null; | ||||||
} | ||||||
|
||||||
|
@@ -112,5 +160,133 @@ private static bool IsFactory<TReturn>(MethodInfo? factory) | |||||
var servicesProperty = hostType.GetProperty("Services", DeclaredOnlyLookup); | ||||||
return (IServiceProvider?)servicesProperty?.GetValue(host); | ||||||
} | ||||||
|
||||||
private class HostingListener : IObserver<DiagnosticListener>, IObserver<KeyValuePair<string, object?>> | ||||||
{ | ||||||
private readonly string[] _args; | ||||||
private readonly MethodInfo _entryPoint; | ||||||
private readonly TimeSpan _waitTimeout; | ||||||
private readonly bool _stopApplication; | ||||||
|
||||||
private readonly TaskCompletionSource<object> _hostTcs = new(); | ||||||
private IDisposable? _disposable; | ||||||
private Action<object>? _configure; | ||||||
|
||||||
public HostingListener(string[] args, MethodInfo entryPoint, TimeSpan waitTimeout, bool stopApplication, Action<object>? configure) | ||||||
{ | ||||||
_args = args; | ||||||
_entryPoint = entryPoint; | ||||||
_waitTimeout = waitTimeout; | ||||||
_stopApplication = stopApplication; | ||||||
_configure = configure; | ||||||
} | ||||||
|
||||||
public object CreateHost() | ||||||
{ | ||||||
using var subscription = DiagnosticListener.AllListeners.Subscribe(this); | ||||||
|
||||||
// Kick off the entry point on a new thread so we don't block the current one | ||||||
// in case we need to timeout the execution | ||||||
var thread = new Thread(() => | ||||||
{ | ||||||
try | ||||||
{ | ||||||
var parameters = _entryPoint.GetParameters(); | ||||||
if (parameters.Length == 0) | ||||||
{ | ||||||
_entryPoint.Invoke(null, Array.Empty<object>()); | ||||||
} | ||||||
else | ||||||
{ | ||||||
_entryPoint.Invoke(null, new object[] { _args }); | ||||||
} | ||||||
|
||||||
// Try to set an exception if the entry point returns gracefully, this will force | ||||||
// build to throw | ||||||
_hostTcs.TrySetException(new InvalidOperationException("Unable to build IHost")); | ||||||
} | ||||||
catch (TargetInvocationException tie) when (tie.InnerException is StopTheHostException) | ||||||
{ | ||||||
// The host was stopped by our own logic | ||||||
} | ||||||
catch (TargetInvocationException tie) | ||||||
{ | ||||||
// Another exception happened, propagate that to the caller | ||||||
_hostTcs.TrySetException(tie.InnerException ?? tie); | ||||||
} | ||||||
catch (Exception ex) | ||||||
{ | ||||||
// Another exception happened, propagate that to the caller | ||||||
_hostTcs.TrySetException(ex); | ||||||
} | ||||||
}) | ||||||
{ | ||||||
// Make sure this doesn't hang the process | ||||||
IsBackground = true | ||||||
}; | ||||||
|
||||||
// Start the thread | ||||||
thread.Start(); | ||||||
|
||||||
try | ||||||
{ | ||||||
// Wait before throwing an exception | ||||||
if (!_hostTcs.Task.Wait(_waitTimeout)) | ||||||
{ | ||||||
throw new InvalidOperationException("Unable to build IHost"); | ||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I want the timeout to be an implementation detail. I'm also thinking this could also return null. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I need to check the callers but its possible null is a better return than throwing. |
||||||
} | ||||||
} | ||||||
catch (AggregateException) when (_hostTcs.Task.IsCompleted) | ||||||
{ | ||||||
// Lets this propagate out of the call to GetAwaiter().GetResult() | ||||||
} | ||||||
|
||||||
Debug.Assert(_hostTcs.Task.IsCompleted); | ||||||
halter73 marked this conversation as resolved.
Show resolved
Hide resolved
|
||||||
|
||||||
return _hostTcs.Task.GetAwaiter().GetResult(); | ||||||
} | ||||||
|
||||||
public void OnCompleted() | ||||||
{ | ||||||
_disposable?.Dispose(); | ||||||
} | ||||||
|
||||||
public void OnError(Exception error) | ||||||
{ | ||||||
|
||||||
} | ||||||
|
||||||
public void OnNext(DiagnosticListener value) | ||||||
{ | ||||||
if (value.Name == "Microsoft.Extensions.Hosting") | ||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This is where we wire up to the new event to intercept calls to build. |
||||||
{ | ||||||
_disposable = value.Subscribe(this); | ||||||
} | ||||||
} | ||||||
|
||||||
public void OnNext(KeyValuePair<string, object?> value) | ||||||
{ | ||||||
if (value.Key == "HostBuilding") | ||||||
{ | ||||||
_configure?.Invoke(value.Value!); | ||||||
} | ||||||
|
||||||
if (value.Key == "HostBuilt") | ||||||
{ | ||||||
_hostTcs.TrySetResult(value.Value!); | ||||||
|
||||||
if (_stopApplication) | ||||||
{ | ||||||
// Stop the host from running further | ||||||
throw new StopTheHostException(); | ||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
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. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Yeah, I wasn't imagining they continue normally, more like they have some complex error handling code that will be running in parallel.
[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 |
||||||
} | ||||||
} | ||||||
} | ||||||
|
||||||
private class StopTheHostException : Exception | ||||||
{ | ||||||
|
||||||
} | ||||||
} | ||||||
} | ||||||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't see a test that uses
configureHostBuilder
. Do we need this parameter?There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, we will need it for https://github.com/dotnet/aspnetcore/blob/main/src/Mvc/Mvc.Testing/src/WebApplicationFactory.cs
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ok, then it would be great if we had a test ensuring it doesn't break.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
2 things: