Warn me if my handler is async

[kkozmic]

public class MyHandler : IHandleCommand<MyMessage>
{
   public async void Handle(MyMessage busCommand)
   {
      throw new InvalidOperationException("HA HA HA, you can't catch me!");
   }
}

This message will be viewed as handled, and therefore not retried/put in error queue.

Perhaps Nimbus should enforce (throw upfront) when I have async implementation of the Handle method?

It's easy to miss (we did) and can cause a lot of pain.

[shiftkey]

What about making support for async Task handlers first-class so you can detect when faults occur?

[kkozmic]

@shiftkey that would double the number of IHandle* interfaces from 4 to 8.

[shiftkey]

OK, but given how easily you used async incorrectly perhaps it's worth letting people opt in.

Supporting both modes does require a bit more work, but you could always encapsulate your blocking handlers in a task like this:

var task = Task.Run(() => handler.Handle(command));

and have a common way of detecting faults using the TPL...

[gertjvr]

a possible interim solution could be a convention or approval tests to check for all IHandler that have a method with a async void Handler and warning the developer. Just an idea :)

[kkozmic]

I just built one for us :)

Krzysztof Kozmic

On Monday, 17 February 2014 at 11:51 am, Gert Jansen van Rensburg wrote:

a possible interim solution could be a convention or approval tests to check for all IHandler that have a method with a async void Handler and warning the developer. Just an idea :)

—
Reply to this email directly or view it on GitHub (https://github.com/DamianMac/Nimbus/issues/50#issuecomment-35223386).

[uglybugger]

Why not just deal with the async handler transparently? If it's async, just have the call to broker.Dispatch (already async) await its result. Then we could happy have synchronous and asynchronous handlers working happily without doubling the number of interfaces.

[shiftkey]

Async void is a "fire-and-forget" mechanism: the caller is unable to know when an async void has finished, and the caller is unable to catch any exceptions from it. The only case where this kind of fire-and-forget is appropriate is in top-level event-handlers. Every other async method in your code should return "async Task".

Citation: http://channel9.msdn.com/Series/Three-Essential-Tips-for-Async/Tip-1-Async-void-is-for-top-level-event-handlers-only

[uglybugger]

Hmm... Thinking about this some more, it might actually be smarter to push people towards async handlers that return a Task. We can deal with the async void scenario with a custom synchronisation context without too much hassle but on reflection I think that hiding the asynchronous nature of handlers might have been a design flaw in the first place.

Given that any operation triggered by the bus has a pretty good chance of doing something else on the bus, and that something else will be async, it seems silly to hide it in a synchronous wrapper. If we're going async, we should go all-in.

There are a couple of minor interface-breaking changes that are on my personal shortlist anyway so it might be worth pencilling in a change to async-only handlers and marking the synchronous ones as obsolete in a minor version bump soon.

Thoughts?

[uglybugger]

@shiftkey Dammit.. Wish I'd seen your reply before posting mine.

What do you think, then? New interfaces whose Handle methods return a Task, and check for AsyncStateMachineAttribute on the old synchronous handler methods and throw on startup? That gives us backward-compatibility, a nice way for people to call async methods within their own handlers and a fairly safe way to catch it when someone's fallen into the same trap that @kkozmic did..