[mina] Refactoring MINA IoFilterChain (Was: IoFilters: DIRMINA-121 / 122)

[Trustin Lee]

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/