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

StopTheHostException should be made public to be dealt with gracefully #60600

Closed
cristalink opened this issue Oct 19, 2021 · 19 comments · Fixed by #69404
Closed

StopTheHostException should be made public to be dealt with gracefully #60600

cristalink opened this issue Oct 19, 2021 · 19 comments · Fixed by #69404
Assignees
Labels
api-approved API was approved in API review, it can be implemented area-Extensions-Hosting help wanted [up-for-grabs] Good issue for external contributors
Milestone

Comments

@cristalink
Copy link

cristalink commented Oct 19, 2021

Description

In brief, StopTheHostException should be made public as it may need to be handled by framework users.

This commit introduced StopTheHostException thrown out of, ultimately, HostBuilder.Build() on, among other things, adding a migration to a EF/.NET Core 6.0 RC2 project.

The exception itself seems to be expected, and is supposed to be caught by HostFactoryResolver.

However, that private, formally undocumented exception reaches my code which I believe is quite a common pattern:

try
{
   CreateHostBuilder().Build().Run()
}
catch (Exception ex)
{
   _logger.Fatal(ex, "Unhandled exception");
   return 1;
}

The following output is produced on an attempt to add a migration:

PM> Add-Migration InitialCreate -Context PersistedGrantDbContext -OutputDir Migrations\PersistedGrantDbContext
Build started...
Build succeeded.
18:21:59 [FTL] Unhandled exception
Microsoft.Extensions.Hosting.HostFactoryResolver+HostingListener+StopTheHostException: Exception of type 'Microsoft.Extensions.Hosting.HostFactoryResolver+HostingListener+StopTheHostException' was thrown.
at Microsoft.Extensions.Hosting.HostFactoryResolver.HostingListener.OnNext(KeyValuePair`2 value)
at System.Diagnostics.DiagnosticListener.Write(String name, Object value)
at Microsoft.Extensions.Hosting.HostBuilder.Build()

To undo this action, use Remove-Migration.
PM>

Reproduction Steps

(1) Add try/catch similar to the above around IHostBulder.Build() in any .NET/EF Core 6.0 RC2 project,
(2) Attempt to add a migration.

Expected behavior

StopTheHostException needs to be public and documented, to be able to be gracefully re-thrown. Alternatively, it should never propagate out of the framework.

Actual behavior

StopTheHostException is private, undocumented, and reaches customer code.

Regression?

Yes - introduced in .NET Core 6.

Known Workarounds

I fixed the issue with the following:

catch (Exception ex)
{
   string type = ex.GetType().Name;
   if (type.Equals("StopTheHostException", StringComparison.Ordinal))
   {
      throw;
   }

   _logger.Fatal(ex, "Unhandled exception");
   return 1;
}

Normally, I would use catch(StopTheHostException) or similar but I can't because StopTheHostException is private.

Configuration

Any ASP.NET Core/EF project, and potentially other kinds of projects.

Other information

No response

@dotnet-issue-labeler dotnet-issue-labeler bot added area-Extensions-Hosting untriaged New issue has not been triaged by the area owner labels Oct 19, 2021
@eerhardt
Copy link
Member

@davidfowl - thoughts here? My assumption is that StopTheHostException is supposed to be an internal implementation detail.

@maryamariyan maryamariyan removed the untriaged New issue has not been triaged by the area owner label Jan 11, 2022
@maryamariyan maryamariyan added this to the Future milestone Jan 11, 2022
josephdecock added a commit to josephdecock/Samples that referenced this issue Jan 14, 2022
The EF CLI throws a starts up the host to read database configuration and then throws a StopTheHost exception to stop the host from doing anything else. Check for that expected exception so that it is not logged as a fatal error.

See also dotnet/runtime#60600.
@sommmen
Copy link

sommmen commented Mar 8, 2022

@davidfowl - thoughts here? My assumption is that StopTheHostException is supposed to be an internal implementation detail.

Any idea on what the proper way to handle this would be?

my code is now:

protected void CreateHost()
{
    if (HostBuilder == null)
        throw new InvalidOperationException(nameof(HostBuilder) + " was null");

    try
    {
        Host = HostBuilder.Build();
            
        // Release resources
        HostBuilder = null;
    }
    catch (Exception e) when(e is not OperationCanceledException && e.GetType().Name != "StopTheHostException")
    {
        _logger?.LogCritical(e, "Caught exception whilst building Host");
        throw;
    }
}

I'm running into this ex with a custom bootstrapper and ef core - would this be the proper way to handle this?

@davidfowl
Copy link
Member

@eerhardt we need to expose something basic for this scenario either an interface or the exception itself if we want to avoid code like above. The scenarios are pretty narrow though (avoiding the log for when migrations happen). We could at a minimum document the current behavior.

@maryamariyan maryamariyan modified the milestones: Future, 7.0.0 Mar 8, 2022
@maryamariyan maryamariyan self-assigned this Mar 8, 2022
@nanasi880
Copy link

The scenarios are pretty narrow though (avoiding the log for when migrations happen)

This is extremely important.
The worst operation is to say, "I get an error log, but please ignore this." (and people will stop trusting the logs)

As long as you throw an exception, you can only expose that exception.
Don't document the behavior; expose the exception.

This is clearly a design error, but fortunately the problem can be easily solved by simply exposing the exception.

@davidfowl
Copy link
Member

When do you see this exception happen in practice? Locally during development right when adding Migrations right? Narrow doesn't mean that its not important, but if there are more cases than we're aware of, that would be nice to know.

@jasonthorsness
Copy link

@nanasi880 the HostFactoryResolver prefers the older patterns; I worked around this exception issue by just moving my IHostBuilder creation to a static Program.CreateHostBuilder for EF to find.

// Prefer the older patterns by default for back compat.

In my case this was a CLI tool using System.CommandLine.Hosting which kept dumping StopTheHostException to the console after the ef migration was successfully created, it's a bit off the beaten path. However I've been frustrated in the past by unhandleable internal exceptions, since this can escape the platform code maybe it should be public.

@nanasi880
Copy link

Thanks reply.

@davidfowl
This exception occurred only during development and was specifically thrown when the following command was executed
dotnet ef migrations add {parameters...}

@jasonthorsness
This is very interesting information.
I am currently running ASP.NetCore in a subcommand using the Microsoft.Extensions.CommandLineUtils package.

IHostBuilder creation to a static Program.CreateHostBuilder for EF to find.

This means that from the Program class (the Main method of the entry point, to be precise), without ever going through an instance method (perhaps even dynamic branching by if is not allowed?). Is it necessary to write code to use IHostBuilder?

In that case, it may not be practical to use this, as I prefer to use a model where subcommands distribute multiple functions (e.g., only perform migrations)

@eerhardt
Copy link
Member

This seems to be a pretty common problem. I agree we should do something about it.

At this point I think it would make the most sense to make the exception public. (We should probably give it a better name if we are going that route.) @davidfowl - would you be OK with that? I assume this pattern is here to stay and won't be replaced anytime soon, so exposing the exception shouldn't be a big issue. Or are there plans to replace/change the HostFactoryResolver?

@davidfowl
Copy link
Member

davidfowl commented Mar 14, 2022

OK lets expose the exception. I'm sympathetic to the problems experienced here as well.

@maryamariyan
Copy link
Member

Setting the API proposal in Microsoft.Extensions.Hosting:

namespace Microsoft.Extensions.Hosting
{
    public partial class StopTheHostException : Exception
    {
        public StopTheHostException() { }
        protected StopTheHostException(System.Runtime.Serialization.SerializationInfo info, System.Runtime.Serialization.StreamingContext context) { }
        public StopTheHostException(string? message) { }
        public StopTheHostException(string? message, System.Exception? innerException) { }
    }
}

@maryamariyan maryamariyan added the api-ready-for-review API is ready for review, it is NOT ready for implementation label Mar 14, 2022
@bartonjs
Copy link
Member

bartonjs commented Mar 15, 2022

Video

We renamed StopTheHostException to HostAbortedException now that it's a public type. (And sealed it, and added [Serializable] for .NET Framework interop concerns)

namespace Microsoft.Extensions.Hosting
{
    [Serializable]
    public sealed partial class HostAbortedException : Exception
    {
        public HostAbortedException() { }
        private HostAbortedException(System.Runtime.Serialization.SerializationInfo info, System.Runtime.Serialization.StreamingContext context) { }
        public HostAbortedException(string? message) { }
        public HostAbortedException(string? message, System.Exception? innerException) { }
    }
}

@bartonjs bartonjs added api-approved API was approved in API review, it can be implemented and removed api-ready-for-review API is ready for review, it is NOT ready for implementation labels Mar 15, 2022
@deeprobin
Copy link
Contributor

deeprobin commented Apr 3, 2022

Seems to be a small issue.

I think I would have time for that. If you are interested in a community contribution, feel free to assign me.
(see https://github.com/deeprobin/runtime/tree/issue-60600)

@maryamariyan maryamariyan added the help wanted [up-for-grabs] Good issue for external contributors label Apr 6, 2022
@maryamariyan
Copy link
Member

@deeprobin sure looking forward to your contribution

@deeprobin
Copy link
Contributor

@maryamariyan Thank you for waiting. I've been busy lately. The PR will be made this week.

@ghost ghost added the in-pr There is an active PR which will close this issue when it is merged label May 16, 2022
@deeprobin
Copy link
Contributor

@bartonjs

The proposal proposes a [Serializable] type.
However, since November 2021, no new Serializable type may be introduced - See BinaryFormatter obsoletion.

Was this an exception (if so, for what reason?)?

@bartonjs
Copy link
Member

Was this an exception (if so, for what reason?)?

Yes. To quote that document:

The exclusion is for dotnet org first-party projects which target netstandard2.0 or earlier. For these projects, Exception-derived types should continue to be annotated as [Serializable] so that they may be remoted across app domain boundaries. Any new Exception-derived types that are defined in a library which is not available to netstandard2.0 callers will not be annotated with [Serializable]. We should be following our own guidance on how to safely serialize exception information without involving BinaryFormatter.

(Emphasis mine.)

@deeprobin
Copy link
Contributor

deeprobin commented May 16, 2022

@bartonjs Thank you. Should we obsolete the private ctor?

@bartonjs
Copy link
Member

If that's being done for other Exception-derived types, then yes. If not, then no 😄. (I don't know off the top of my head, but that's the formula)

@deeprobin
Copy link
Contributor

@bartonjs I did a grep in the extension-packages

E:\external\dotnet\runtime\src\libraries>git grep -i "namespace Microsoft.Extensions" -- --files-with-matches *Exception*
Microsoft.Extensions.Configuration.FileExtensions/src/FileLoadExceptionContext.cs:namespace Microsoft.Extensions.Configuration
Microsoft.Extensions.Hosting/src/BackgroundServiceExceptionBehavior.cs:namespace Microsoft.Extensions.Hosting
Microsoft.Extensions.Hosting/src/HostAbortedException.cs:namespace Microsoft.Extensions.Hosting
Microsoft.Extensions.Hosting/tests/UnitTests/HostAbortedExceptionTests.cs:namespace Microsoft.Extensions.Hosting.Unit.Tests
Microsoft.Extensions.Logging.Abstractions/tests/Microsoft.Extensions.Logging.Generators.Tests/TestClasses/ExceptionTestExtensions.WithDiagnostics.cs:namespace Microsoft.Extensions.Logging.Generators.Tests.TestClasses
Microsoft.Extensions.Logging.Abstractions/tests/Microsoft.Extensions.Logging.Generators.Tests/TestClasses/ExceptionTestExtensions.cs:namespace Microsoft.Extensions.Logging.Generators.Tests.TestClasses
Microsoft.Extensions.Logging.EventSource/src/ExceptionInfo.cs:namespace Microsoft.Extensions.Logging.EventSource
Microsoft.Extensions.Options/src/OptionsValidationException.cs:namespace Microsoft.Extensions.Options

I only found OptionsValidationException which does not implement a private ctor.

Accordingly, there is no standard to follow.
Or do you know any other exception that we can use as an example here?

@ghost ghost removed the in-pr There is an active PR which will close this issue when it is merged label Jun 1, 2022
@ghost ghost locked as resolved and limited conversation to collaborators Jul 1, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
api-approved API was approved in API review, it can be implemented area-Extensions-Hosting help wanted [up-for-grabs] Good issue for external contributors
Projects
No open projects
Development

Successfully merging a pull request may close this issue.

9 participants