directory-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Trustin Lee <trus...@gmail.com>
Subject Re: [mina] Refactoring MINA IoFilterChain (Was: IoFilters: DIRMINA-121 / 122)
Date Tue, 15 Nov 2005 08:44:10 GMT
Hi Niklas,

2005/11/15, Niklas Therning <niklas@trillian.se>:
>
> 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
> > 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)) );
> }


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


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/

Mime
View raw message