mina-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From "Alex Karasulu" <akaras...@apache.org>
Subject Re: [About the Filter Chain] Proposals
Date Sat, 01 Nov 2008 20:29:33 GMT
On Thu, Oct 30, 2008 at 8:28 PM, Emmanuel Lecharny <elecharny@gmail.com>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

Mime
  • Unnamed multipart/alternative (inline, None, 0 bytes)
View raw message