Mailing-List: contact dev-help@mina.apache.org; run by ezmlm
Precedence: bulk
Reply-To: dev@mina.apache.org
Received-SPF: neutral (athena.apache.org: local policy)
Date: Fri, 31 Oct 2008 16:38:58 -0400
From: Jeanfrancois Arcand <jfarcand@apache.org>
Subject: Re: [About the Filter Chain] Proposals
In-reply-to: <490A512D.90704@nextury.com>
Sender: Jeanfrancois.Arcand@Sun.COM
To: dev@mina.apache.org
Message-id: <490B6CE2.8050502@apache.org>
MIME-version: 1.0
Content-type: text/plain; format=flowed; charset=ISO-8859-1
Content-transfer-encoding: 7BIT
References: <490A512D.90704@nextury.com>
User-Agent: Thunderbird 2.0.0.17 (Macintosh/20080914)

Salut,

Emmanuel Lecharny wrote:
> Hi, I'm currently reviewing the Filter chain we are using internally. 
> This is one of the most appealing feature we have, as it allows us to 
> design a versatile handling of incoming and outgoing messages. However, 
> in many respects, the current implementation is pretty complicated, and 
> could benefit from some refactoring.
> 
> I will first try to summarize what we need, and then, from these needed 
> features, I will try to define what should remain, and what could be 
> change and improved.
> 
> 1) What we need
> ---------------
> 
> - Incoming requests must be handled in a pipeline, where filters are 
> executed one after the other, in a predefined order.
> - This order is defined by the protocol designer
> - It can change during the processing, for instance if we need to set up 
> some logging for debug purpose
> - Another case would be that some filter can be added or removed 
> depending on the current message state (a good example could be a 
> multi-layer protocol decoder)
> - This pipeline is a two-way pipeline : it handles incoming requests, 
> and outgoing responses
> - The filter chain is not static, but it should be thread safe, so any 
> kind of modification must *not* lead to some exception (NPE or 
> inconsistent state)

Can you made this behavior configurable? In a project I'm working on :-) 
we made this configurable. e.g. a filters can be thread safe or not. 
When not safe, we internally pool filters. That might sound a weird 
design, but we have seen application that needed to write 
'statefull'/thread unsafe filter. The performance penalty is limited to 
WorkerThread that poll for instance of those filters...which is not that 
significant.


> - Passing to the next filter should be possible in the middle of a 
> filter processing (ie, in a specific filter, you can do some 
> pre-processing, call the next filter, and do some post-processing)

Would it make it "too" complicated for users? What I did in Grizzly was 
to split the task into 2 operations (Filter.execute(), 
Filter.postExecute()). The execute() method return a boolean to telling 
the chain to invoke the next filter or not. Then, in reverse order, we 
invoke the postExecute() on the previously invoked filters. But I might 
be wrong here :-)


> - We should be able to use different pipelines depending on the service 
> (filters can be arranged differently on the same server for 2 different 
> services)

That one sound interesting. I'm curious to find how you will detect 
which pipeline to invoke. You will need some Mapper right?

> - Even for two different sessions, we may have different chain of 
> filters (one session might use a Audit filter while the next is just 
> fine without this filter)
> - We want to decouple the message processing from the socket processor, 
> using a special filter which use an executor to process the message in 
> its own thread

Yes that one will for sure improve performance :-)

> 
> 
> (I wish I didn't forgot anything)
> and, ultimately :
> - It should be easy to debug
> - it should be FAST... :)
> 
> All those features describe the perfect system :). How what we have fits 
> this beautiful picture ?
> 
> 2) Possible improvements
> ------------------------
> The first big point is that we currently have one single chain, when 
> having two could help a lot. The current chain is two-ways, with forward 
> links and backwards link. As we have to handle incoming requests but 
> also write outgoing response, it has to be two-ways. But one single 
> chain is certainly not the best way to handle this. There is no reason 
> why the outgoing chain should be the same than the incoming chain. We 
> may even not use the same filters !
> 
> Proposition (a) : Let's create two chains instead of one : a 
> reader-chain/incoming-chain and a writer-chain/outgoing-chain.
> 
> One other big issue is the way the chain is created and used. Here is, 
> for a very simple chain, the stack trace we obtain (we just have a 
> ProtocolCodecFilter into the chain, nothing else) when an incoming 
> message is being processed, from the bottom to the top (o.a.m stands for 
> org.apache.mina in this trace)
> 
> Thread [NioProcessor-1] (Suspended)    
> o.a.d.agent.ldap.LdapConnectionImpl.messageReceived(o.a.m.core.session.IoSession, 
> java.lang.Object) line: 597   <------ here, we call the handler ...  
> ------------->
>  
> o.a.m.core.filterchain.DefaultIoFilterChain$TailFilter.messageReceived(o.a.m.core.filterchain.IoFilter$NextFilter, 
> o.a.m.core.session.IoSession, java.lang.Object) line: 755    
> o.a.m.core.filterchain.DefaultIoFilterChain.callNextMessageReceived(o.a.m.core.filterchain.IoFilterChain$Entry, 
> o.a.m.core.session.IoSession, java.lang.Object) line: 440    
> o.a.m.core.filterchain.DefaultIoFilterChain.access$5(o.a.m.core.filterchain.DefaultIoFilterChain, 
> o.a.m.core.filterchain.IoFilterChain$Entry, o.a.m.core.session.IoSession,
>    java.lang.Object) line: 435    
> o.a.m.core.filterchain.DefaultIoFilterChain$EntryImpl$1.messageReceived(o.a.m.core.session.IoSession, 
> java.lang.Object) line: 835   <------ here, we call the next filter in 
> the chain, which is the Tail filter ------------->
>  
> o.a.m.filter.codec.ProtocolCodecFilter$ProtocolDecoderOutputImpl.flush() 
> line: 539    
> o.a.m.filter.codec.ProtocolCodecFilter.messageReceived(o.a.m.core.filterchain.IoFilter$NextFilter, 
> o.a.m.core.session.IoSession, java.lang.Object) line: 265   <------ 
> here, we jump to the ProtocolCodec filter ------------->    
> o.a.m.core.filterchain.DefaultIoFilterChain.callNextMessageReceived(o.a.m.core.filterchain.IoFilterChain$Entry, 
> o.a.m.core.session.IoSession, java.lang.Object) line: 440    
> o.a.m.core.filterchain.DefaultIoFilterChain.access$5(o.a.m.core.filterchain.DefaultIoFilterChain, 
> o.a.m.core.filterchain.IoFilterChain$Entry, o.a.m.core.session.IoSession,
>    java.lang.Object) line: 435    
> o.a.m.core.filterchain.DefaultIoFilterChain$EntryImpl$1.messageReceived(o.a.m.core.session.IoSession, 
> java.lang.Object) line: 835    
> o.a.m.core.filterchain.DefaultIoFilterChain$HeadFilter(o.a.m.core.filterchain.IoFilterAdapter).messageReceived(o.a.m.core.filterchain.IoFilter$NextFilter, 
> 
>    o.a.m.core.session.IoSession, java.lang.Object) line: 121   <------ 
> here, we enter into the chain starting on the Head filter 
> ------------->    
> o.a.m.core.filterchain.DefaultIoFilterChain.callNextMessageReceived(o.a.m.core.filterchain.IoFilterChain$Entry, 
> o.a.m.core.session.IoSession, java.lang.Object) line: 440
>  
> o.a.m.core.filterchain.DefaultIoFilterChain.fireMessageReceived(java.lang.Object) 
> line: 432    
> o.a.m.transport.socket.nio.NioProcessor(o.a.m.core.polling.AbstractPollingIoProcessor<T>).read(T) 
> line: 588    
> o.a.m.transport.socket.nio.NioProcessor(o.a.m.core.polling.AbstractPollingIoProcessor<T>).process(T) 
> line: 549    
> o.a.m.transport.socket.nio.NioProcessor(o.a.m.core.polling.AbstractPollingIoProcessor<T>).process() 
> line: 541    
> o.a.m.core.polling.AbstractPollingIoProcessor<T>.access$7(o.a.m.core.polling.AbstractPollingIoProcessor) 
> line: 538   
>  o.a.m.core.polling.AbstractPollingIoProcessor$Processor.run() line: 
> 873    o.a.m.util.NamePreservingRunnable.run() line: 65    
> java.util.concurrent.ThreadPoolExecutor$Worker.runTask(java.lang.Runnable) 
> line: 650    java.util.concurrent.ThreadPoolExecutor$Worker.run() line: 
> 675    java.lang.Thread.run() line: 595  
> As we can see, for a very simple example, we have a 13 line stack trace 
> just to go from the read() method where the incoming bytes are received 
> up to the Ldap handler, traversing 3 filters in the mean time, out of 
> which the Head and Tail filters does not seems to be that necessary...
> 
> We should not have to traverse such a high number of methods in order to 
> process the filters. Not only this is time consuming (as we have to 
> invoke as many methods as we have lines in the stack trace, with all the 
> arguments to pass), but when it comes to debug filters, it's an absolute 
> nightmare. And we have injected ONE single filter in the chain !

Agree :-)

> 
> Proposition (b) : Remove the Head filter. Currently, the Head filter is 
> just used to gather some statistics, and to inject the outgoing message 
> to a blocking queue. Statistics belong to a specific filter, we don't 
> have to compute them for every single incoming message. Writing the 
> outgoing message to a queue is not something we should do in a filter : 
> it's the result of all the chain processing, and the method which 
> invoked the chain has to do this job.
> 
> Proposition (c) : Remove the Tail filter. For the very same reason, this 
> filter is useless. It gather statistics (not it's job...), and them 
> forward to a handler (cf farther about this handler).
> 
> When we have processed the incoming message, we call a ProtocolHandler, 
> which is the terminaison point for this processing. There is no special 
> reason this should be considered differently than a standard filter
> 
> Proposition (d) The Protocolhandler should be a filter, like any other one.


We had the same discussion in Grizzly and we came with the same 
conclusion :-)

> 
> Last, not least, we are using a ChainBuilder to define the filter chain. 
> This structure is a fake FilterChain, which render the manipulation 
> tricky, and leads to confusion. I'm fine with the idea of having a 
> ChainBuilder, but it should be consistent. Another big issue with this 
> class is that it implements many useless methods (like the ones which 
> let you add or remove a filter giving its class, or name, or the filter 
> itself. One single method is enough : we manipulate filters by their 
> name...
> 
> Proposition (e) Simplify the chainBuilder to avoid any risk of 
> confusion, and removing all the useless methods, just keeping those 
> which deal with the filter's name (this is true for the IoFilterChain 
> manipulation itself).
> 
> All those changes should not be complicated to implement, and should 
> also not break the internal logic. It's just a matter of modifying a bit 
> the existing interfaces and the way we process the chain internally. 
>  From the user perspective, it won't change the way filters are added to 
> the chain, and for those who develop new filters, or who have 
> implemented some filters, the modification will be quite minimal.
> 
> PS : All those changes need to be validated, as I may have missed some 
> points. I also suggest that some prototype be written, in a sandbox. 
> This will be the best way to check if those ideas are sane or insane, 
> and also to correctly evaluate the impact on existing code.
> 
> So, wdyt, guys ?

 From an outlier view, that looks promising (and dangerous for the 
outlier project :-))

-- Jeanfrancois


> 
> Thanks !
>