Provide error logs instead of exception spew on missing config #73
Conversation
|
Ugh, missed a case. Please hold off a moment. |
|
Ready for review now; sorry for the botch job. |
|
LGTM , Win32Exception on Linux 😆 . |
| @@ -39,19 +49,39 @@ public void Start(Channel c) | |||
| var port = c.PortID + Channel.EphemeralPortRange; | |||
|
|
|||
| var wagiProgram = WagiBinaryPath(); | |||
| var bindleUrl = Environment.GetEnvironmentVariable(ENV_BINDLE); | |||
|
|
|||
| if (string.IsNullOrWhiteSpace(bindleUrl)) | |||
There was a problem hiding this comment.
Should this be checked in the constructor instead of Start? The user won't know BINDLE_SERVER_URL is set until the first time a job is scheduled. With the current DataSeeder fixtures, an error will be reported during startup. But for "production" cases where no apps are installed, it will not report until they've registered their first app and uploaded their first few revisions.
There was a problem hiding this comment.
Alternatively, we could look up this environment variable during Startup. We may want to support more options than an environment variable in the future (e.g. from appsettings.json, or from a feature flag).
There was a problem hiding this comment.
I can check in the constructor but are you thinking throw in that case? The trouble is that will give us spew again, albeit one-off and terminal spew. I'm not sure if there is a way to tell ASP.NET to simply exit without blowing chunks...?
There was a problem hiding this comment.
I've added a check and critical log in the constructor, so you will now get an error log on startup even if there are no channels. I couldn't find a clean way to tell ASP.NET to terminate though.
There was a problem hiding this comment.
Ah yeah I was thinking about throwing, but if it means more spew... Eww.
| _wagiProcessIds[c.Id] = process.Id; | ||
| } | ||
| } | ||
| catch (Win32Exception e) // yes, even on Linux |
There was a problem hiding this comment.
It's for backward compatibility. This API dates from 2002 and the exception types were documented then; changing them just for six new versions and a different OS would break existing code. Microsoft does not fuck around when it comes to backcompat.
| @@ -39,19 +49,39 @@ public void Start(Channel c) | |||
| var port = c.PortID + Channel.EphemeralPortRange; | |||
|
|
|||
| var wagiProgram = WagiBinaryPath(); | |||
| var bindleUrl = Environment.GetEnvironmentVariable(ENV_BINDLE); | |||
|
|
|||
| if (string.IsNullOrWhiteSpace(bindleUrl)) | |||
There was a problem hiding this comment.
Ah yeah I was thinking about throwing, but if it means more spew... Eww.
Fixes #63.