Hi all,

We've talked about refactoring the current IoFilterChain implementation, and
I'd like to address some issues:

1) There should be only *one* NextFilter implementation for all filters in
the same chain for thread safety, and upstream/downstream (forwarding)
operations should be executed in head or tail filter. It can be overriden by
overriding createHeadFilter and createTailFilter. (and this is the same way
Dave suggested to us, but the current implementation hides head and tail
from the filter list to keep OOP encapsulation principal 

2) We need to carefully consider if we have any other use case than having
concrete number of filter chain. I think we can have only three chains per
session at maximum. So I'm afraid this is an overengineering. Of course we
can provide more than one chain in session level, but I think we don't need
it actually because we can simply append the chain by adding
IoFilterChain.addAllLast() and addAllFirst(), and etc.

3) Should we pass IoHandler as a parameter even if I can get it
IoSession.getHandler()? Please let me know if I am missing something.

4) We cannot simply expose IoFilterChain.setIoProcessor() method because it
can break MINA so easily. So I'd like to suggest hiding setIoProcessor()
like this:

public void setFilterChain( IoFilterChain chain ) {
this.chain = new TransportTypeSpecificFilterChain( chain ); //
TransportTypeSpecificFilterChain extends IoFilterChainProxy.
}

public IoFilterChain getFilterChain() {
return this.chain.getFilterChain(); // Unwrap
}

Only TransportTypeSpecificFilterChain will provide setIoHandler or something
like that. But I think we already have doWrite and doClose in
AbstrctIoFilterChain and there are existing subclasses that implement them.
So I don't see much difference here except the behavior of
get/setFilterChain().

And please note that per-port and per-session filter chain should be cleared
(destroying all filters) when port is unbound or session is closed. This
means they are one-time use only because destroy call on a filter means
removal from a chain and vice versa. So we cannot have
IoSession.setFilterChain() because the specified chain will be cleared when
the connection is closed so it will prevent from specifying the same chain
for more than one sessions.

The biggest problem is that IoFilter.init() and destroy() is invoked
together when IoFilter.add(), remove(), and clear() is invoked. (It's
because the filter can be added or removed at any time.) To do so, IoFilter
has to remember who its parent is (IoSession or IoSessionManager). This
means we have to specify the correct parent when we construct a chain
unfortunately.

So I have to admit that there are so much complexity unfortunately to
provide setFilterChain() simply. Any solutions?

Not let's consider Jose's IoFilterChainBuilder. It is really simple solution
and work without any big changes. The only problem with the builder pattern
is that it can be abused to contain more than just building. A user can even
call System.exit(0) there.  But it is entirely up to users eventually. So
I'd like to choose Jose's approach. I thought his approach adds more
complexity to the API, but now I think it is a way simpler and easy to
understand.

Cheers,
Trustin
--
what we call human nature is actually human habit
--
http://gleamynode.net/

[ Post a follow-up to this message ]

2005/11/15, Trustin Lee <trustin-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>:
>
> 1) There should be only *one* NextFilter implementation for all filters in
> the same chain for thread safety, and upstream/downstream (forwarding)
> operations should be executed in head or tail filter. It can be overridenb
y
> overriding createHeadFilter and createTailFilter. (and this is the same wa
y
> Dave suggested to us, but the current implementation hides head and tail
> from the filter list to keep OOP encapsulation principal 


I forgot to mention that we can remove createHead/TailFilter and let users
to specify them as constructor parameters.

Not let's consider Jose's IoFilterChainBuilder. It is really simple solution
> and work without any big changes. The only problem with the builder patter
n
> is that it can be abused to contain more than just building. A user can ev
en
> call System.exit(0) there.  But it is entirely up to users eventually.
> So I'd like to choose Jose's approach. I thought his approach adds more
> complexity to the API, but now I think it is a way simpler and easy to
> understand.


Not -> Now

Trustin
--
what we call human nature is actually human habit
--
http://gleamynode.net/

[ Post a follow-up to this message ]

Hi Niklas,

2005/11/15, Niklas Therning <niklas-8FIgwK2HfyIwFerOooGFRg@public.gmane.org>:
>
> If we forget about the IoFilterChain.getNextFilter() methods for the
> moment I don't really understand why there has to be one NextFilter.


It is because NextFilter can be cached by a filter in case it have to emit
the event even if any I/O event or request didn't occur. For example, there
can be a customized dynamic timeout filter.

With the solution I and Dave talked about Entry.nextFilter would go
> away. They would have to be created on the fly for each filter
> invocation in a chain. I realize that it wouldn't be possible to
> implement IoFilterChain.getNextFilter() if Entry.nextFilter wouldn't
> exist but are they really needed? If we are going to chain chains
> without copying them we will have to propagate the next IoHandler
> through the chain at filter time.


You're right. getNextFilter() is not actually required to be exposed, but it
is still true that a filter should be able to emit any event at any time
using nextFilter instance it cached.

I'm only interested in having at most three chains really. One tied to
> the SessionManager. This is where I would put the ThreadPoolFilter. One
> tied to the port. This is where I would put the SSLFilter. And one
> private to the IoSession. There should not be an
> IoSession.setFilterChain() method.


I see. 

> 3) Should we pass IoHandler as a parameter even if I can get it 
>
> The SessionManager (or SocketIoProcessor) will simply call
>
> this.filterChain.sessionOpened( session, session.getHandler() );
>
> If this.filterChain is Dave's ChainedFilterChain
> this.filterChain.sessionOpened() could look like:
>
> public void sessionOpened( session, handler ) {
> this.firstChain.sessionOpened( session, new
> IoFilterToIoHandlerAdapter(handler, this.secondFilter)) );
> }


I didn't get it yet. Can't we simply override head/tail filter for each
chain?

Yes, setIoProcessor() isn't very nice, I agree. And I don't have a
> solution for it except that copying the chain passed to setFilterChain()
> into something like an IoProcessorAwareFilterChain. I can't see how the
> above solves this. How does the wrapped chain know where to route
> write() / close() calls? Or is it just to prevent setIoProcessor() from
> being called?


Yes that's also a problem... so we have to copy the chain. (Sorry for the
confusion ;) But copying the chain rises another problem; init() and
destroy() is called immediately when a filter is added or removed.

> The biggest problem is that IoFilter.init () and destroy() is invoked 
>
> I don't understand. Could you describe the problem a bit more?


As you know, there's no init() or destroy() in IoFilterChain. This means
there's no life cycle in IoFilterChain. The life cycle only exists in
filter-level. So when you add a filter to a chain, IoFilter.init() is called
imediately to initialize the filter. And IoFilter.destroy() is invoked when
the filter is removed from the chain. I didn't put init() or destroy() to
IoFilterChain because filters can be added and removed whenever a user
wants. User could even add filters after init() is invoked, and then we
should call IoFilter.init() immediately, so there's no much reason to
provide init() and destroy() in IoFilterChain.

So we know we have to call init() and destroy() when a filter is added. The
init() and destroy() method requires an IoFilterChain parameter as a parent.
.
(I made a mistake in my previous post. It requires IoFilterChain as a
parent) It is because one filter instance can be added to more than one
chains.

Now let's talk about cloning the chain. Take a look at this code:

IoFilterChain newFilterChain = new ...;
IoFilter filterA = new FilterA();
newFilterChain.addLast( "a", filterA);

acceptor.setFilterChain( newFilterChain ); // internally newFilterChain is
cloned.

This code will call filterA.init() twice with different parent
(IoFilterChain). This means user have to call newFilterChain.clear()
explicitly. Of course user will have to call acceptor.getFilterChain().clear
()
when it shuts down, but two calls and one call are different IMHO.

Yes it's complex. But I wouldn't say that the current implementation is
> much simpler. It took quite some time and debugging to figure out how
> the filter chains work. (Maybe that's just me being really stupid ;) )
> What I like about this approach (except for setIoProcessor() which isn't
> really acceptable) is that the configuration of chains are totally
> detached and independent from the SessionManager.


Right, I love that advantage you mentioned. To work around the init() and
destroy() issue, the IoFilterChain implementation that user can construct
and that doesn't invoke init() and destroy() at all. But it's too tricky.
What about just introducing IoFilterConfig with minimal interface?

public interface IoFilterConfig { // perhaps we can implement is as a
concrete class simply.
public String getName();
public IoFilter getFilter();
}

and accepting a List of IoFilterConfigs?

If so, the implemtation of IoAcceptor.setFilterChain() will be...

public void setFilterChain(List filterConfigs) throws Exception {
this.chain.clear();
for( Iterator i = filterConfigs.iterator(); i.hasNext(); ) {
Object o = i.next();
if( o instanceof IoFilterConfig ) {
IoFilterConfig cfg = ( IoFilterConfig ) o;
this.chain.addLast( cfg.getName(), cfg.getFilter() );
} else if( o instanceof IoFilter ) {
IoFilter filter = ( IoFilter ) o;
this.chain.addLast( someAnonymousNameBasedOnFilterType, cfg.getFilter() );
}
}
}

WDYT?

Maybe I have totally misunderstood what Jose meant but I cant see how
> that would simplify anything. You would still have the problem of wiring
> the three chains together. Or am I missing something fundamental? As I
> said previously, the wiring of chains is problem number 1, how to
> configure per-port chains is problem number 2.


For problem #1, yes, we still have to resolve that problem. But Jose's
approach solves Problem #2. We can pass port-level filter chain to the
builder.

Cheers,
Trustin
--
what we call human nature is actually human habit
--
http://gleamynode.net/

[ Post a follow-up to this message ]

2005/11/15, Niklas Therning <niklas-8FIgwK2HfyIwFerOooGFRg@public.gmane.org>:
>
> I just realized that when calling IoSession.write() the concrete
> IoSession implementation will always know the final destination of the
> write event. For SocketSessionImpl the final destination is always
> SocketIoProcessor.
>
> So the IoFilterChain methods for downstream events look like this:
>
> void write( IoSession session, WriteRequest writeRequest, IoProcessor p
> ) throws Exception;
>
> SocketSessionImpl's write() method looks like this:
>
> public WriteFuture write( Object message ) {
> this.filterChain.write(this, new WriteRequest(),
> SocketIoProcesso.getInstance());
> }


What is the difference from overriding the head filter of
sessionmanager-level chain to call SocketIoProcessor.flush() explicitly? We
can still get rid of SocketSessionManagerFilterChain without introducing a
new parameter.

I'll check in this change so you can see it more clearly.

Cheers,
Trustin
--
what we call human nature is actually human habit
--
http://gleamynode.net/

[ Post a follow-up to this message ]

I too agree that setIoProcessor isn't all that nice - and what I was
trying to think about last night was ways to get away from it.
Im not so tired this morning (although 4 hours sleep didn't help
much...) but I'll try to explain my approach a bit more clearly.
It kind of boils down to a few points:

1) We want to chain "sequences" of filters (a filter chain) together
2) Soon we want to be able to let users build their own filters=20
3) We want to preserve OOP

To me, the current filter chain / filter implementation looks quite
complicated. I haven't got familiar enough with the code yet to know
whether ths complexity is an unfortunate necessity.
Just so we know what my proposed approach is, I'll try and describe the
basics here:

1) I don't see how refactoring the chains affects Jose's approach. If we
have individual chains, we can still pass them to a builder to be
populated
2) Why not make IoFilterChain move towards the composite pattern? A
filter chain is just a special type of filter which filters in a
sequence. No special head, no special tail. Just a sequence 0..n. So
given some "BasicFilterChain" impl, we can add both individual filters
and filter chains to the chain. NextFilters can ** still ** be cached by
individual filters - no change there.
3) I think filters can still be used ** without ** cloning and without
special "setIoProcessor" methods. Asume that we have (2). When a new
connection is established, all we have to do is "hook up" our sub
chains:

BasicFilterChain connectionChain =3D new BasicFilterChain();
connectionChain.addLast(sessionManagerChain);=20
connectionChain.addLast(portChain);
BasicFilterChain sessionChain =3D new BasicFilterChain();
someChainBuilder.buildChain(sessionChain); // Jose's approach
connectionChain.addLast(sessionChain);

connectionChain.add(endOfTheLineFilter);

And that's it - connectionChain becomes the uniqueue chain for a
connection. The last addition, the "endOfTheLineFilter" - is just a
filter that does the "real" work (like Niklas' IoProcessor approach -
but now encapsulated in a filter). Because this is per-connection, the
"endOfTheLine" filter can be provided with the collaborators it needs to
do its job without affecting any other filter.
This doesn't require any "cloning" of chains, or creation of NextFilters
per traversal either.

This to me seems like a much cleaner approach - but I must be missing
something?

Dave


This e-mail and any attachment is for authorised use by the intended recipi=
ent(s) only. It may contain proprietary material, confidential information =
and/or be subject to legal privilege. It should not be copied, disclosed to=
, retained or used by, any other party. If you are not an intended recipien=
t then please promptly delete this e-mail and any attachment and all copies=
and inform the sender. Thank you.

[ Post a follow-up to this message ]

Hi Dave,

2005/11/15, Irving, Dave <dave.irving-iKsOTpgdUR76V6G2DxALlg@public.gmane.org>:
>
> To me, the current filter chain / filter implementation looks quite
> complicated. I haven't got familiar enough with the code yet to know
> whether ths complexity is an unfortunate necessity.


Yes, it is really complex. 

Just so we know what my proposed approach is, I'll try and describe the
> basics here:
>
> 1) I don't see how refactoring the chains affects Jose's approach. If we
> have individual chains, we can still pass them to a builder to be
> populated


Right there's no much relationship.

2) Why not make IoFilterChain move towards the composite pattern? A
> filter chain is just a special type of filter which filters in a
> sequence. No special head, no special tail. Just a sequence 0..n. So
> given some "BasicFilterChain" impl, we can add both individual filters
> and filter chains to the chain. NextFilters can ** still ** be cached by
> individual filters - no change there.
> 3) I think filters can still be used ** without ** cloning and without
> special "setIoProcessor" methods. Asume that we have (2). When a new
> connection is established, all we have to do is "hook up" our sub
> chains:
>
> BasicFilterChain connectionChain = new BasicFilterChain();
> connectionChain.addLast(sessionManagerChain);
> connectionChain.addLast(portChain);
> BasicFilterChain sessionChain = new BasicFilterChain();
> someChainBuilder.buildChain(sessionChain); // Jose's approach
> connectionChain.addLast(sessionChain);
>
> connectionChain.add(endOfTheLineFilter);


Explicitly asking users to add head/tail filter is a bad idea. But I think
it is a great idea. But we don't need to make a chain be nested more than
one level. And head and tail filter must be hidden from user to prevent from
unexpected removal or reordering.

Cheers,
Trustin
--
what we call human nature is actually human habit
--
http://gleamynode.net/

[ Post a follow-up to this message ]

Hi,

2005/11/15, Niklas Therning <niklas-8FIgwK2HfyIwFerOooGFRg@public.gmane.org>:
>
> Trustin Lee wrote:
> 
> If the user can change the filter chain of an IoAcceptor by calling
> setFilterChain() you won't be able to override any method to inject this
> behaviour. Likewise if there is a bind(address, chain, handler) method.
> Unless you copy the user supplied chain into some IoFilterChain
> implementation which we have control over. And from what I understand
> copying is out of the question. Or am I missing something?


If we're going to use setfilterChain only for the initial configuration,
then we can copy. Otherwise we cannot. The problem here is that we cannot
easily reuse IoFilterChain because it contains too much information.

Trustin
--
what we call human nature is actually human habit
--
http://gleamynode.net/

[ Post a follow-up to this message ]

>  Explicitly asking users to add head/tail filter is a bad idea.   
 
I agree. In fact - thats my point :o)
 
>  But I think it is a great idea.  But we don't need to make a chain be
nested more than one level.   
>  And head and tail filter must be hidden from user to prevent from
unexpected removal or reordering. 
 
Exactly. The ** user ** doesn't write the code above: This is in the
acceptor (or whatever).
The idea is that the user can contribute a chain (Jose's approach) how
ever they like.
The special "head tail" behaviour is encapsulated to the client who
requires it: I.e, the acceptor / connector.
 
This is what I like about the idea. Currently, if you write a filter
chain, you need to care about that special head tail behaviour. Using
this approach you wouldn't - because a chain is nothing more than a
sequence of filters.
Its the acceptor / connector who cares about the head / tail behaviour -
so he can specify it directly.
Hopefully this example will clear up the creational logic:
 
Imagine this code is in SocketAcceptorDelegate.Worker#processSessions -
when we've just found out we've got a new session:
 
// Create the filter chain for the new connection / session
BasicFilterChain connectionChain = new BasicFilterChain();   
// Add the session manager chain to the connection chain
connectionChain.addLast(sessionManagerChain); 
// Add the per-port chain to the connection chain 
connectionChain.addLast(portChain); 
// Let the user contribute to a new per-session chain (Jose's approach)
BasicFilterChain sessionChain = new BasicFilterChain();  
someChainBuilder.buildChain(sessionChain);
// Add the per-session filter to the connection chain
connectionChain.addLast(sessionChain); 
 
// Now create the filter which does ** our ** work (i.e, real handler
writing)
IoFilter workerFilter = new EndOfTheLineFilter(); 
connectionChain.add( workerFilter );
 
Notice that the user isn't specifying the worker filter - the
SocketAcceptorDelegate is.
So I belive this gives us real benefits:
 
1) Makes the filter chain mechanism simpler (reduces complexity as we're
now using composition rather than inheritance)
2) Paves the way for user contribution to chains (Jose's approach)
3) Doesn't require filter cloning or per-traversal clones
4) Is OO: The acceptor / connector is the actor who wants to specify
"end of chain" behaviour: So he does so by means of adding a custom
filter to the end of the built chain
 
Does this clear it up at all?
What do you think about this approach?
 
Many thanks,
 
Dave (give me sleep) Irving
 


This e-mail and any attachment is for authorised use by the intended recipie
nt(s) only. It may contain proprietary material, confidential information an
d/or be subject to legal privilege. It should not be copied, disclosed to, r
etained or used by, any other party. If you are not an intended recipient th
en please promptly delete this e-mail and any attachment and all copiesand i
nform the sender. Thank you.

[ Post a follow-up to this message ]

> Its the acceptor / connector who cares about the head / tail behaviour
- so he can specify it directly
 
Just to clarify: With this approach there is no explicit head / tail.
The SocketAcceptorDelegate et al just add their work as the last filter
in the per-connection chain.
The user is never exposed to the whole per connection chain (they only
ever see the per session chain) - so it is impossible for them to tinker
with it
 
Dave

________________________________

From: Irving, Dave [mailto:dave.irving-iKsOTpgdUR76V6G2DxALlg@public.gma
ne.org] 
Sent: 15 November 2005 09:41
To: Apache Directory Developers List
Subject: RE: [mina] Refactoring MINA IoFilterChain (Was: IoFilters:
DIRMINA-121 / 122)



>  Explicitly asking users to add head/tail filter is a bad idea.   
 
I agree. In fact - thats my point :o)
 
>  But I think it is a great idea.  But we don't need to make a chain be
nested more than one level.   
>  And head and tail filter must be hidden from user to prevent from
unexpected removal or reordering. 
 
Exactly. The ** user ** doesn't write the code above: This is in the
acceptor (or whatever).
The idea is that the user can contribute a chain (Jose's approach) how
ever they like.
The special "head tail" behaviour is encapsulated to the client who
requires it: I.e, the acceptor / connector.
 
This is what I like about the idea. Currently, if you write a filter
chain, you need to care about that special head tail behaviour. Using
this approach you wouldn't - because a chain is nothing more than a
sequence of filters.
Its the acceptor / connector who cares about the head / tail behaviour -
so he can specify it directly.
Hopefully this example will clear up the creational logic:
 
Imagine this code is in SocketAcceptorDelegate.Worker#processSessions -
when we've just found out we've got a new session:
 
// Create the filter chain for the new connection / session
BasicFilterChain connectionChain = new BasicFilterChain();   
// Add the session manager chain to the connection chain
connectionChain.addLast(sessionManagerChain); 
// Add the per-port chain to the connection chain 
connectionChain.addLast(portChain); 
// Let the user contribute to a new per-session chain (Jose's approach)
BasicFilterChain sessionChain = new BasicFilterChain();  
someChainBuilder.buildChain(sessionChain);
// Add the per-session filter to the connection chain
connectionChain.addLast(sessionChain); 
 
// Now create the filter which does ** our ** work (i.e, real handler
writing)
IoFilter workerFilter = new EndOfTheLineFilter(); 
connectionChain.add( workerFilter );
 
Notice that the user isn't specifying the worker filter - the
SocketAcceptorDelegate is.
So I belive this gives us real benefits:
 
1) Makes the filter chain mechanism simpler (reduces complexity as we're
now using composition rather than inheritance)
2) Paves the way for user contribution to chains (Jose's approach)
3) Doesn't require filter cloning or per-traversal clones
4) Is OO: The acceptor / connector is the actor who wants to specify
"end of chain" behaviour: So he does so by means of adding a custom
filter to the end of the built chain
 
Does this clear it up at all?
What do you think about this approach?
 
Many thanks,
 
Dave (give me sleep) Irving
 



This e-mail and any attachment is for authorised use by the intended
recipient(s) only. It may contain proprietary material, confidential
information and/or be subject to legal privilege. It should not be
copied, disclosed to, retained or used by, any other party. If you are
not an intended recipient then please promptly delete this e-mail and
any attachment and all copies and inform the sender. Thank you.

[ Post a follow-up to this message ]

Niklas Therning wrote:

>I really like your approach, Dave. Some question though:
> What would session.getFilterChain() return? It will be sessionChain,
right?

Yes. This allows IoHandlers or who-ever to contribute to the session
chain.=20
They ** never ** get access to the "connection chain" (which is a
private implementation detail of SocketAcceptorDelegate etc).

> Will IoFilterChain extend IoFilter and be just another IoFilter?

Depends. I think at the moment there's not all that much need for true
composite - as Trustin says.
However, I don't think its any more or less work either way - and I
think its cleaner to have a chain as a special type of filter.=20
However, this ** could ** be blown away by the NextFilter problem.

> This is basically the same as nesting filter chains as I proposed
yesterday (remember the (((A, B), C), D) thingy?)=20
> but it's a flat list of chains instead, right?

Yeah - I think we were both talking about the same thing all along but
didn't realise it in our respective tiredness :o)

> You still have the NextFilter problem.=20
> As I understand it these have to be static since IoFilter.init() is
called when the filter is added to a chain.=20
> A NextFilter is passed to this method and the filter could remember
this for future use.

Yes - I certainly do have a NextFilter problem. This is quite similar to
the "this problem" often found when employing the decorator pattern.
Im going to spend some time this morning thinking about how we can get
round it cleanly and transparently: But I understand that support for
NextFilter caching is required, important, and must not be lost.

> I can't see how the last filter in a wrapped chain will know where to
go next if the NextFilter given=20
> to it can't be created dynamically.=20
> Remember, sessionManagerChain and portChain will be shared by many
connectionChains.

Yep - Im going to have to think about that this morning / this
afternoon.
However, I believe that finding a solution is worth it - as Im sure
we're on to a clean solution :o)


> /Niklas

Dave


This e-mail and any attachment is for authorised use by the intended recipi=
ent(s) only. It may contain proprietary material, confidential information =
and/or be subject to legal privilege. It should not be copied, disclosed to=
, retained or used by, any other party. If you are not an intended recipien=
t then please promptly delete this e-mail and any attachment and all copies=
and inform the sender. Thank you.

[ Post a follow-up to this message ]