Hi, Comments inline
On Fri, 31 Oct 2008 01:28:29 +0100 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. Agree, specially sentMessage handling, Entry.., and Tail/Head filters. > > 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... :) And memory friendly, cause we can have 1 chain per session. > > 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 ! The decoder could be multi layered the the encoder not. It's a frequent use case I got, so you finish with weird IoFilter. > > 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 What a deep autopsy :) It's showing well the complexity of Entry/Tail/Head > > 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 ! Debugging but reading the internal code too. > > 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. Agree, the Headfilter need to go. > > 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). Need to go too. If we want mandatory stats, wee need to place in "fireMessageReceived" or somewhere else but not in a dummy filter. > > 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. > Agree, and will help to make multi layer protocols, just stack more protocol filters. > 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... I dislike this filter pick&placing API, I just teach how to use MINA to another guy, this API confused him, and he created the chain in reverse order and was unable to understand the bug. > > 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 ? > > Thanks ! > Mostly agree here, I think we need to discuss the implementation in detail, another painful subject is sentMessage(..) reading the IoFilter tutorial, explain all the braindamage it's doing. Perhaps we need to kill this one with this refactoring, elsewhere you will need to do all the "I preserve the initial message for triggering IoHandler.sentMessage(..)" in your new code. Julien
signature.asc
Description: PGP signature
