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

Fire diagnostic source events from IHostBuilder.Build #53757

Merged
merged 17 commits into from
Jun 8, 2021
Merged
Changes from 1 commit
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Prev Previous commit
Added a flag to allow running the application to completion
  • Loading branch information
davidfowl committed Jun 8, 2021
commit 315575d2ad957adb22379ecc2d1be27c82f46f42
Original file line number Diff line number Diff line change
Expand Up @@ -40,7 +40,7 @@ internal sealed class HostFactoryResolver
return ResolveFactory<THostBuilder>(assembly, CreateHostBuilder);
}

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

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?

Copy link
Member Author

Choose a reason for hiding this comment

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

Copy link
Member

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.

Copy link
Member Author

Choose a reason for hiding this comment

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

2 things:

  1. As of right now, ASP.NET Core will need to use it so it kinda has to work or tests will fail regardless 😄.
  2. I agree with you and will add a test.

{
if (assembly.EntryPoint is null)
{
Expand All @@ -66,7 +66,7 @@ internal sealed class HostFactoryResolver
return null;
}

return args => new HostingListener(args, assembly.EntryPoint, waitTimeout ?? s_defaultWaitTimeout, configureHostBuilder).CreateHost();
return args => new HostingListener(args, assembly.EntryPoint, waitTimeout ?? s_defaultWaitTimeout, stopApplication, configureHostBuilder).CreateHost();
}

private static Func<string[], T>? ResolveFactory<T>(Assembly assembly, string name)
Expand Down Expand Up @@ -166,16 +166,18 @@ private class HostingListener : IObserver<DiagnosticListener>, IObserver<KeyValu
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, 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;
}

Expand Down Expand Up @@ -273,8 +275,11 @@ public void OnNext(KeyValuePair<string, object?> value)
{
_hostTcs.TrySetResult(value.Value!);

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

Choose a reason for hiding this comment

The 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.

Copy link
Member Author

@davidfowl davidfowl Jun 8, 2021

Choose a reason for hiding this comment

The 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.

Copy link
Member Author

Choose a reason for hiding this comment

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

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.

Copy link
Member

Choose a reason for hiding this comment

The 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.

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

}
}
}

Expand Down