Warn me if my handler is async about nimbus HOT 9 CLOSED

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

from nimbus.

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

from nimbus.

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...

from nimbus.

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 :)

from nimbus.

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).

from nimbus.

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.

from nimbus.

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

from nimbus.

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?

from nimbus.

@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..

from nimbus.