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 !
