On Thu, Oct 30, 2008 at 8:28 PM, Emmanuel Lecharny <[EMAIL PROTECTED]>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) > - 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) > - 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) > - 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 > > > (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 ! > > 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. > > 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 ? > I agree with most of these ideas. Probably all of them and those that I may be iffy on are merely a question of how you intend to implement them. This is a great delineation of why we need the chain and what it must do. Thanks for it. I say just go and work on a branch. If working with others like Julien for example this might be better than using a sandbox. Thanks, Alex
