Always log startup exceptions

[cwe1ss]

The following sample currently does not send the exception to the logger:

public class Program
{
    public static void Main(string[] args)
    {
        new WebHostBuilder()
            .UseKestrel()
            .ConfigureLogging(logging =>
             {
                 logging.AddConsole();
             })
            .Configure(app =>
            {
                throw new InvalidOperationException("Something went wrong!");
            })
            .Build()
            .Run();
    }
}

It will send it if CaptureStartupErrors(true) is called. However, this will also keep the application running and display an error to the user.

I'm using Service Fabric so I want the application to crash in order to let Service Fabric know that something is wrong. And I also want the exception in my logger.

Before logging was using DI, I created my own logger factory and caught startup exceptions manually (with some ugly code). This is no longer possible/reasonable now.

This PR would always send startup exceptions to the logger, no matter what value CaptureStartupErrors has.

We already discussed this in #622 and one concern was that this is a case of a "logging and re-throw anti-pattern" however I think that this is acceptable in this case as this is a very common use-case (IMO). Also, due to recent changes (e.g. logging DI) there's less and less reasons to have logic outside of the WebHostBuilder that would want to prevent that log message.

[davidfowl]

Any reason you moved this?

[cwe1ss]

It wasn't called before when CaptureStartupErrors=false. Since it re-throws, there's no need to create the server.

[davidfowl]

Add the braces

[cwe1ss]

I fixed the braces - sorry for that!

[Tratcher]

We don't still want the ConsoleWriteline do we?

[cwe1ss]

I'd keep it as people probably don't use the Console Logger in non-development environments and having the console output as an additional troubleshooting mechanism could be helpful.

E.g. in Service Fabric you can redirect console output to a file - by keeping this Console.WriteLine one would at least get the startup messages ("Listening on...") and any startup exceptions.

[davidfowl]

This looks good to me.

[davidfowl]

This is something we might want to back port to 1.x.

/cc @DamianEdwards @Eilon @muratg

[muratg]

@jkotalik could you file two bugs to bring this back to 1.0.x and 1.1.x?

[jkotalik]

#1159 and #1160. Once fully approved I'll add this to 2.0.x, 1.1.x, and 1.0.x

[Tratcher]

@jkotalik please review and merge.

[jkotalik]

@cwe1ss Would it be possible to add tests for this change?

[cwe1ss]

@jkotalik I added two tests. Hope they are ok?!

[jkotalik]

Looks good! @muratg @Eilon how do you want to handle this for 2.0.x? As a patch?

[jkotalik]

Merged with: 07f96a4

[Eilon]

If I understand correctly, this has a simple workaround: just enable the setting, right? If that's the case, then this isn't particularly suitable for a patch candidate.

[Tratcher]

The setting has significant side effects. It keeps your app running and serves up an error page rather than letting it crash and restart.

[Eilon]

And we'd want to turn that behavior on by default? That's a pretty significant breaking change, no? If another process is monitoring the Kestrel process (dotnet.exe), it won't know that it's dead and it won't spin a new one up... is that correct?

[Tratcher]

This change doesn't turn that behavior on by default, it only moves things around so that the logging happens prior to the crash. Before no logging would happen for the crash scenario.

[Eilon]

Ok sorry I'm not quite understanding this. What behavior exactly are we considering porting to 1.0.x/1.1.x? What exactly would it do by default that is different from what it used to do?

[Eilon]

Ok just synced with @Tratcher on IM. We're only changing that the logging will still happen. No change to whether the server lives or dies.

[cwe1ss]

Thank you very much for accepting this PR!! 👍