Moving from IoHandlerChain to something easier to debug

[Emmanuel Lecharny <el...@gmail.com>]

Hi,

in the different protocols we have in ADS, we are using a
IoHandlerChain inherited class to handle the received messages.
Although I think that this pattern is sometime usefull if you want to
use an evolving message handling (like adding dynamically some
processing), in this very case, we have just fixed handlers.

There are three main concerns whith this pattern usage :
1) it's really painfull to debug, as you have to set BP in every
handler when you step through the code
2) it slow down the processing for heavily loaded servers
3) it forces the implementor to inject data through a session
attribute, which is not exactly the best solution, but you do'nt have
any other

I would suggest to switch from this pattern to a simpler one, where we
keep the handler decomposition (I like the idea of decomposing
treatments in small disconnected methods, compared to a big fat
method), but using as many methods as we have handlers. Doing so will
remove the 2 first points, without modifying the logic of the current
code, while the 3rd point will substitute a parameter passing to a
session storage.

The last gain is that we won't have anymore 150 lines in stackTraces ...

wdyt ?

-- 
Regards,
Cordialement,
Emmanuel Lécharny
www.iktek.com

Re: Moving from IoHandlerChain to something easier to debug

[Enrique Rodriguez <en...@gmail.com>]

On 6/21/07, Emmanuel Lecharny <el...@apache.org> wrote:
> Enrique Rodriguez a écrit :
> ...
> > It would help MINA developers to know
> > whether to use the IoHandlerChain or not.
>
> If you are going to use a Chain patter, then I guess that the important
> point is to think about the problem it solves. For instance, monitoring
> by inserting loggers could be a good usage. But the main advantage of
> chain parttern is its versatility : you can change the order of
> handlers, or simply add or remove handlers on demand. If you never do
> that, then using a chain pattern is questionable (IMHO).

You can also use it to share chain links.  And links are shared
between Change Password and Kerberos, since they both work with
tickets and principals and they both have to perform some of the same
validation and security checks.

> ...
> Method 2 is slightly better, because it keeps the decompaosition as it
> was in the chain. The main problem is that you can easily factorize
> their usage, and it's impossible to tests, as each method is private

Nothing says these couldn't be package or protected if you want to test.

> So I would favor method 3, where we don't use instances of classes, but
> static methods. So each IoHandlerCommand inherited class can be kept
> with a very minor refactoring.

The problem with static access is we may wish to set instance state
since some of the commands are shared, today in Change Password and
Kerberos and I can see some need in DNS, when it is acting as both a
client and server.

But, this is barely different than #3, just an instantiation of a
class before calling the method to execute.

FWIW, this is how the protocols were originally written, before
IoHandlerChain support was added to MINA.  The problem here isn't the
pattern, but rather the implementation.  Much of the problem you are
reporting is due to the bi-directional nature of the MINA chains,
which is a feature we never use anywhere and which leads to the
maintenance of the position in the chain and the big stack traces.

Enrique

Re: Moving from IoHandlerChain to something easier to debug

[Alex Karasulu <ak...@apache.org>]

On 6/22/07, Emmanuel Lecharny <el...@apache.org> wrote:
>
> Enrique Rodriguez a écrit :


SNIP ..

> On 6/21/07, Emmanuel Lecharny <el...@gmail.com> wrote:
> > It would help MINA developers to know
> > whether to use the IoHandlerChain or not.
>
> If you are going to use a Chain patter, then I guess that the important
> point is to think about the problem it solves. For instance, monitoring
> by inserting loggers could be a good usage. But the main advantage of
> chain parttern is its versatility : you can change the order of
> handlers, or simply add or remove handlers on demand. If you never do
> that, then using a chain pattern is questionable (IMHO).


With MINA filters I think we can still achieve the level of logging that
you're talking
about but perhaps not at the same resolution so I think the dynamism we gain
from the chain pattern is somewhat redundant.  I don't think it's necessary
here
since the protocol is relatively static.  Actually the chain pattern might
benefit
LDAP more since it can introduce new mechanisms for things like SASL,
Controls,
and extended operations  but there might be more manageable patterns for
this.

Alex

Re: Moving from IoHandlerChain to something easier to debug

[Emmanuel Lecharny <el...@apache.org>]

Enrique Rodriguez a écrit :

> On 6/21/07, Emmanuel Lecharny <el...@gmail.com> wrote:
>
>> ...
>> I would suggest to switch from this pattern to a simpler one, where we
>> keep the handler decomposition (I like the idea of decomposing
>> treatments in small disconnected methods, compared to a big fat
>> method), but using as many methods as we have handlers. Doing so will
>> remove the 2 first points, without modifying the logic of the current
>> code, while the 3rd point will substitute a parameter passing to a
>> session storage.
>
>
> Thanks for listing reasons.
>
> I think it would help people to know what size performance
> improvements you are seeing.  

I have no ideas, but what I can tell is that it can't be faster with 
chaining than without, so if we can switch to an easier pattern there 
will be some gain. But this is not the main issue. Decomposition is much 
more important.

> It would help MINA developers to know
> whether to use the IoHandlerChain or not.

If you are going to use a Chain patter, then I guess that the important 
point is to think about the problem it solves. For instance, monitoring 
by inserting loggers could be a good usage. But the main advantage of 
chain parttern is its versatility : you can change the order of 
handlers, or simply add or remove handlers on demand. If you never do 
that, then using a chain pattern is questionable (IMHO).

>
> I agree that we should move to a simpler method based structure.

At this point, we have three ways to go :
1) one big method, handling everything, like what did the BindHandler
2) one single class, with many methods into it, mapped directly on each 
handler
3) many classes, each of them containing static methods, called by a 
messageHandler, in the order defined by the previous chain.

I would say that method 1 is a disaster when it comes to maintain it : 
far too complex, difficult to understand, difficult to fix, impossible 
to maintain.

Method 2 is slightly better, because it keeps the decompaosition as it 
was in the chain. The main problem is that you can easily factorize 
their usage, and it's impossible to tests, as each method is private

So I would favor method 3, where we don't use instances of classes, but 
static methods. So each IoHandlerCommand inherited class can be kept 
with a very minor refactoring.

Emmanuel

>
> Enrique
>

Re: Moving from IoHandlerChain to something easier to debug

[Enrique Rodriguez <en...@gmail.com>]

On 6/21/07, Emmanuel Lecharny <el...@gmail.com> wrote:
> ...
> I would suggest to switch from this pattern to a simpler one, where we
> keep the handler decomposition (I like the idea of decomposing
> treatments in small disconnected methods, compared to a big fat
> method), but using as many methods as we have handlers. Doing so will
> remove the 2 first points, without modifying the logic of the current
> code, while the 3rd point will substitute a parameter passing to a
> session storage.

Thanks for listing reasons.

I think it would help people to know what size performance
improvements you are seeing.  It would help MINA developers to know
whether to use the IoHandlerChain or not.

I agree that we should move to a simpler method based structure.

Enrique