amoerie / dead-man-switch Goto Github PK

@brandondahler suggested the following:

IDeadManSwitchLoggerFactory_ and IDeadManSwitchLogger are required (must not be null), but not provided for non-AspNetCore package. Honestly this is super annoying as a consumer, why should I have to implement your logging interfaces if I just don't care about the logging and want throw all of the messages away? I'd recommend allowing null to be passed for those and refactor to handle them as such.

My take on this would be to implement a silent logger that simply does nothing, and use this logger by default when the consumer does not provide one.

@brandondahler suggested the following:

public CancellationTokenSource CancellationTokenSource { get; set; }_ on DeadManSwitchContext feels dirty to me -- you're allowing external classes directly modify and/or replace mutable state that is fetched and and used by other external classes. Surely there's a better way to achieve this.

The triggerer class is currently responsible for canceling the CTS and swapping it with a new one (necessary for infinite workers). It would make more sense that the triggerer also creates the initial CTS and that the CTS is moved of the Context class.

@brandondahler suggested the following:

• The channel usage feels unnecessary -- notifications would be better stored in a ring buffer with proper locking, statuses would be better stored as a class member that is properly atomically modified.
• The unnecessary channel usage is driving the public API to be unnecessarily async-based. Remember that the underlying reason why async-await exists is so that we don't block threads while waiting for external I/O to occur. If there's no I/O (possibly) occurring, in theory I wouldn't expect to see async methods (yes this is an over-simplification).
• Channel usage is also driving how we're having to reset our death timer, making us have to wake up for every status update on our background thread, even if there was little to no time that passed between notifications. If we pretend that the timeout is 30s and that the loop inside the work only takes 100ms on average, we'd be queueing (on the worker thread) and dequeueing (on the watcher thread) 300 times every interval. See below for recommended approach that would reduce this to 1 per interval.

Taken from the Reddit discussion, @brandondahler suggests to simplify the usage of this library by exposing a method akin to Task.Run:

• It feels like we've made the problem more complicated than it is. It wasn't immediately apparent to me why there existed the main class DeadManSwitch, followed by DMSOptions, Runner, Worker, Context, Notification, Session, SessionFactory, Triggerer, and Watcher in order to achieve the goal.
• To use this library we have to 1) create an implementation of IDeadManSwitchWorker, 2) resolve a IDeadManSwitchRunner, 3) define a new DeadManSwitchOptions (not a big deal, but sugar overloads would be nice), and finally 4) call runner.RunAsync. A more ideal API would be for us to provide a Action<IDeadManSwitch, CancellationToken> or Func<IDeadManSwitch, CancellationToken, Task> to a DeadManSwitch.RunAsync(Func<...>, DeadManSwitchOptions, CancellationToken) - following a somewhat similar pattern that Task.Run(...) has.

@brandondahler suggested the following:

• Nit - public DateTime Timestamp { get; } in DeadManSwitchNotification ought to be DateTimeOffset so that we're storing unambiguous timestamps. If you disagree, we should at least be using DateTime.UtcNow instead of DateTime.Now.

We can convert the timestamps to local time when displaying them in the logs