directory-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Niklas Therning <nik...@trillian.se>
Subject Re: [mina] Refactoring MINA IoFilterChain (Was: IoFilters: DIRMINA-121 / 122)
Date Tue, 15 Nov 2005 07:30:00 GMT
Trustin Lee wrote:

> 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 :)

If we forget about the IoFilterChain.getNextFilter() methods for the
moment I don't really understand why there has to be one NextFilter.
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.

>
> 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.

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.

>
> 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.

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)) );
}

>
> 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().

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?

>
> 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.

There shouldn't be an IoSession.setFilterChain() method.

>
> 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.

I don't understand. Could you describe the problem a bit more?

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

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.

>
> 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.

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.

/Niklas


Mime
View raw message