Rework API to better work with async and sync calls

[robhruska]

These changes will all be backwards compatible, so this ought to result in a minor version update where clients can move code from the old Command over to the two new types at their leisure. Not sure if Command will get an [Obsolete] tag yet, though.

For an example of some of the API changes, check out Example.cs

Code Review: Two good entry points for review are CommandInvoker.cs and BaseCommand.cs. CommandInvoker delegates to a couple other invokers (BulkheadInvoker and BreakerInvoker) that could use review as well.

Separate sync/async Command APIs

Instead of Invoke() and InvokeAsync() on Command, implementations need to inherit from SyncCommand or AsyncCommand. This avoids the forced async-to-sync that's built into the library, and avoids the need for callers to make problematic async-to-sync conversions. Though convenient for callers, it's often a source of confusion, and also means that Mjolnir is more inefficient with async than it ought to be.

Currently, Commands are used like this:

// MyCommand : Command<TResult>

var command = new MyCommand(foo, bar, baz);
var value = await command.InvokeAsync();
// or
var value = command.Invoke(); // Sync equivalent.

This PR offers an alternative API:

var invoker = new CommandInvoker(); // Injectable via ICommandInvoker

// MyAsyncCommand : AsyncCommand<TResult>

var command = new MyAsyncCommand(foo, bar, baz);
var value = await invoker.InvokeThrowAsync(command, 1000); // Throws exceptions

var command = new MyAsyncCommand(fool, bar, baz);
var wrapped = await invoker.InvokeReturnAsync(command, 1000); // Wraps exceptions and returns
var value = wrapped.Value;

// MySyncCommand : SyncCommand<TResult>

var command = new MySyncCommand(foo, bar, baz);
var result = invoker.InvokeReturn(command);
var value = result.Value;

Throw and Return variants

The Return variants will catch and wrap Exceptions, preferring to always return a result to the caller (even when errors occur). Since Mjolnir's intent is to introduce fast failure to avoid failure cascading in unpredictable ways, it's useful for callers to handle that failure in ways beyond just re-throwing the exception upward. This could be handled by the caller using try/catch, but baking it into the Mjolnir API helps callers realize that they need to think about failure and make conscious decisions about how to handle it.

The Return variant returns a CommandResult<TResult> instead of just TResult. The wrapper contains the result or the causing exception if the command failed. The Throw method overrides don't return a wrapped result since it would add an unnecessary "unwrapping" step for callers.

Timeout override

Default and configured timeouts still apply, but callers can also provide a per-call timeout if they desire more lenient/rigid SLAs.

Improved testability

This will also make commands more unit-testable, since ICommandInvoker will be injectable (where injecting Command behavior is currently a challenge).

Use semaphore bulkheads (without queues) by default

I'm not sold on the use of thread pools as a bulkhead. Their strongest advantage is the ability to have a small queue in front that allows for absorbing some latency and bursts. However, there are a number of disadvantages.

• They're unoptimal for async. Fully async code means that threads are returned to the thread pool until the async work is done. Introducing an entire thread to "wrap" that and limit concurrency feels nonsensical to me.
• SmartThreadPool is a bit rough. We currently have to deliver our own forked copy of STP with Mjolnir because the main repo has gone stale and not merged PRs with functionality that we need. We've also had struggles with context flowing across STP threads, and the library isn't really well-equipped for async work.
• Queuing and pooling makes Mjolnir harder to reason about. Since the message gets queued off the current thread, keeping track of it and getting the result back to the appropriate context is a bit tricky. It works, but means a fair amount of hackery around delegates and WorkItems. It also means that we've forced both synchronous and async code down the same code path, which makes the async bits a lot heavier (see bullet 1).

I'd like to try using semaphores as the default bulkhead. It does mean (for now, at least) getting rid of the queues, but they can be re-introduced if needed in the future.

Better interface for hooking into metrics

The existing interface (IStats ^[docs]) is pretty vague and free-form, and makes it difficult to adapt events into collectors that use standardized metric types like timers and meters.

A new interface, IMetricEvents, is available that should make that easier. It provides specific methods for events that occur, and recommends a metric type for each. A couple of example methods:

void CommandInvoked(string commandName, double invokeMillis, double executeMillis, string status, string failureAction);
void RejectedByBulkhead(string bulkheadName, string commandName);
void BulkheadConfigGauge(string bulkheadName, string bulkheadType, int maxConcurrent);

The original IStats were targeted both at profiling and operational events; IMetricEvents focuses less on method profiling and more on relevant bulkhead, breaker, and command events. Parity between IStats and IMetricEvents wasn't a goal, so they're fairly different.

The new BaseCommand/CommandInvoker pipeline will not publish the old IStats events.

The adapter we use will also be made available (once complete).

For Release Notes

A list of items that should end up in the release notes.

Differences between old and new Command

• The Invoke INFO log is Invoke for both sync and async calls (Invoke Command={0} Breaker={1} Pool={2} Timeout={3}). In the old Command, it's InvokeAsync for the async call.
• Exceptions aren't wrapped in a CommandFailedException. Instead, the root cause exception is simply rethrown with some Command-specific data attached to it.
• "Pool" is now "Bulkhead" in a handful of configs, logs, and properties. If porting old Commands over to the new base classes, be sure to duplicate any configured pool values with the "bulkhead" configuration key.
• New commands (sync/async) have a timeout configuration key that starts with mjolnir.command. The old commands were inconsistent with the rest of the library and just started with command.. Examples:
  • Old: command.my-group.MyCommand.Timeout=1000
  • New: mjolnir.command.my-group.MyCommand.Timeout=1000
• New commands default to a 2000ms timeout instead of 15000ms.

Possible Remaining Work

• [Command] support for sync and async commands
  • This may go into a future release that also involves pulling the invoker out of the main Mjolnir project and into its own (e.g. Hudl.Mjolnir.Interceptor), which would help get rid of a dependency on Castle.Core.
• Ability to set separate bulkhead and breaker groups on new commands
• Support for returning Task (non-generic) from execute methods
  • This is tricky - if anyone's got suggestions on how to do it without heavily duplicating code paths, I'd love some ideas. Everything I've tried ends up kind of lame.
• Ability to roll out commands in a "dry-run" or "measure-only" mode, where nothing will get rejected, but MetricEvents will still fire. This would allow tuning before committing to a set of failure thresholds.

[lewislabs]

Where are you passing through the Command's CancellationToken? i.e. if one has been explicitly passed into the call.
This might be happening somewhere else, but I can't find it at quick glance.

[robhruska]

The GetActualTimeout() call on L61 should get the right value given configuration and default values - is that what you're referring to? I'm still kind of rethinking this a bit, too, for testing and logging purposes.

[lewislabs]

Ignore me, I was thinking that there was a user specified CancellationToken passed through here, but actually there's not.....carry on 👀

[lewislabs]

This looks so much cleaner and easier to debug!

[deflume1]

Could we maybe rename this one to _metricsTimer, and the other one to _statsTimer? I was confused about why we had two, until I read the comment below.

[robhruska]

👍

[robhruska]

Renamed these.

[deflume1]

This looks sweet, nice work.

[lewislabs]

Maybe call the method below? Also I think we spoke about PublicationOnly before, should we make the default ExecutionAndPublication, since the most common use case is to not allow the valueFactory to be called more than once.

[robhruska]

Yeah, I might even get rid of this one altogether and have the caller make a decision about the concurrency they'd like.

[lewislabs]

General comment: It looks like the new default timeout will be 2000ms, rather than 15000. Should it be clear somewhere in code/release notes that this is changing when you switch to the new CommandInvoker approach?

[robhruska]

@lewislabs Yeah, good call. I added a bullet to the release notes. Any thoughts on the change to 2000 as a default?

[lewislabs]

I think it's a good idea to bring it down. Did @marcusdarmstrong have some analysis on timeouts that we could use for a suitable default?

[tlil]

Do we want to rename this to CountAvailable or something indicative of it being a count?

[robhruska]

👍

[robhruska]

Renamed to CountAvailable.