routemeister icon indicating copy to clipboard operation
routemeister copied to clipboard

Check for null when resolving handlers

Open cpx86 opened this issue 7 years ago • 5 comments

Fixes #8

cpx86 avatar Oct 24 '17 13:10 cpx86

Looking at the code I got some ideas that I would like to run by you. What do you think of doing something like this:

public static class MessageHandlerCreatorExtensions
{
    public static object RequiredHandlerInvoke(
        this MessageHandlerCreator creator,
        Type messageHandlerType,
        MessageEnvelope envelope)
            => creator(messageHandlerType, envelope)
            ?? throw new InvalidOperationException($"Message handler creator could not create a message handler of type {messageHandlerType.FullName}.");
}

Then if someone wants to write their own dispatcher or router they can easily use the same logic. This would be used like this:

var handler = _messageHandlerCreator.RequiredHandlerInvoke(action.HandlerType, envelope);

Could then potentially not do any change of existing routers and dispatchers and leave it up the the user:

public static class MessageHandlerCreators
{
    public static MessageHandlerCreator NotAcceptingNullHandlers(MessageHandlerCreator creator)
        => creator.RequiredHandlerInvoke;
}

which then could be used:

UnitUnderTest = new AsyncDispatcher(
    MessageHandlerCreators.NotAcceptingNullHandlers((t, e) => Activator.CreateInstance(t)), routes);

I guess the footprint of pre-creating the handlers isn't to large, how-ever, it will be done once per message of the same type routed. So for n > 1 it's really unnecessary, because if it could be created the first time... And yes, then the check is also unnecessary, but perhaps fair. Ideally the user should pick the strategy. I mean, if the creator returns null they have messed up. Perhaps offer something that can be used when they bootstrap everything. Run their creator through all the routes to ensure it's satisfied?

danielwertheim avatar Oct 24 '17 19:10 danielwertheim

@cpx86 what's your opinion on the above?

danielwertheim avatar Oct 26 '17 19:10 danielwertheim

@danielwertheim Sorry, was a bit swamped lately..

I like the idea of making it less intrusive on the implementors. Would it be possible to take it one step further and actually wrap the MessageHandlerCreator before passing it to the underlying router/dispatcher? That way the routers wouldn't even need to change, and new routers wouldn't need to know about this behavior. Not sure if it would make any difference on performance or number of allocations though - I'm thinking that pre-wrapping it could affect the ability to inline it - that's something that could be worth profiling.

cpx86 avatar Oct 30 '17 09:10 cpx86

@cpx86 I might be misunderstanding now, but isn't that what my last "sample" does?

UnitUnderTest = new AsyncDispatcher(
    MessageHandlerCreators.NotAcceptingNullHandlers((t, e) => Activator.CreateInstance(t)), routes);

Sure it could be a type or something, but the idea of the sample is to wrap the passed creator with a behavior of not accepting null resolves. That way, it's up to the user when bootstrapping.

The other code can very well be used by "authors" to enforce it in their specific routers/dispatchers.

danielwertheim avatar Oct 30 '17 09:10 danielwertheim

Ah my bad, I mixed something up in my head :)

Letting it be up to the user makes sense, I agree. Although the question then is, does it even need to be provided by Routemeister? In that case the user could just create their own extension if they want to simplify handling null-resolved handlers (since it is anyway dependent on what factory you use).

cpx86 avatar Oct 30 '17 13:10 cpx86