After letting the dust settle on this thread, I can say that I agree with what Alex is saying. I think a 2-way system makes complete sense. One great interest I have in this is how will it work with containers such as Spring. I have been using MINA with Spring alot lately and want to make sure we keep this support in our system.
--Mark On Sat, Nov 1, 2008 at 4:29 PM, Alex Karasulu <[EMAIL PROTECTED]> wrote: > 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 >
