Create the hub pipeline

[yngvebn]

This will enable to control what happens to the request before and/or after the invocation of the hub-action. I should be possible to decorate both a hub and a method with this attribute.

This would also open up for more fluent handling of Authorization as you could do this on Hub-level or Actionlevel with a derived Authorize attribute, just as you would do in MVC.

The current state of the discussion is that this should first be implemented on a lower level with a concept currently called HubInvocation. Implementations of IHubInvocation can be registered with the DependenyResolver.

• The IHubInvocation must be able to short circuit the execution of a hub-action and its HubInvocations

Interfaces/Classes:

    public interface IHubPipelineBuilder
    {
        Func<IHubInvokerContext, Task<object>> Build(Func<IHubInvokerContext, Task<object>> callback);
    }

    public interface IHubPipeline
    {
        IHubPipeline Use(IHubPipelineBuilder builder);
        Task<object> Invoke(IHubInvokerContext context);
    }

    public class HubPipeline : IHubPipeline
    {
        private Stack<IHubPipelineBuilder> _builders = new Stack<IHubPipelineBuilder>();

        public IHubPipeline Use(IHubPipelineBuilder builder)
        {
            _builders.Push(builder);
            return this;
        }

        public Task<object> Invoke(IHubInvokerContext context)
        {
            IHubInvoker masterInvoker = _builders.Reverse().Aggregate((a, b) => new ComposedInvoker(a, b));
            return masterInvoker.Build(BaseInvoke)(context);
        }

        public Task<object> BaseInvoke(IHubInvokerContext context)
        {
            // This would be the default hub logic
            return Fail<object>(new Exception("Error!"));
        }

        private Task<T> Fail<T>(Exception exception)
        {
            var tcs = new TaskCompletionSource<T>();
            tcs.SetException(exception);
            return tcs.Task;
        }

        private class ComposedInvoker : IHubPipelineBuilder
        {
            private IHubPipelineBuilder left;
            private IHubPipelineBuilder right;

            public ComposedInvoker(IHubPipelineBuilder left, IHubPipelineBuilder right)
            {
                this.left = left;
                this.right = right;
            }

            public Func<IHubInvokerContext, Task<object>> Build(Func<IHubInvokerContext, Task<object>> callback)
            {
                return context =>
                {
                    return left.Build(right.Build(callback))(context);
                };
            }
        }
    }

    // Implementation example
    public class GeneralLogger : IHubPipelineBuilder
    {
        public Func<IHubInvokerContext, Task<object>> Build(Func<IHubInvokerContext, Task<object>> callback)
        {
            return context =>
            {
                Console.WriteLine("Before");
                return callback(context).ContinueWith(task =>
                {
                    if (task.IsFaulted)
                    {
                        var ex = task.Exception.GetBaseException();
                        Console.WriteLine("Exception ocurred");
                        Console.WriteLine(ex.Message);
                        Console.WriteLine(ex.StackTrace);
                    }

                    Console.WriteLine("After");

                    return (object)null;
                });
            };
        }
    }

    public class Authorized : IHubPipelineBuilder
    {
        public Func<IHubInvokerContext, Task<object>> Build(Func<IHubInvokerContext, Task<object>> callback)
        {
            return context =>
            {
                var attribute = (AuthorizeAttribute)context.Method.GetCustomAttributes(typeof(AuthorizeAttribute), inherit: true).FirstOrDefault();

                if (attribute != null)
                {
                    if (!context.User.Identity.IsAuthenticated)
                    {
                        throw new InvalidOperationException("You suck");
                    }

                    if (attribute.Roles != null)
                    {
                        if (!attribute.Roles.Any(role => context.User.IsInRole(role)))
                        {
                            throw new InvalidOperationException("You suck");
                        }
                    }
                }

                return callback(context);
            };
        }
    }

The MasterInvoker (or whatever it should be called) gets a list of IHubInvocations and invokes them all. If one of the HubInvocations omits the call to invocation.Invoke(...) the process is stopped.

[davidfowl]

Great idea. I see you also sent a pull request (I made some comments). Generally, before implementing a feature (especially one that affects public api), a design discussion is needed so that we don't make you waste your time implementing something that doesn't make it into the product.

[yngvebn]

Thanks!

I read your comments, and totally agree :) Where does usually this designdiscussion take place?
We could close the pull request, and try and figure out a proper way to do this right?

[davidfowl]

Your initial comment will become a changing proposal. So just iterate on here and edit your initial post as we go until we finally agree on something.

[yngvebn]

Alright

[yngvebn]

I propose that we could introduce two classes, MethodInvocatingContext and MethodInvocatedContext, much like the ActionExecutingContext etc., in MVC.
The Processing in the HubDispatcher should then use a MethodInvocationContext class internally to manage data, and update this accordingly.

The MethodInvocatingContext and MethodInvocatedContext could contain the information about the current request such as the HubContext, (and as Damian mentioned in a comment) perhaps HubContext, ClientAgent and GroupManager.

When the method is invoked in the Dispatcher, the result is assigned to the current MethodInvocationContext, which in turn passes it to the OnMethodInvoked method on the filters (if any), and updates it back to the MethodInvocationContext.

[davidfowl]

What properties are different on the 2 contexts?

[yngvebn]

I'm actually experimenting a bit here now, and it seems the only difference would really be the Result. However, properties in the MethodInvokingContext would be changable, and could have a direct effect on the resulting flow (such as the request parameters, the data passed in and so on), whereas in the MethodInvokedContext changing these values would have no function. Which is why I would propose keeping them separate.

[davidfowl]

What things are mutable on the InvokingContext?

[yngvebn]

I would argue that only the Parameters should be mutable.

[davidfowl]

Ok lets start fleshing out each of the types in your original post (you can edit it).

[yngvebn]

Ok, I started fleshing out with the minimal amount of information required on the classes and interfaces. I'm building this in my local repo as I go, so there will most probably be changes :)

[davidfowl]

What about Exception in the MethodInvokedContext?

[yngvebn]

Ah, yes. Good idea

[yngvebn]

Now, I'm not really sure about the timing of the OnMethodInvoked method. I guess it should be called before processing of the result starts in the Dispatcher? (Immediately after the invocation)

[davidfowl]

You know, we can do something unlike mvc and more like owin middleware. Consider this for a moment. If you need to add anything to the pipeline you get passed the previous filter:

public interface IFilter
{
    void Execute(IHub hub, IFilter previous);
}

public class AuthorizeFilter : IFilter
{
    public void Execute(IHub hub, IFilter previous)
    {
        if(hub.User.Identity.IsAuthenticated) {
            previous.Execute(hub, this);
        }
    }
}

Something like that. It gets around having to create crazy ed and ing methods and contexts.

[yngvebn]

That would work. Although it restricts execution of filters to prior to invocation. But maybe that's an acceptable place to start?

Also, how would you go ahead as a user to add these filters to the hub or methods?

[davidfowl]

That was a rough idea, it doesn't have to be filters. The general idea is you pass in the "execution" so you don't have events in the pipeline. More generally:

public interface IFilter
{
    void Execute(IHub hub, Action invocation);
}

public class AuthorizeFilter : IFilter
{
    public void Execute(IHub hub, Action invocation)
    {
        if(hub.User.Identity.IsAuthenticated) {
            invocation();
        }
    }
}

Something to that effect.

[yngvebn]

Makes sense. Let me play around with the idea a bit, and see what I come up with!

[davidfowl]

Does this work?

public interface IHubInvocation 
{
   public object Invoke(InvocationContext context, IHubInvocation invocation);
}

public class InvocationContext
{
    public IHub Hub { get; internal set; }
    public MethodDescriptor { get; set; }
    public IList<ParameterDescriptor> Parameters { get; set; }
}

public class AuthorizeFilter : IHubInvocation
{
    public object Invoke(InvocationContext context, IHubInvocation invocation)
    {
        if(context.Hub.User.Identity.IsAuthenticated)
        {
            return invocation.Invoke(context, this);
        }
        return null;
    }
}

public class LoggingFilter : IHubInvocation
{
    private ILogger _logger;
    public class LoggingFilter(ILogger logger)
    {
        _logger = logger;
    }

    public object Invoke(InvocationContext context, IHubInvocation invocation)
    {
        try 
        {
            _logger.Log("Before calling " + context.MethodDescriptor.Name);
            return invocation.Invoke(context, this);
        }
        catch(Exception ex) 
        {
            _logger.LogException(ex);
        }
        finally 
        {
            _logger.Log("After calling " + context.MethodDescriptor.Name);
        }
        return null.
    }
}

public class BlockEveything : IHubInvocation
{
    public object Invoke(InvocationContext context, IHubInvocation invocation)
    {
        if(context.MethodDescriptor.ReturnType == typeof(Task)) 
        {
            ((Task)invocation.Invoke(context, invocation)).Wait();
        }
    }
}

[yngvebn]

That ought to work. But shouldn't it still be a type that inherits Attribute in order to be able to decorate hubs and hub methods with the different filters?

[davidfowl]

It doesn't have to be an attribute. You can imagine a fluent api that just registers a bunch of classes built on top of this core.

[yngvebn]

Right, so then we would need somewhere to register these classes. Are any similar fluent operations done elsewhere in the SignalR Core?

[davidfowl]

No, that was just an example

[davidfowl]

It'd would be some new type in the dependency resolver I'd imagine.

[yngvebn]

Yeah, we're doing similar things in a project I'm working on where we use Castle.Windsor and we register implementations of DomainEventHandlers in the container. I'm picturing this as something similar. Just trying to figure out a proper way of doing it here. The way I see it, you should be able to register a IHubInvocation to one or more Hubs, and not just as a global registration.

[davidfowl]

Or just pass enough context for the HubInvocation to decide, then there's no need for a crazy configuration API. The majority of users will use the attribute based system anyways.

[yngvebn]

Wait, so you're suggesting that we do both? Attribute-based, and registration of IHubInvocations?

[davidfowl]

I'm saying once the core is solid, you can build anything on top. Let's agree on the most powerful and simplest implementation then discuss how the other systems could work on top of that (i.e. fluent api and attribute based).

[yngvebn]

Okay, sounds good. I'll try and implement something along the lines of your example, write some tests around it and see how it works in a project. And I'll keep it as simple as possible :)

[davidfowl]

Awesome! You can even implement the attribute based filters as a HubInvocation itself:

public class AttributeBaseInvocation : IHubInvocation
{
    public object Invoke(InvocationContext context, IHubInvocation invocation)
    {
        var attrib = (Attribute[])context.MethodDescriptor.GetCustomAttributes(typeof(Attribute), true).FirstOrDefault();

        if(attrib != null) 
        {
            var attribInvocation = attrib as IHubInvocation;
            if(attribInvocation != null) 
            {
               return attribInvocation.Invoke(context, invocation);
            }
        }

        return invocation.Invoke(context, this);
    }
}

This code was written in notepad and may not work as is. But you get the idea.

[drub0y]

With the way the InvocationContext class is defined right now it would make unit testing filters difficult since only SignalR would be able to set the IHub since it's marked internal which makes sense since it shouldn't be possible to change in the context of a filter. I would propose an interface and a slightly more explicit name:

public interface IHubInvocationContext
{
    public IHub Hub { get; }
    public MethodDescriptor { get; set; }
    public IList<ParameterDescriptor> Parameters { get; set; }    
}

public class HubInvocationContext : IHubInvocationContext
{
    public IHub Hub { get; internal set; }
    public MethodDescriptor { get; set; }
    public IList<ParameterDescriptor> Parameters { get; set; }
}

[davidfowl]

I think certain things require order, like Authorization, but it has no bearing on the HubInvocation logic at all. HubInvocation is a lower level feature that gives you a chance to put something in the pipeline (and it could be anything).

When we build higher level concepts like authorization attributes, we can have some rules about order and probably just copy mvc in that respect.

[yngvebn]

@davidfowl I'm having some issues with the proposed implementation you mentioned earlier (#548 (comment)).

I don't seem to be able to wrap my head around how the execution of these should work. The problem I'm having is with the invocation being passed in, and then executed with (context, this). Won't this just turn into an endless loop, unless the invocation that was passed in stops further execution?

Update
I've updated the original post with a solution to the problem. I'm not sure I like it. Maybe we should rethink the solution a bit?:)

[davidfowl]

I'll hack around with the solution a bit and try to come up with something cleaner. I'm trying to avoid anything attribute based and that requires the user to think about pre/post/order and exception handling if possible (and I know it's possible). I think people are having a hard time with the design since they are used to MVC but that's easy to fix (they'll get used to SignalR).

The intent is to be more like message handlers (webapi) but you get a control the entire pipeline or alter it in subtle way.

[yngvebn]

Yeah, i totallt agree. I just got a bit stuck here. All help is greatly appreciated, and I'd love to be able to contribute to SignalR :)

[yngvebn]

@davidfowl Just a suggestion, but what about if the Invoke-method doesn't take in the Invocation, but requires the implementation to return an object that holds information about if the flow should continue or not. That way you force the implementation to handle exceptions (if it throws, the exception will just continue up the stack). The HubInvocation-"executor" would then continue execution only if the returnvalue is valid.
Something like:

public class HubInvocationResult
{ 
      public HubInvocationState State {get; set;}

      // Whatever else is relevant. Exception-details etc...
}

public enum HubInvocationState
{
    Failed,
    Success
}

[davidfowl]

How do I do something like exception handling?

[davidfowl]

At a high level this is what I'm going for:

Action<Context> Middleware(Action<Context> app)
{
    return context => 
   {
        app(context);
    };
}

Action<Context> HandleExceptions(Action<Context> app)
{
    return context => 
    {
        try
        {
            app(context);
        }
        catch(Exception ex)
        {
            LogException(ex);
        } 
    };
}

[yngvebn]

Yeah, after a nights sleep I pull back my suggestion, as it makes something like exceptionhandling unusable in the way that we initially were thinking.

I was also thinking similarily to what you're suggesting but whatever I ended up with I had problems with chaining the execution.

[davidfowl]

class Program
{
    static void Main(string[] args)
    {
        var pipeline = new HubPipeline();

        // Build the pipeline
        pipeline.Use(invoker => new GeneralLogger(invoker))
                .Use(AuthenticateEverything);

        // Invoke the pipeline with the appropriate context
        pipeline.Invoke(new HubInvokerContext());
    }

    private static object AuthenticateEverything(IHubInvoker invoker, IHubInvokerContext context)
    {
        if (context.User.Identity.IsAuthenticated)
        {
            throw new Exception("Security");
        }

        return invoker.Invoke(context);
    }
}

public static class PipelineExtensions
{
    public static IHubPipeline Use(this IHubPipeline pipeline, Func<IHubInvoker, IHubInvokerContext, object> middleware)
    {
        return pipeline.Use(invoker => new FuncInvoker(invoker, middleware));
    }
}

public class FuncInvoker : IHubInvoker
{
    private IHubInvoker _invoker;
    private Func<IHubInvoker, IHubInvokerContext, object> _funcInvoker;

    public FuncInvoker(IHubInvoker invoker, Func<IHubInvoker, IHubInvokerContext, object> funcInvoker)
    {
        _invoker = invoker;
        _funcInvoker = funcInvoker;
    }

    public object Invoke(IHubInvokerContext context)
    {
        return _funcInvoker.Invoke(_invoker, context);
    }
}

public class GeneralLogger : IHubInvoker
{
    private IHubInvoker _invoker;
    public GeneralLogger(IHubInvoker invoker)
    {
        _invoker = invoker;
    }

    public object Invoke(IHubInvokerContext context)
    {
        try
        {
            Console.WriteLine("Before");
            return _invoker.Invoke(context);
        }
        catch (Exception ex)
        {
            Console.WriteLine("Exception ocurred");
            Console.WriteLine(ex.Message);
            Console.WriteLine(ex.StackTrace);
            return null;
        }
        finally
        {
            Console.WriteLine("After");
        }
    }
}

public interface IHubInvokerContext
{
    List<ParameterInfo> Parameters { get; set; }
    MethodInfo Method { get; set; }
    object[] Args { get; set; }

    dynamic Clients { get; set; }
    IPrincipal User { get; set; }
}

public interface IHubInvoker
{
    object Invoke(IHubInvokerContext context);
}

public interface IHubPipeline
{
    IHubPipeline Use(Func<IHubInvoker, IHubInvoker> builder);
    object Invoke(IHubInvokerContext context);
}

public class HubPipeline : IHubPipeline
{
    private Stack<Func<IHubInvoker, IHubInvoker>> _builders = new Stack<Func<IHubInvoker, IHubInvoker>>();
    private IHubInvoker _root = new DefaultInvoker();
    public IHubPipeline Use(Func<IHubInvoker, IHubInvoker> builder)
    {
        _builders.Push(builder);
        return this;
    }

    public object Invoke(IHubInvokerContext context)
    {
        IHubInvoker masterInvoker = _builders.Reverse().Aggregate((a, b) => invoker => a(b(invoker)))(_root);
        return masterInvoker.Invoke(context);
    }
}

public class DefaultInvoker : IHubInvoker
{
    public object Invoke(IHubInvokerContext context)
    {
        // Invoke default hub pipeline in here
        throw new NotImplementedException();
    }
}

I'll mess around and try to implement something and see if there's anything that wouldn't work. I can't see it not working though, the design is pretty simple. Figuring out things like what context and naming of interfaces can be figured out. One issue with this approach is that we might need to make it return Task since the pipeline is async.

One of the built in Invoker will be the thing that runs attributes.

[yngvebn]

Wow, that's a neat solution. I like your thinking! One minor thing though (that I changed in main post) is the list of ParameterInfos. Isn't that just information about what the parameters are? Isn't the JsonValue[] array the correct parameter to use in the Context?
edit noticed you use object[] args along with ParameterInfo.. Was that what you was thinking in that regard?

This ended up very different from my initial thoughts :)

[davidfowl]

That was my hacking in a console application, we'd come up with what we thought the appropriate context should be. I think what you have in your last code sample looks good.

[yngvebn]

Yeah, I'm playing around in a console-app myself.. Just copied your code to get the gist of the flow. Seems pretty simple as you say

[yngvebn]

We should also think about the ability to take more arguments in the constructors, which then will be resolved with dependencyresolver. I mean, if I create my own IHubInvoker implementation, I might want to pull in different stuff into it..

[davidfowl]

Yep, that makes total sense. The developer gets total control of building the pipeline. That's hard to do since we pass the previous invoker as a ctor parameter and that's hard to inject. Need to think about it a bit more.

[drub0y]

FWIW, I like where this design is going. FuncInvoker is an important piece of the puzzle to keep the barrier to entry low as it'll just allow simple inline delegates for simple/one off impls, yet you still have IHubInvoker for people who may want to create libraries of reusable invokers and stuff. Don't know if I love Use as the verb to building the pipeline, but that's just a matter of taste.

I assume you are you familiar with the DelegatingHandler model of ASP.NET Web API? You're getting pretty close to that. Maybe using the same exact model of its MessageHandlers to stack them with a HubConfiguration class would be clearer and provide some consistency across the two APIs?

[yngvebn]

Hmm.. Maybe I'm missing something, but how about something as simple as:

    class Program
    {
        static void Main(string[] args)
        {
            IHubInvocationContext context = new HubInvocationContext();
            IHubInvocationPipeline pipeline = new HubInvocationPipeline();

            // We can add several HubInvocations to the pipeline
            IHubInvocation authorization = new AuthorizationInvocation();
            IHubInvocation logger = new LoggingInvocation();
            IHubInvocation failing = new FailingInvocation();
            pipeline.Use(logger);
            pipeline.Use(authorization);
            pipeline.Use(failing);

            pipeline.Invoke(context);

            Console.ReadLine();

        }
    }

    public interface IHubInvocationContext
    {
        IHub Hub { get; }
        MethodDescriptor MethodDescriptor { get; set; }
        IJsonValue[] ParameterValues { get; set; }
    }
    public interface IHubInvocation
    {
        void Invoke(IHubInvocationContext context, Action<IHubInvocationContext> invocation);
    }

    public interface IHubInvocationPipeline
    {
        void Invoke(IHubInvocationContext context);
        void Use(IHubInvocation authorization);
    }

    public class HubInvocationPipeline : IHubInvocationPipeline
    {
        private Queue<IHubInvocation> _invocationQueue;

        public HubInvocationPipeline()
        {
            _invocationQueue = new Queue<IHubInvocation>();
        }
        public void Invoke(IHubInvocationContext context)
        {
            if(_invocationQueue.Any())
                 _invocationQueue.Dequeue().Invoke(context, Invoke);
        }

        public void Use(IHubInvocation invocation)
        {
            _invocationQueue.Enqueue(invocation);
        }
    }

    public class HubInvocationContext : IHubInvocationContext
    {
        public IHub Hub { get; internal set; }
        public MethodDescriptor MethodDescriptor { get; set; }
        public IJsonValue[] ParameterValues { get; set; }
    }

    public class MethodDescriptor
    {
         public MethodInfo MethodInfo { get; set; }
    }


    // HubInvocations
    public class FailingInvocation : IHubInvocation
    {
        public void Invoke(IHubInvocationContext context, Action<IHubInvocationContext> invocation)
        {
            try
            {
                Console.WriteLine("Failing");
                throw new Exception();
                invocation(context);
            }
            catch
            {
                Console.WriteLine("Failing- Catching");
            }
            finally
            {
                Console.WriteLine("Failing- Ending");
            }
        }
    }

    public class LoggingInvocation : IHubInvocation
    {
        public void Invoke(IHubInvocationContext context, Action<IHubInvocationContext> invocation)
        {
            try
            {
                Console.WriteLine("Logging");
                invocation(context);
            }
            catch
            {
                Console.WriteLine("Logging- Catching");
            }
            finally
            {
                Console.WriteLine("Logging- Ending");
            }
        }
    }

    public class AuthorizationInvocation : IHubInvocation
    {
        public void Invoke(IHubInvocationContext context, Action<IHubInvocationContext> invocation)
        {
            try
            {
                Console.WriteLine("Authorize");
                invocation(context);
            }
            catch
            {
                Console.WriteLine("Authorize- Catching");
            }
            finally
            {
                Console.WriteLine("Authorize- Ending");
            }
        }
    }

Removing all constructor dependencies means we can control injection of these when resolving IHubInvocation implementations

The AttributeResolver could then be implemented like so:

    public class AttributeResolver : IHubInvocation
    {
        public AttributeResolver()
        {
            _attributeInvocations = new Queue<IHubInvocation>();
        }
        private Queue<IHubInvocation> _attributeInvocations;
        public void Invoke(IHubInvocationContext context, Action<IHubInvocationContext> invocation)
        {
            _attributeInvocations = new Queue<IHubInvocation>(context.MethodDescriptor.MethodInfo.GetCustomAttributes(true).OfType<HubInvocationAttribute>());
            Invoke(context);
            invocation(context);
        }

        public void Invoke(IHubInvocationContext context)
        {
            if (_attributeInvocations.Any())
                _attributeInvocations.Dequeue().Invoke(context, Invoke);
        }
    }

Just playing in a consoleapp. The AttributeResolver in this example only resolves attributes on the methods, and not the hub itself. But that's pretty simple to implement

[yngvebn]

@davidfowl One question - What was your initial thought when letting the Invoke-method return object (or Task)? What would that returnvalue be used for? (As you can see I've made the Invoke a void-method)

[davidfowl]

So you just moved more parameters to the method itself. Looks fine to me (Do we even need an interface at this point 😄 jk jk). Ideally it would be async but that will make it hard to write these...Need to think about it some more.

[yngvebn]

I think it's an acceptable trade-off instead of requiring the implementation to take an implementation of IHubInvocation as an argument in the constructor.

I guess we could change the void Invoke(IHubInvocationContext context, Action<IHubInvocationContext> invocation) signature to object Invoke(IHubInvocationContext context, Func<IHubInvocationContext, object> invocation) instead, but I'll leave my code sample as is for the moment. Maybe you get a bright idea :)

[davidfowl]

@yngvebn can you update this with the latest proposal? I think it's in the jabbr room in history somwhere

[yngvebn]

@davidfowl I'll do it later on today when I'm home from work :)
(I have the complete code in a project on my home computer)

[davidfowl]

Implementation has started here https://github.com/SignalR/SignalR/tree/pipeline

[yngvebn]

Nice! :) Really looking forward to this being implemented!

[Xiaohongt]

verified