Catch startup exceptions and show them in the browser. #370
Conversation
|
So pretty |
|
RE the content-negotiation question: That sounds really nice actually. In the case that the client doesn't send an |
|
RE content-negotiation: no 😄 |
|
What did we decide to do RE making this easier to debug in non-development environment? Where does this message go in that case? |
|
Spoke to @Tratcher, decided we should also print this message (sans-HTML) to stdout. With the HttpPlatformHandler config this will go to the file so you can see the error easily in Azure, etc. |
|
Sweet! We need to figure out if we want to use development as the only switch. We wouldn't want people to change the env to Development do we? |
|
@davidfowl we would not. Are you saying there should be a knob to allow show detailed errors that overrides the development check? Or are you referring to the stdout printing? |
|
Yep. That horrible one we had before ASPNET_DETAILED_ERRORS=true |
|
Yep sounds good. Can you add that pls @Tratcher? |
| } | ||
|
|
||
|
|
||
| #if !DNXCORE50 |
There was a problem hiding this comment.
They added StackFrame to CoreCLR https://www.nuget.org/packages/System.Diagnostics.StackTrace/4.0.1-beta-23225 though you;ll have to see if it's usable 😄
Tratcher
force-pushed
the
tratcher/errorpage
branch
from
36dc40d to
7c2c649
Compare
|
Updated |
| public FailSafeStartup(bool showDetails, IRuntimeEnvironment env, Exception ex) | ||
| { | ||
| // Write errors to standard out so they can be retrieved when not in development mode. | ||
| Console.Out.WriteLine("Application startup exception: " + ex.ToString()); |
There was a problem hiding this comment.
Log as well. One thing we said was that logging is always good to do. Console.WriteLine is the thing to question.
There was a problem hiding this comment.
This can't log, logging is app configured and probably wasn't set up yet.
There was a problem hiding this comment.
You can log. Hosting creates the logger factory.
There was a problem hiding this comment.
But the logs don't go anywhere until the app configures them, and the app failed to start.
There was a problem hiding this comment.
Don't assume that, we should just log. You can imagine hosting registering an EventLogProvider on windows so that critical exceptions like this show up there.
|
Updated. |
| _runtimeEnv = runtimeEnv; | ||
| _loggerFactory = loggerFactory; | ||
| ShowDetailedErrors = hostingEnv.IsDevelopment() | ||
| || string.Equals("true", Environment.GetEnvironmentVariable("ASPNET_DETAILED_ERRORS"), StringComparison.OrdinalIgnoreCase); |
Tratcher
force-pushed
the
tratcher/errorpage
branch
from
4c7621d to
0760b64
Compare
|
Redone. |
|
|
||
| // Write errors to standard out so they can be retrieved when not in development mode. | ||
| Console.Out.WriteLine("Application startup exception: " + ex.ToString()); | ||
| var logger = _applicationServices.GetRequiredService<ILogger<HostingEngine>>(); |
There was a problem hiding this comment.
Dont use Logger<T> it's hella inefficient (unfortunately) 😄. Get the LoggerFactory
There was a problem hiding this comment.
Its like LINQ, nice to use in some cases but mostly easy to do something in efficient. It's worth trying to see why it's so slow. TBH tho, this code path is error path so it doesn't matter that much.
There was a problem hiding this comment.
Plus you'll make @DamianEdwards cry, but thats ok
There was a problem hiding this comment.
This executes once per app and only in a catastrophic failure scenario, forget perf.
There was a problem hiding this comment.
What happened to my beloved ILogger<T>?!?! 😭
There was a problem hiding this comment.
I say keep ILogger<HostingEngine> since perf is not that important in the failure case.
|
minor changes, this change looks much better |
|
Updated. |
|
ping @davidfowl |
|
|
|
Thought you already merged this change |
|
😄 |
Tratcher
force-pushed
the
tratcher/errorpage
branch
from
6da327e to
a9e7948
Compare
#77 @davidfowl @DamianEdwards @muratg @Eilon
Catch any application startup related exceptions and display them in the browser rather than on the console. Note this only applies to Hosting.Program.Main scenarios, not to self-hosted WebHostBuilder or to TestHost
Handled exceptions:
The error details are only displayed if your in Development. Otherwise you get:
There is one open question:
Should we attempt any content-negotiation to avoid displaying HTML to non-browser clients?