onPreAdd, PostAdd, remove : correction

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

Hi,

those methods are called when a new filter is registred into the chain. 
However, the few filters implementing those methods typically check that 
the added filter is not already present in the chain :

ProtocolCodecFilter, ExecutorFilter, KeepAliveFilter :
    public void onPreAdd(IoFilterChain parent, String name,
            NextFilter nextFilter) throws Exception {
        if (parent.contains(this)) {
            throw new IllegalArgumentException(
                    "You can't add the same filter instance more than 
once.  Create another instance and add it.");

but SslFilter does some intrigating things, which seems to me deserves 
to be found in a init() method.

IMO, pre/post operation are just a bit too much. The way it should be 
done is :
- init the filter
- add/remove a filter from the chain

and that's it.

Anyone has a better vision of what those method are good for ?


-- 
--
cordialement, regards,
Emmanuel Lécharny
www.iktek.com
directory.apache.org

Re: onPreAdd, PostAdd, remove : correction

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

Mark Webb wrote:
> If they are not being used, delete them.  I can say that I have never used them.
>   
After further look at the code, I found that SslFilter relies deeply on 
those guys. I still have to understand why.

This is something we have to do : we have a hell lot of filters in core, 
with many of them totally undocumented, which makes it difficult to do 
modifications on the code. I have started to document some of the 
filters a few weeks ago :
http://cwiki.apache.org/confluence/display/MINA/IoFilter
but it's far from being perfect.

We have to go further and then we will be able to clean those filters 
(many of them aren't using the same logic, as they have been created at 
different point in the pass).

Thanks Mark !

-- 
--
cordialement, regards,
Emmanuel Lécharny
www.iktek.com
directory.apache.org

Re: onPreAdd, PostAdd, remove : correction

[Mark Webb <el...@gmail.com>]

If they are not being used, delete them.  I can say that I have never used them.

--Mark

On Sat, Nov 8, 2008 at 5:43 PM, Emmanuel Lecharny <el...@gmail.com> wrote:
> Hi,
>
> those methods are called when a new filter is registred into the chain.
> However, the few filters implementing those methods typically check that the
> added filter is not already present in the chain :
>
> ProtocolCodecFilter, ExecutorFilter, KeepAliveFilter :
>   public void onPreAdd(IoFilterChain parent, String name,
>           NextFilter nextFilter) throws Exception {
>       if (parent.contains(this)) {
>           throw new IllegalArgumentException(
>                   "You can't add the same filter instance more than once.
>  Create another instance and add it.");
>
> but SslFilter does some intrigating things, which seems to me deserves to be
> found in a init() method.
>
> IMO, pre/post operation are just a bit too much. The way it should be done
> is :
> - init the filter
> - add/remove a filter from the chain
>
> and that's it.
>
> Anyone has a better vision of what those method are good for ?
>
>
> --
> --
> cordialement, regards,
> Emmanuel Lécharny
> www.iktek.com
> directory.apache.org
>
>
>

RE: onPreAdd, PostAdd, remove : correction no---signature

[Steve Ulrich <st...@proemion.com>]

> Emmanuel Lecharny [mailto:elecharny@gmail.com] wrote:
>
> I'm not sure I see what you mean here. Let me clarify :
> - should we have init/destroy(session) methods replacing the
> preAdd/postRemover methods ?

Erm, Yes that's what I meant.

> > This way, the "if (chain.contains(this))" could be removed from the
> filter implementations.
> >
> Those checks are pretty painful. But if the filter can be added only
> once by design, we need to have them, otherwise we rely on user's
> wisdom, which is dangerous ;). Ie, I think they are like a safety net
> for the base case. Not sure we need them though, this is much a design
> decision than a technical one.

Yes, they can take a long time, but think about how long a filter chain
is and how often a filter is added or removed.
Nonetheless a lot of filters does this check already and some more
doesn't but should do...

> > Sounds a bit comlicated from a users point of view. Maybe we can use
> an interface here, too.
> > The add and remove method can check if the filter implements the
> interface and call the method as needed.
> >
> Yeah. I have to rethink this part a bit...
>
> Thanks !

No problem, an even better design of mina makes my life even more easier ;)
So I have to thank you for reviewing.

Steve

Re: onPreAdd, PostAdd, remove : correction no---signature

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

Steve Ulrich wrote:
>> Seriously, you make a good point here, and enlighted the reason we have
>> these methods. And after a day looking at those preAdd and postRemove
>> (well, not a full day !), I think it's better to have those two guys
>> called directly when you do a add() and remove() than to have an
>> explicit init() method, generally speaking.  But there is another
>>     
>
> Hmm.. dedicated init and destroy methods would help to make the code clean. If they don't exists, some will call it preAdd/postRemove, others init/destroy, beforeInsert/afterRemove...
> Additional (and increasing with the number of method-pair-names) there's a possibility to forget to call the methods and just doing add/remove and getting errors.
>   
I'm not sure I see what you mean here. Let me clarify :
- should we have init/destroy(session) methods replacing the 
preAdd/postRemover methods ?
- or should we have init/destroy methods not related to the session, 
with the pre/post methods ?
> <snip/>
>> so : I think that we need to rethink about what those two methods do,
>> in
>> order to protect the user against dangerous hidden mistakes...
>>     
>
> IMO, the normal case is that a filter will only be added once. 
Absolutely. But some peeps are using some filters more than once. For 
instance, Ashish is using the ExecutorFilter more than once to reproduce 
a SEDA like system. (IMHO, a very special use case).
> If they're added more than one time, the filter must be responsible for that.
>   
Agreed.
> Maybe an additional interface can provide the neccessary methods to handle that case. If the Filter does not implement it, it can only be added once.
>   
Yep. Good idea.
> This way, the "if (chain.contains(this))" could be removed from the filter implementations.
>   
Those checks are pretty painful. But if the filter can be added only 
once by design, we need to have them, otherwise we rely on user's 
wisdom, which is dangerous ;). Ie, I think they are like a safety net 
for the base case. Not sure we need them though, this is much a design 
decision than a technical one.
> <snip/>
>> - if we don't need those initialization/destruction, then default to a
>> flag being set indication the filter's status (initialized/removed)
>> - throw an exception if the status is incorrect when he use the filter
>> (protecting the user if he forgets to initialize the filter).
>>     
>
> Sounds a bit comlicated from a users point of view. Maybe we can use an interface here, too.
> The add and remove method can check if the filter implements the interface and call the method as needed.
>   
Yeah. I have to rethink this part a bit...

Thanks !

-- 
--
cordialement, regards,
Emmanuel Lécharny
www.iktek.com
directory.apache.org

RE: onPreAdd, PostAdd, remove : correction no---signature

[Steve Ulrich <st...@proemion.com>]

> Emmanuel Lecharny [mailto:elecharny@gmail.com] wrote:
>
> Steve Ulrich wrote:
> >
> > An init() method doesn't know anything about the session so far.
> >
> I was specifically thinking if a init(session) method. We also need a
> destroy(session) method. Eh, call them preAdd and postRemove ;)

Okay :-)

> Seriously, you make a good point here, and enlighted the reason we have
> these methods. And after a day looking at those preAdd and postRemove
> (well, not a full day !), I think it's better to have those two guys
> called directly when you do a add() and remove() than to have an
> explicit init() method, generally speaking.  But there is another

Hmm.. dedicated init and destroy methods would help to make the code clean. If they don't exists, some will call it preAdd/postRemove, others init/destroy, beforeInsert/afterRemove...
Additional (and increasing with the number of method-pair-names) there's a possibility to forget to call the methods and just doing add/remove and getting errors.

> problem :
>
> If you look at the existing javadoc :
>
>     /**
>      * Invoked by {@link ReferenceCountingFilter} when this filter
>      * is added to a {@link IoFilterChain} at the first time, so you
> can
>      * initialize shared resources.  Please note that this method is
> never
>      * called if you don't wrap a filter with {@link
> ReferenceCountingFilter}.
>      */
>     void init() throws Exception;

ouch!

>     /**
>      * Invoked before this filter is added to the specified
> <tt>parent</tt>.
>      * Please note that this method can be invoked more than once if
>      * this filter is added to more than one parents.  This method is
> not
>      * invoked before {@link #init()} is invoked.
>      */
>     void onPreAdd(IoFilterChain parent, String name, NextFilter
> nextFilter)
>             throws Exception;
>
> You can see that the init is used in a very specific case : when you
> wrap the filter with ReferenceCountingFilter (seems like an abusive
> usage to me), and that the onPreAdd may be called more than once, which
> seems also abusive when you know that we will only have one instance of
> this filter, leading potentially to many mistakes if the initialization
> done during this phase is done more than once.
> so : I think that we need to rethink about what those two methods do,
> in
> order to protect the user against dangerous hidden mistakes...

IMO, the normal case is that a filter will only be added once. If they're added more than one time, the filter must be responsible for that.
Maybe an additional interface can provide the neccessary methods to handle that case. If the Filter does not implement it, it can only be added once.
This way, the "if (chain.contains(this))" could be removed from the filter implementations.

> I'm going to propose something slightly better for those methods :
> - remove the postAdd and preRemove methods
> - rename the preAdd/postRemove to init(session) and destroy(session),

agreed

> and call them explicitly _before_ we remove or add this filter from the
> session chain

see above comment.

> - if we don't need those initialization/destruction, then default to a
> flag being set indication the filter's status (initialized/removed)
> - throw an exception if the status is incorrect when he use the filter
> (protecting the user if he forgets to initialize the filter).

Sounds a bit comlicated from a users point of view. Maybe we can use an interface here, too.
The add and remove method can check if the filter implements the interface and call the method as needed.

> I'm not completely convinced that I have the best possible idea in this
> area, I'm just trying to be sure that we don't miss something important
> here. feel free to comment and tell if I'm wrong !

regards

Steve

Re: onPreAdd, PostAdd, remove : correction no---signature

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

Steve Ulrich wrote:
>> Emmanuel Lecharny [mailto:elecharny@gmail.com] wrote
>>
>> Hi,
>>     
>
> Hi!
>
> I have already used them for some session specific initialization.
>   
Makes sense.  Seems that pre-add and post-remove can be useful, as you 
say later in your reply.

<snip/>
>> but SslFilter does some intrigating things, which seems to me deserves
>> to be found in a init() method.
>>     
>
> An init() method doesn't know anything about the session so far.
>   
I was specifically thinking if a init(session) method. We also need a 
destroy(session) method. Eh, call them preAdd and postRemove ;)

Seriously, you make a good point here, and enlighted the reason we have 
these methods. And after a day looking at those preAdd and postRemove 
(well, not a full day !), I think it's better to have those two guys 
called directly when you do a add() and remove() than to have an 
explicit init() method, generally speaking.  But there is another problem :

If you look at the existing javadoc :

    /**
     * Invoked by {@link ReferenceCountingFilter} when this filter
     * is added to a {@link IoFilterChain} at the first time, so you can
     * initialize shared resources.  Please note that this method is never
     * called if you don't wrap a filter with {@link 
ReferenceCountingFilter}.
     */
    void init() throws Exception;

    /**
     * Invoked before this filter is added to the specified <tt>parent</tt>.
     * Please note that this method can be invoked more than once if
     * this filter is added to more than one parents.  This method is not
     * invoked before {@link #init()} is invoked.
     */
    void onPreAdd(IoFilterChain parent, String name, NextFilter nextFilter)
            throws Exception;

You can see that the init is used in a very specific case : when you 
wrap the filter with ReferenceCountingFilter (seems like an abusive 
usage to me), and that the onPreAdd may be called more than once, which 
seems also abusive when you know that we will only have one instance of 
this filter, leading potentially to many mistakes if the initialization 
done during this phase is done more than once.
so : I think that we need to rethink about what those two methods do, in 
order to protect the user against dangerous hidden mistakes...

I'm going to propose something slightly better for those methods :
- remove the postAdd and preRemove methods
- rename the preAdd/postRemove to init(session) and destroy(session), 
and call them explicitly _before_ we remove or add this filter from the 
session chain
- if we don't need those initialization/destruction, then default to a 
flag being set indication the filter's status (initialized/removed)
- throw an exception if the status is incorrect when he use the filter 
(protecting the user if he forgets to initialize the filter).

I'm not completely convinced that I have the best possible idea in this 
area, I'm just trying to be sure that we don't miss something important 
here. feel free to comment and tell if I'm wrong !

Thanks !




-- 
--
cordialement, regards,
Emmanuel Lécharny
www.iktek.com
directory.apache.org

RE: onPreAdd, PostAdd, remove : correction no---signature

[Steve Ulrich <st...@proemion.com>]

> Emmanuel Lecharny [mailto:elecharny@gmail.com] wrote
>
> Hi,

Hi!

I have already used them for some session specific initialization.

> those methods are called when a new filter is registred into the chain.
> However, the few filters implementing those methods typically check
> that
> the added filter is not already present in the chain :
>
> ProtocolCodecFilter, ExecutorFilter, KeepAliveFilter :
>     public void onPreAdd(IoFilterChain parent, String name,
>             NextFilter nextFilter) throws Exception {
>         if (parent.contains(this)) {
>             throw new IllegalArgumentException(
>                     "You can't add the same filter instance more than
> once.  Create another instance and add it.");
>
> but SslFilter does some intrigating things, which seems to me deserves
> to be found in a init() method.

An init() method doesn't know anything about the session so far.

> IMO, pre/post operation are just a bit too much. The way it should be
> done is :
> - init the filter
> - add/remove a filter from the chain

I can't imagine a good reason to use postAdd or preRemove, but a lot for preAdd and postRemove.

> and that's it.
>
> Anyone has a better vision of what those method are good for ?

They are good to initialize or cleanup session specific contexts.

regards

Steve