DelegatingChannel is poorly designed

Topics: Web Api
Apr 18, 2011 at 5:15 PM
Edited Apr 18, 2011 at 5:16 PM

I think you should favor composition over inheritance and having the inner channel as a dependency feels wrong. It adds lot of complexity to do dependency injection in channels.

This is an example in the source code:

 

public class LoggingChannel : DelegatingChannel
{
    public LoggingChannel(HttpMessageChannel handler)
        :base(handler)
    {
       
    }

    protected override System.Threading.Tasks.Task<HttpResponseMessage> SendAsync(HttpRequestMessage request, CancellationToken cancellationToken)
    {
        System.Diagnostics.Trace.TraceInformation("Begin Request: {0} {1}", request.Method, request.RequestUri);
        return base.SendAsync(request, cancellationToken);
    }
}

 

I believe that a better design will be an interface like this:

 

public interface IChannelContext 
{
	HttpResponseMessage GetInner();
	Task<HttpResponseMessage> GetInnerAsync();
	HttpRequestMessage Request { get; }
}

public interface IDelegatingChannel
{
    Task<HttpResponseMessage> SendAsync(IChannelContext context);
HttpResponseMessage Send(IChannelContext context);
}

 

An implementation of an IDelegatingChannel with proper dependency injection is:

 

public class LoggingChannel : IDelegatingChannel
{
    private ILogger logger;

    public LoggingChannel(ILogger logger) 
    { 
       this.logger = logger; 
    }

 

    public HttpResponseMessage Send(IChannelContext context)
    {
      logger.Log("Begin request: {0} {1}", 
                      context.Request.Method,
                      context.Request.RequestUri);
      return context.GetInner();
    }

 

    public Task<HttpResponseMessage> SendAsync(IChannelContext context)
    {
      logger.Log("Begin async request: {0} {1}", 
                      context.Request.Method,
                      context.Request.RequestUri);
      return context.GetInnerAsync();
    }
}

 

Is the responsibility of the calle to assemble a proper IChannelContext, the order of the channels and so on.

Coordinator
Apr 18, 2011 at 7:32 PM
Edited Apr 18, 2011 at 7:41 PM

Whether or not a base class is used or not is orthagonal to testability / IoC container usage. There are very clear reasons why we would opt for a base class here in particular relating to the evolution of the interface.

In terms of testability, I agree there are some issues here that we need to explore. Many IoCs support chaining / decorators thus they can handle creating the channel. We will look into this though.

Thanks

Glenn

 

 

Apr 18, 2011 at 7:39 PM

I respectfully disagree. May you give me an example of an HttpMessageHandlerFactory using a servicelocator or IoC container with the following interface:

public interface IServicelocator
{
   object GetService(Type serviceType);
   T GetService<T>();
   IEnumerable<T> GetServices<T>();
}

| There are very clear reasons why we would opt for a base class here in particular relating to the evolution of the interface.

May you give a more detailed description here? Because the source code is right in this site and I can't find any reason for such design.

Coordinator
Apr 18, 2011 at 7:45 PM

A base class allows us to evolve the interface introducing new members in the future without breaking existing clients. If we use an interface we cannot evolve it. We end up having to create lots of versioned interfaces which is something that has bit us in the past and we want to avoid.

HttpMessageHandlerFactory implements an interface IHttpMessageHandlerFactory thus you can use whatever mechanism you like to actually build up the channel anyway you want.

Apr 18, 2011 at 8:16 PM
gblock wrote:

A base class allows us to evolve the interface introducing new members in the future without breaking existing clients. If we use an interface we cannot evolve it. We end up having to create lots of versioned interfaces which is something that has bit us in the past and we want to avoid.

SERIOUSLY? So you write a base class instead of an interface just in case you have to add a new member in the future? And you will not even make the member abstract because you will break existing clients...

 

| HttpMessageHandlerFactory implements an interface IHttpMessageHandlerFactory thus you can use whatever mechanism you like to actually build up the channel anyway you want.

In your previous mail you said:

| Whether or not a base class is used or not is orthagonal to testability / IoC container usage.

I didn't complain only on the base class, but also in the constructor argument. I AM still waiting your example of IHttpMessageHandlerFactory or HttpMessageHandlerFactory to see how orthagonal is the usage of such constructor argument and the factory. (BTW; the configuration accept only an implementation of HttpMessageHandlerFactory not IHttpMessageHandlerFactory).

Apr 18, 2011 at 9:09 PM

I don't see any problem with use a base class for this particular scenario. The channel model is represented as a "Decorator" pattern, in which every inner channel adds new functionally (You can think in this as a System.IO.Stream in .NET but here, every stream modifies the inner message or does something with it) 

Pablo.

 

Coordinator
Apr 18, 2011 at 10:13 PM

Very good point on the configuration class accepting the concrete factory. I will definitely fix that.

Here is a post on decorators in castle that shows how it supports chaining. In the example GetCostCalculatorDecorator depends on ICostCalculator.

Far as the other question, we can debate about the reasons for an abstract class vs not, though we're not likely to agree as it is a debate that goes on and on.

We can agree to explore ways to make it easier to test ;-)

Glenn

 

Apr 18, 2011 at 10:27 PM
I was thinking in an approach like that.
So I can do the -chaining- in Castle registration and ignore the innerChannel parameter in the HttpMessageHandlerFactory.OnCreate
but.. where do i get the most inner/default channel?
Do you think this is okey?


2011/4/18 gblock <notifications@codeplex.com>

From: gblock

Very good point on the configuration class accepting the concrete factory. I will definitely fix that.

Here is a post on decorators in castle that shows how it supports chaining. In the example GetCostCalculatorDecorator depends on ICostCalculator.

Far as the other question, we can debate about the reasons for an abstract class vs not, though we're not likely to agree as it is a debate that goes on and on.

We can agree to explore ways to make it easier to test ;-)

Glenn

Read the full discussion online.

To add a post to this discussion, reply to this email (wcf@discussions.codeplex.com)

To start a new discussion for this project, email wcf@discussions.codeplex.com

You are receiving this email because you subscribed to this discussion on CodePlex. You can unsubscribe on CodePlex.com.

Please note: Images and attachments will be removed from emails. Any posts to this discussion will also be available online at CodePlex.com


Coordinator
Apr 18, 2011 at 10:51 PM
Edited Apr 18, 2011 at 10:52 PM

First, on the interface it turns out there is not factory interface as we got rid of it. If you just override OnCreate on HttpMessageHandlerFactory you take complete control over the construction.

Second, The inner channel is passed to the factory when create is called. You definitely need to call to it at the innermost channel or the message will never pass on to service model.  One way to do that is to create a special channel (WindsorChannel) which depends on a keyed instance "InnerChannel" in it's ctor (i.e. either in windsor config or with attributes the key is configured). This channel will be configured to be the outermost decorator.

Then in your WindsorMessageHandlerFactory.OnCreate you grab your windsor container and add the innerChannel instance passed in to the container using that key. When the channel is resolved from the container, it will build up the stack including the WindsorChannel which will get it's dependency.

Glenn

Apr 19, 2011 at 4:34 PM
Thank you Glenn, I'd write a sample doing IoC with the channels, post it and be back to this thread with a link.

2011/4/18 gblock <notifications@codeplex.com>

From: gblock

First, on the interface it turns out there is not factory interface as we got rid of it. If you just override OnCreate on HttpMessageHandlerFactory you take complete control over the construction.

Second, The inner channel is passed to the factory when create is called. You definitely need to call to it at the innermost channel or the message will never pass on to service model. One way to do that is to create a special channel (WindsorChannel) which depends on a keyed instance "InnerChannel" in it's ctor (i.e. either in windsor config or with attributes the key is configured). This channel will be configured to be the outermost decorator.

Then in your WindsorMessageHandlerFactory.OnCreate you grab your windsor container and add the innerChannel instance passed in to the container using that key. When the channel is resolved from the container, it will build up the stack including the WindosrChannel which will get it's dependency.

Glenn

Read the full discussion online.

To add a post to this discussion, reply to this email (wcf@discussions.codeplex.com)

To start a new discussion for this project, email wcf@discussions.codeplex.com

You are receiving this email because you subscribed to this discussion on CodePlex. You can unsubscribe on CodePlex.com.

Please note: Images and attachments will be removed from emails. Any posts to this discussion will also be available online at CodePlex.com


Coordinator
Apr 19, 2011 at 4:43 PM
Sweet. Just to let you know, we started an internal thread on testability of channels.

My initial response was more of a gut reaction to the poor design comment.

Glenn

Sent from my Windows Phone

From: jfromaniello
Sent: Tuesday, April 19, 2011 8:34 AM
To: Glenn Block
Subject: Re: DelegatingChannel is poorly designed [wcf:254370]

From: jfromaniello

Thank you Glenn, I'd write a sample doing IoC with the channels, post it and be back to this thread with a link.

2011/4/18 gblock <notifications@codeplex.com>

From: gblock

First, on the interface it turns out there is not factory interface as we got rid of it. If you just override OnCreate on HttpMessageHandlerFactory you take complete control over the construction.

Second, The inner channel is passed to the factory when create is called. You definitely need to call to it at the innermost channel or the message will never pass on to service model. One way to do that is to create a special channel (WindsorChannel) which depends on a keyed instance "InnerChannel" in it's ctor (i.e. either in windsor config or with attributes the key is configured). This channel will be configured to be the outermost decorator.

Then in your WindsorMessageHandlerFactory.OnCreate you grab your windsor container and add the innerChannel instance passed in to the container using that key. When the channel is resolved from the container, it will build up the stack including the WindosrChannel which will get it's dependency.

Glenn

Read the full discussion online.

To add a post to this discussion, reply to this email (wcf@discussions.codeplex.com)

To start a new discussion for this project, email wcf@discussions.codeplex.com

You are receiving this email because you subscribed to this discussion on CodePlex. You can unsubscribe on CodePlex.com.

Please note: Images and attachments will be removed from emails. Any posts to this discussion will also be available online at CodePlex.com


Apr 19, 2011 at 6:26 PM
something like this:..
https://gist.github.com/928823

it is working for me..
But i am not sure if is okey as singleton?
HttpMessageHandlerFactory.OnCreate is called once?
Coordinator
Apr 20, 2011 at 1:24 AM

Should be ok. The channels are not configured per request, they are configured per host. OnCreate should only get called once per host.

Looks much cleaner than I expected :-)

Glenn

Coordinator
Apr 20, 2011 at 3:23 AM

The design we are discussing is having a SetInnerChannel method on DelegatingChannels as well as adding a factory hook to our HttpMessageHandlerFactory that you can wire to an IoC. With this model we will handle chaining all the channels together. You will specifying in the factory which channels to get like IChannelA, IChannelB, IChannelC, and then we will automatically chain them together.

Make sense?

Apr 20, 2011 at 3:24 AM
Definitily, please.
It is much better.

2011/4/19 gblock <notifications@codeplex.com>

From: gblock

The design we are discussing is having a SetInnerChannel method on DelegatingChannels as well as adding a factory hook to our HttpMessageHandlerFactory that you can wire to an IoC. With this model we will handle chaining all the channels together. You will specifying in the factory which channels to get like IChannelA, IChannelB, IChannelC, and then we will automatically chain them together.

Make sense?

Read the full discussion online.

To add a post to this discussion, reply to this email (wcf@discussions.codeplex.com)

To start a new discussion for this project, email wcf@discussions.codeplex.com

You are receiving this email because you subscribed to this discussion on CodePlex. You can unsubscribe on CodePlex.com.

Please note: Images and attachments will be removed from emails. Any posts to this discussion will also be available online at CodePlex.com


Jul 4, 2011 at 11:56 PM

Is there a complete example of how to wire all this together? I'm struggling to resolve my dependencies in my channels :(

Looking at the Web Api Contrib samples, There are classes whcih seem to be using DI in them, but it doesn't show how the entire program would be wired.

Coordinator
Aug 4, 2011 at 8:57 AM

I have good news...we're fixing this :-)

In the next drop using message handlers (channels) with IoC should be much easier. We no longer will require you to chain the handlers, nor will you have to pass a handler via the ctor. You will simply return a collection of handlers which you can retrieve from an IoC and we will wire them up properly.

Glenn

Aug 4, 2011 at 11:57 AM

Excellent! thank you very much

Aug 4, 2011 at 1:18 PM

So I'm guessing the new message handlers will have a way (return value?) to tell the pipeline whether to continue executing the remaining handlers or to stop, right? 

Kinda Processing.Continue | Stop?

Very good news indeed :)

Aug 4, 2011 at 2:43 PM

Sounds good Glenn, do you know when is the next drop?

Coordinator
Aug 4, 2011 at 3:42 PM
Working on it (really). We are finishing up some automation to make the process push button...

Sent from my Windows Phone

From: machadogj
Sent: 8/4/2011 6:43 AM
To: Glenn Block
Subject: Re: DelegatingChannel is poorly designed [wcf:254370]

From: machadogj

Sounds good Glenn, do you know when is the next drop?

Coordinator
Aug 4, 2011 at 4:06 PM
No, message handlers do not work that way. They are in a Russian doll model and always unwind.

If you want to stop you don't call Send on the base, you return Task<HttpResponseMessage> and the rest of the handlers unwind. If you want to continue you do.

for operation handlers however you can short circuit the rest of the pipeline by throwing an HttpResponseException<T>

Sent from my Windows Phone

From: dcazzulino
Sent: 8/4/2011 5:19 AM
To: Glenn Block
Subject: Re: DelegatingChannel is poorly designed [wcf:254370]

From: dcazzulino

So I'm guessing the new message handlers will have a way (return value?) to tell the pipeline whether to continue executing the remaining handlers or to stop, right?

Kinda Processing.Continue | Stop?

Very good news indeed :)