Null returned from message creator is not handled

[cpx86]

If a message creator returns a null value, Routemeister will still invoke the handler method but with a null reference for the handler instance. This is technically allowed by reflection, but not correct and leads to some unexpected behaviors. A realistic case where this might happen is if you plugin a container that returns default values instead of throwing exceptions when it can't resolve a type.

I've gone through the code and adding a simple null-check seems to be trivial and should hardly have any performance impact. Would you be interested in a PR?

Here's a simple repro:

void Main()
{
    var factory = new MessageRouteFactory();
    var routes = new MessageRoutes
    {
        factory.Create(new[] {GetType().GetTypeInfo().Assembly}, typeof (IHandle<>))
    };
    var router = new SequentialAsyncRouter((t, e) => null, routes);

    router.RouteAsync("hello");
}

interface IHandle<T>
{
    Task HandleAsync(T message);
}

public class StringHandler : IHandle<string>
{
    public Task HandleAsync(string message)
    {
        Console.WriteLine(this.ToString()); // this is null
        return Task.CompletedTask;
    }
}

[danielwertheim]

That sounds fair. I'll gladly accept "something" preventing it from accepting null handlers being resolved.

On 17 Oct 2017 14:11, "Christian Palmstierna" [email protected] wrote:

If a message creator returns a null value, Routemeister will still invoke the handler method but with a null reference for the handler instance. This is technically allowed by reflection, but not correct and leads to some unexpected behaviors. A realistic case where this might happen is if you plugin a container that returns default values instead of throwing exceptions when it can't resolve a type.

I've gone through the code and adding a simple null-check seems to be trivial and should hardly have any performance impact. Would you be interested in a PR?

Here's a simple repro:

void Main() { var factory = new MessageRouteFactory(); var routes = new MessageRoutes { factory.Create(new[] {GetType().GetTypeInfo().Assembly}, typeof (IHandle<>)) }; var router = new SequentialAsyncRouter((t, e) => null, routes);

router.RouteAsync("hello"); } interface IHandle<T> { Task HandleAsync(T message); } public class StringHandler : IHandle { public Task HandleAsync(string message) { Console.WriteLine(this.ToString()); // this is null here return Task.CompletedTask; } }

— You are receiving this because you are subscribed to this thread. Reply to this email directly, view it on GitHub https://github.com/danielwertheim/routemeister/issues/8, or mute the thread https://github.com/notifications/unsubscribe-auth/AAL9zCqgctDWOMQ-Q0yo29RtPBko-LsFks5stJmAgaJpZM4P8C_J .

[cpx86]

I was thinking to throw InvalidOperationException if the handler is null. Sound good?

One question about the publish methods - should it resolve all handlers first and validate them before attempting to execute them? Also, slightly off topic, but currently if one handler throws an exception then subsequent handlers will not be executed. Is that by design?

[danielwertheim]

Regarding the subsequent handlers, yes, the router and dispatchers are pretty dumb by intention. The idea would be to either create a more advanced using the Routes and contribute if it makes sense. And just to be an in-memory routing/dispatching solution.

Regarding InvalidOperationException, sounds fair.

The pre-resolving sounds nice, but not sure if they should be re-used. Thinking the strategy of lifetime of a handler is up to the creator, but I guess it's fair to think "per-message".

Now to a bit of.... The same goes for Dispatcher(s) which doesn't use the router under the hood. This should probably be taken care of in a separate PR/Issue.

[cpx86]

No, it's probably better not to re-use them. Was just thinking that it could make sense to first ensure all handlers can actually be resolved before executing any of them.

I'll prepare a PR for this issue now then.