-
Notifications
You must be signed in to change notification settings - Fork 15
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
Make sure AkkaHostedService
failures are always visible
#494
Make sure AkkaHostedService
failures are always visible
#494
Conversation
close akkadotnet#470 - application crashes and exits should always be explicitly logged someplace where they can be seen regardless of logging configuration.
@@ -72,6 +72,9 @@ async Task TerminationHook() | |||
catch (Exception ex) | |||
{ | |||
Logger.Log(LogLevel.Critical, ex, "Unable to start AkkaHostedService - shutting down application"); | |||
|
|||
// resolve https://github.com/akkadotnet/Akka.Hosting/issues/470 - never allow failures to be silent | |||
Console.WriteLine($"Unable to start AkkaHostedService due to [{ex}] - shutting down application"); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ensure that this gets logged regardless of how the MSFT.EXT.Logger inputs have been configured
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think for exception, it should always be added at the end, because the stack trace will screw up the string formatting.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Or we can change that to "due to [{ex.Message}] - shutting down application.\nCause: {ex}"
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
or maybe we should just re-throw the exception as a new exception and let MS.EXT.Hosting deal with it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think this should be changed a bit
@@ -72,6 +72,9 @@ async Task TerminationHook() | |||
catch (Exception ex) | |||
{ | |||
Logger.Log(LogLevel.Critical, ex, "Unable to start AkkaHostedService - shutting down application"); | |||
|
|||
// resolve https://github.com/akkadotnet/Akka.Hosting/issues/470 - never allow failures to be silent | |||
Console.WriteLine($"Unable to start AkkaHostedService due to [{ex}] - shutting down application"); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think for exception, it should always be added at the end, because the stack trace will screw up the string formatting.
@@ -72,6 +72,9 @@ async Task TerminationHook() | |||
catch (Exception ex) | |||
{ | |||
Logger.Log(LogLevel.Critical, ex, "Unable to start AkkaHostedService - shutting down application"); | |||
|
|||
// resolve https://github.com/akkadotnet/Akka.Hosting/issues/470 - never allow failures to be silent | |||
Console.WriteLine($"Unable to start AkkaHostedService due to [{ex}] - shutting down application"); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Or we can change that to "due to [{ex.Message}] - shutting down application.\nCause: {ex}"
@@ -72,6 +72,9 @@ async Task TerminationHook() | |||
catch (Exception ex) | |||
{ | |||
Logger.Log(LogLevel.Critical, ex, "Unable to start AkkaHostedService - shutting down application"); | |||
|
|||
// resolve https://github.com/akkadotnet/Akka.Hosting/issues/470 - never allow failures to be silent | |||
Console.WriteLine($"Unable to start AkkaHostedService due to [{ex}] - shutting down application"); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
or maybe we should just re-throw the exception as a new exception and let MS.EXT.Hosting deal with it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Changes look good @Arkatufus
var capturedException = ExceptionDispatchInfo.Capture(ex); | ||
using var cts = new CancellationTokenSource(TimeSpan.FromSeconds(3)); | ||
await StopAsync(cts.Token); | ||
capturedException.Throw(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
Changes
close #470 - application crashes and exits should always be explicitly logged someplace where they can be seen regardless of logging configuration.
Checklist
For significant changes, please ensure that the following have been completed (delete if not relevant):
AkkaHostedService
get eaten / not logged #470