[mina] Refactoring MINA IoFilterChain (Was: IoFilters: DIRMINA-121 / 122)

[Trustin Lee]

Hi all,

We've talked about refactoring the current IoFilterChain implementation, and I'd like to address some issues:

1) There should be only *one* NextFilter implementation for all filters in the same chain for thread safety, and upstream/downstream (forwarding) operations should be executed in head or tail filter.  It can be overriden by overriding createHeadFilter and createTailFilter.  (and this is the same way Dave suggested to us, but the current implementation hides head and tail from the filter list to keep OOP encapsulation principal :)

2) We need to carefully consider if we have any other use case than having concrete number of filter chain.  I think we can have only three chains per session at maximum.  So I'm afraid this is an overengineering.  Of course we can provide more than one chain in session level, but I think we don't need it actually because we can simply append the chain by adding IoFilterChain.addAllLast() and addAllFirst(), and etc.

3) Should we pass IoHandler as a parameter even if I can get it IoSession.getHandler()?  Please let me know if I am missing something.

4) We cannot simply expose IoFilterChain.setIoProcessor() method because it can break MINA so easily.  So I'd like to suggest hiding setIoProcessor() like this:

public void setFilterChain( IoFilterChain chain ) {
    this.chain = new TransportTypeSpecificFilterChain( chain );  // TransportTypeSpecificFilterChain extends IoFilterChainProxy.
}

public IoFilterChain getFilterChain() {
    return this.chain.getFilterChain(); // Unwrap
}

Only TransportTypeSpecificFilterChain will provide setIoHandler or something like that.  But I think we already have doWrite and doClose in AbstrctIoFilterChain and there are existing subclasses that implement them.  So I don't see much difference here except the behavior of get/setFilterChain().

And please note that per-port and per-session filter chain should be cleared (destroying all filters) when port is unbound or session is closed.  This means they are one-time use only because destroy call on a filter means removal from a chain and vice versa.  So we cannot have IoSession.setFilterChain() because the specified chain will be cleared when the connection is closed so it will prevent from specifying the same chain for more than one sessions.

The biggest problem is that IoFilter.init () and destroy() is invoked together when IoFilter.add(), remove(), and clear() is invoked. (It's because the filter can be added or removed at any time.)  To do so, IoFilter has to remember who its parent is (IoSession or IoSessionManager).  This means we have to specify the correct parent when we construct a chain unfortunately.

So I have to admit that there are so much complexity unfortunately to provide setFilterChain() simply.  Any solutions?

Not let's consider Jose's IoFilterChainBuilder.  It is really simple solution and work without any big changes.  The only problem with the builder pattern is that it can be abused to contain more than just building.  A user can even call System.exit(0) there. :)  But it is entirely up to users eventually.  So I'd like to choose Jose's approach.  I thought his approach adds more complexity to the API, but now I think it is a way simpler and easy to understand.

Cheers,
Trustin
--
what we call human nature is actually human habit
--
http://gleamynode.net/

[Trustin Lee]

2005/11/15, Trustin Lee <trustin@...>:

1) There should be only *one* NextFilter implementation for all filters in the same chain for thread safety, and upstream/downstream (forwarding) operations should be executed in head or tail filter.  It can be overriden by overriding createHeadFilter and createTailFilter.  (and this is the same way Dave suggested to us, but the current implementation hides head and tail from the filter list to keep OOP encapsulation principal :)

I forgot to mention that we can remove createHead/TailFilter and let users to specify them as constructor parameters.

Not let's consider Jose's IoFilterChainBuilder.  It is really simple solution and work without any big changes.  The only problem with the builder pattern is that it can be abused to contain more than just building.  A user can even call System.exit(0) there. :)  But it is entirely up to users eventually.  So I'd like to choose Jose's approach.  I thought his approach adds more complexity to the API, but now I think it is a way simpler and easy to understand.

Not -> Now

Trustin

--
what we call human nature is actually human habit
--
http://gleamynode.net/

[Niklas Therning]

Trustin Lee wrote:

> Hi all,
>
> We've talked about refactoring the current IoFilterChain
> implementation, and I'd like to address some issues:
>
> 1) There should be only *one* NextFilter implementation for all
> filters in the same chain for thread safety, and upstream/downstream
> (forwarding) operations should be executed in head or tail filter.  It
> can be overriden by overriding createHeadFilter and createTailFilter.
> (and this is the same way Dave suggested to us, but the current
> implementation hides head and tail from the filter list to keep OOP
> encapsulation principal :)

If we forget about the IoFilterChain.getNextFilter() methods for the
moment I don't really understand why there has to be one NextFilter.
With the solution I and Dave talked about Entry.nextFilter would go
away. They would have to be created on the fly for each filter
invocation in a chain. I realize that it wouldn't be possible to
implement IoFilterChain.getNextFilter() if Entry.nextFilter wouldn't
exist but are they really needed? If we are going to chain chains
without copying them we will have to propagate the next IoHandler
through the chain at filter time.

>
> 2) We need to carefully consider if we have any other use case than
> having concrete number of filter chain.  I think we can have only
> three chains per session at maximum.  So I'm afraid this is an
> overengineering.  Of course we can provide more than one chain in
> session level, but I think we don't need it actually because we can
> simply append the chain by adding IoFilterChain.addAllLast() and
> addAllFirst(), and etc.

I'm only interested in having at most three chains really. One tied to
the SessionManager. This is where I would put the ThreadPoolFilter. One
tied to the port. This is where I would put the SSLFilter. And one
private to the IoSession. There should not be an
IoSession.setFilterChain() method.

>
> 3) Should we pass IoHandler as a parameter even if I can get it
> IoSession.getHandler()?  Please let me know if I am missing something.

The SessionManager (or SocketIoProcessor) will simply call

this.filterChain.sessionOpened( session, session.getHandler() );

If this.filterChain is Dave's ChainedFilterChain
this.filterChain.sessionOpened() could look like:

public void sessionOpened( session, handler ) {
    this.firstChain.sessionOpened( session, new
IoFilterToIoHandlerAdapter(handler, this.secondFilter)) );
}

>
> 4) We cannot simply expose IoFilterChain.setIoProcessor() method
> because it can break MINA so easily.  So I'd like to suggest hiding
> setIoProcessor() like this:
>
> public void setFilterChain( IoFilterChain chain ) {
>     this.chain = new TransportTypeSpecificFilterChain( chain );  //
> TransportTypeSpecificFilterChain extends IoFilterChainProxy.
> }
>
> public IoFilterChain getFilterChain() {
>     return this.chain.getFilterChain(); // Unwrap
> }
>
> Only TransportTypeSpecificFilterChain will provide setIoHandler or
> something like that.  But I think we already have doWrite and doClose
> in AbstrctIoFilterChain and there are existing subclasses that
> implement them.  So I don't see much difference here except the
> behavior of get/setFilterChain().

Yes, setIoProcessor() isn't very nice, I agree. And I don't have a
solution for it except that copying the chain passed to setFilterChain()
into something like an IoProcessorAwareFilterChain. I can't see how the
above solves this. How does the wrapped chain know where to route
write() / close() calls? Or is it just to prevent setIoProcessor() from
being called?

>
> And please note that per-port and per-session filter chain should be
> cleared (destroying all filters) when port is unbound or session is
> closed.  This means they are one-time use only because destroy call on
> a filter means removal from a chain and vice versa.  So we cannot have
> IoSession.setFilterChain() because the specified chain will be cleared
> when the connection is closed so it will prevent from specifying the
> same chain for more than one sessions.

There shouldn't be an IoSession.setFilterChain() method.

>
> The biggest problem is that IoFilter.init () and destroy() is invoked
> together when IoFilter.add(), remove(), and clear() is invoked. (It's
> because the filter can be added or removed at any time.)  To do so,
> IoFilter has to remember who its parent is (IoSession or
> IoSessionManager).  This means we have to specify the correct parent
> when we construct a chain unfortunately.

I don't understand. Could you describe the problem a bit more?

>
> So I have to admit that there are so much complexity unfortunately to
> provide setFilterChain() simply.  Any solutions?

Yes it's complex. But I wouldn't say that the current implementation is
much simpler. It took quite some time and debugging to figure out how
the filter chains work. (Maybe that's just me being really stupid ;) )
What I like about this approach (except for setIoProcessor() which isn't
really acceptable) is that the configuration of chains are totally
detached and independent from the SessionManager.

>
> Not let's consider Jose's IoFilterChainBuilder.  It is really simple
> solution and work without any big changes.  The only problem with the
> builder pattern is that it can be abused to contain more than just
> building.  A user can even call System.exit(0) there. :)  But it is
> entirely up to users eventually.  So I'd like to choose Jose's
> approach.  I thought his approach adds more complexity to the API, but
> now I think it is a way simpler and easy to understand.

Maybe I have totally misunderstood what Jose meant but I cant see how
that would simplify anything. You would still have the problem of wiring
the three chains together. Or am I missing something fundamental? As I
said previously, the wiring of chains is problem number 1, how to
configure per-port chains is problem number 2.

/Niklas

[Niklas Therning]

Niklas Therning wrote:

> ...
>
>
>Yes, setIoProcessor() isn't very nice, I agree. And I don't have a
>solution for it except that copying the chain passed to setFilterChain()
>into something like an IoProcessorAwareFilterChain. I can't see how the
>above solves this. How does the wrapped chain know where to route
>write() / close() calls? Or is it just to prevent setIoProcessor() from
>being called?
>
>  
>

I have a solution now! :)

I just realized that when calling IoSession.write() the concrete
IoSession implementation will always know the final destination of the
write event. For SocketSessionImpl the final destination is always
SocketIoProcessor.

So the IoFilterChain methods for downstream events look like this:

void write( IoSession session, WriteRequest writeRequest, IoProcessor p
) throws Exception;

SocketSessionImpl's write() method looks like this:

public WriteFuture write( Object message ) {
    this.filterChain.write(this, new WriteRequest(),
SocketIoProcesso.getInstance());
}

Provided that SocketIoProcessor implements IoProcessor. I think you get
the point. The ChainedFilterChain will always know the next filter chain
to filter through and can wrap it in an
IoFilterChainToIoProcessorAdapter object.

Bottom line: setIoProcessor() WON'T BE NEEDED! This approach makes
IoFilterChain totally stateless which means the same filter chain can be
reused for different ports if the user wants that (though that would
probably not be advisable).

The only sacrifice I see right now is the IoFilterChain.getNextFilter()
methods. They would not return a proper NextFilter instance.

If it's ok with Dave I would like to see what he comes up with before we
abondon this approach. Dave, if you don't want to spend your time on
something that might get thrown away, please let me know and I can take
over.

/Niklas

[Trustin Lee]

Hi Niklas,

2005/11/15, Niklas Therning <niklas@...>:

If we forget about the IoFilterChain.getNextFilter() methods for the
moment I don't really understand why there has to be one NextFilter.

It is because NextFilter can be cached by a filter in case it have to emit the event even if any I/O event or request didn't occur.  For example, there can be a customized dynamic timeout filter.

With the solution I and Dave talked about Entry.nextFilter would go
away. They would have to be created on the fly for each filter
invocation in a chain. I realize that it wouldn't be possible to
implement IoFilterChain.getNextFilter() if Entry.nextFilter wouldn't
exist but are they really needed? If we are going to chain chains
without copying them we will have to propagate the next IoHandler
through the chain at filter time.

You're right.  getNextFilter() is not actually required to be exposed, but it is still true that a filter should be able to emit any event at any time using nextFilter instance it cached.

I'm only interested in having at most three chains really. One tied to
the SessionManager. This is where I would put the ThreadPoolFilter. One
tied to the port. This is where I would put the SSLFilter. And one
private to the IoSession. There should not be an
IoSession.setFilterChain() method.

I see. :)

> 3) Should we pass IoHandler as a parameter even if I can get it
> IoSession.getHandler()?  Please let me know if I am missing something.

The SessionManager (or SocketIoProcessor) will simply call

this.filterChain.sessionOpened( session, session.getHandler() );

If this.filterChain is Dave's ChainedFilterChain
this.filterChain.sessionOpened() could look like:

public void sessionOpened( session, handler ) {
    this.firstChain.sessionOpened( session, new
IoFilterToIoHandlerAdapter(handler, this.secondFilter)) );
}

I didn't get it yet.  Can't we simply override head/tail filter for each chain?

Yes, setIoProcessor() isn't very nice, I agree. And I don't have a
solution for it except that copying the chain passed to setFilterChain()
into something like an IoProcessorAwareFilterChain. I can't see how the
above solves this. How does the wrapped chain know where to route
write() / close() calls? Or is it just to prevent setIoProcessor() from
being called?

Yes that's also a problem... so we have to copy the chain.  (Sorry for the confusion ;) But copying the chain rises another problem; init() and destroy() is called immediately when a filter is added or removed.

> The biggest problem is that IoFilter.init () and destroy() is invoked
> together when IoFilter.add(), remove(), and clear() is invoked. (It's
> because the filter can be added or removed at any time.)  To do so,
> IoFilter has to remember who its parent is (IoSession or
> IoSessionManager).  This means we have to specify the correct parent
> when we construct a chain unfortunately.

I don't understand. Could you describe the problem a bit more?

As you know, there's no init() or destroy() in IoFilterChain.  This means there's no life cycle in IoFilterChain.  The life cycle only exists in filter-level.  So when you add a filter to a chain, IoFilter.init() is called imediately to initialize the filter.  And IoFilter.destroy() is invoked when the filter is removed from the chain.  I didn't put init() or destroy() to IoFilterChain because filters can be added and removed whenever a user wants.  User could even add filters after init() is invoked, and then we should call IoFilter.init() immediately, so there's no much reason to provide init() and destroy() in IoFilterChain.

So we know we have to call init() and destroy() when a filter is added.  The init() and destroy() method requires an IoFilterChain parameter as a parent.  (I made a mistake in my previous post.  It requires IoFilterChain as a parent)  It is because one filter instance can be added to more than one chains.

Now let's talk about cloning the chain.  Take a look at this code:

IoFilterChain newFilterChain = new ...;
IoFilter filterA = new FilterA();
newFilterChain.addLast( "a", filterA);

acceptor.setFilterChain ( newFilterChain ); // internally newFilterChain is cloned.

This code will call filterA.init() twice with different parent (IoFilterChain).  This means user have to call newFilterChain.clear() explicitly.  Of course user will have to call acceptor.getFilterChain().clear() when it shuts down, but two calls and one call are different IMHO.

Yes it's complex. But I wouldn't say that the current implementation is
much simpler. It took quite some time and debugging to figure out how
the filter chains work. (Maybe that's just me being really stupid ;) )
What I like about this approach (except for setIoProcessor() which isn't
really acceptable) is that the configuration of chains are totally
detached and independent from the SessionManager.

Right, I love that advantage you mentioned.  To work around the init() and destroy() issue, the IoFilterChain implementation that user can construct and that doesn't invoke init() and destroy() at all.  But it's too tricky.  What about just introducing IoFilterConfig with minimal interface?

public interface IoFilterConfig {  // perhaps we can implement is as a concrete class simply.
    public String getName();
    public IoFilter getFilter();
}

and accepting a List of IoFilterConfigs?

If so, the implemtation of IoAcceptor.setFilterChain() will be...

public void setFilterChain(List filterConfigs) throws Exception {
    this.chain.clear();
    for( Iterator i = filterConfigs.iterator (); i.hasNext(); ) {
        Object o = i.next();
        if( o instanceof IoFilterConfig ) {
            IoFilterConfig cfg = ( IoFilterConfig ) o;
            this.chain.addLast( cfg.getName(), cfg.getFilter () );
        } else if( o instanceof IoFilter ) {
            IoFilter filter = ( IoFilter ) o;
            this.chain.addLast( someAnonymousNameBasedOnFilterType, cfg.getFilter() );
        }
    }
}

WDYT?

Maybe I have totally misunderstood what Jose meant but I cant see how
that would simplify anything. You would still have the problem of wiring
the three chains together. Or am I missing something fundamental? As I
said previously, the wiring of chains is problem number 1, how to
configure per-port chains is problem number 2.

For problem #1, yes, we still have to resolve that problem.  But Jose's approach solves Problem #2.  We can pass port-level filter chain to the builder.

Cheers,
Trustin

--
what we call human nature is actually human habit
--
http://gleamynode.net/

[Trustin Lee]

2005/11/15, Niklas Therning <niklas@...>:

I just realized that when calling IoSession.write() the concrete
IoSession implementation will always know the final destination of the
write event. For SocketSessionImpl the final destination is always
SocketIoProcessor.

So the IoFilterChain methods for downstream events look like this:

void write( IoSession session, WriteRequest writeRequest, IoProcessor p
) throws Exception;

SocketSessionImpl's write() method looks like this:

public WriteFuture write( Object message ) {
    this.filterChain.write(this, new WriteRequest(),
SocketIoProcesso.getInstance());
}

What is the difference from overriding the head filter of sessionmanager-level chain to call SocketIoProcessor.flush() explicitly?  We can still get rid of SocketSessionManagerFilterChain without introducing a new parameter.

I'll check in this change so you can see it more clearly.

Cheers,
Trustin
--
what we call human nature is actually human habit
--
http://gleamynode.net/

[Irving, Dave]

I too agree that setIoProcessor isn't all that nice - and what I was
trying to think about last night was ways to get away from it.
Im not so tired this morning (although 4 hours sleep didn't help
much...) but I'll try to explain my approach a bit more clearly.
It kind of boils down to a few points:

1) We want to chain "sequences" of filters (a filter chain) together
2) Soon we want to be able to let users build their own filters
3) We want to preserve OOP

To me, the current filter chain / filter implementation looks quite
complicated. I haven't got familiar enough with the code yet to know
whether ths complexity is an unfortunate necessity.
Just so we know what my proposed approach is, I'll try and describe the
basics here:

1) I don't see how refactoring the chains affects Jose's approach. If we
have individual chains, we can still pass them to a builder to be
populated
2) Why not make IoFilterChain move towards the composite pattern? A
filter chain is just a special type of filter which filters in a
sequence. No special head, no special tail. Just a sequence 0..n. So
given some "BasicFilterChain" impl, we can add both individual filters
and filter chains to the chain. NextFilters can ** still ** be cached by
individual filters - no change there.
3) I think filters can still be used ** without ** cloning and without
special "setIoProcessor" methods. Asume that we have (2). When a new
connection is established, all we have to do is "hook up" our sub
chains:

BasicFilterChain connectionChain = new BasicFilterChain();
connectionChain.addLast(sessionManagerChain);
connectionChain.addLast(portChain);
BasicFilterChain sessionChain = new BasicFilterChain();
someChainBuilder.buildChain(sessionChain); // Jose's approach
connectionChain.addLast(sessionChain);

connectionChain.add(endOfTheLineFilter);

And that's it - connectionChain becomes the uniqueue chain for a
connection. The last addition, the "endOfTheLineFilter" - is just a
filter that does the "real" work (like Niklas' IoProcessor approach -
but now encapsulated in a filter). Because this is per-connection, the
"endOfTheLine" filter can be provided with the collaborators it needs to
do its job without affecting any other filter.
This doesn't require any "cloning" of chains, or creation of NextFilters
per traversal either.

This to me seems like a much cleaner approach - but I must be missing
something?

Dave


This e-mail and any attachment is for authorised use by the intended recipient(s) only. It may contain proprietary material, confidential information and/or be subject to legal privilege. It should not be copied, disclosed to, retained or used by, any other party. If you are not an intended recipient then please promptly delete this e-mail and any attachment and all copies and inform the sender. Thank you.

[Niklas Therning]

Trustin Lee wrote:

> 2005/11/15, Niklas Therning <niklas@...
> <mailto:niklas@...>>:
>
>     I just realized that when calling IoSession.write() the concrete
>     IoSession implementation will always know the final destination of the
>     write event. For SocketSessionImpl the final destination is always
>     SocketIoProcessor.
>
>     So the IoFilterChain methods for downstream events look like this:
>
>     void write( IoSession session, WriteRequest writeRequest,
>     IoProcessor p
>     ) throws Exception;
>
>     SocketSessionImpl's write() method looks like this:
>
>     public WriteFuture write( Object message ) {
>         this.filterChain.write(this, new WriteRequest(),
>     SocketIoProcesso.getInstance());
>     }
>
>
> What is the difference from overriding the head filter of
> sessionmanager-level chain to call SocketIoProcessor.flush()
> explicitly?  We can still get rid of SocketSessionManagerFilterChain
> without introducing a new parameter.
>

If the user can change the filter chain of an IoAcceptor by calling
setFilterChain() you won't be able to override any method to inject this
behaviour. Likewise if there is a bind(address, chain, handler) method.
Unless you copy the user supplied chain into some IoFilterChain
implementation which we have control over. And from what I understand
copying is out of the question. Or am I missing something?

> I'll check in this change so you can see it more clearly.

Please do that. Maybe that will make me understand! :)

/Niklas

[Trustin Lee]

Hi Dave,

2005/11/15, Irving, Dave <dave.irving@...>:

To me, the current filter chain / filter implementation looks quite
complicated. I haven't got familiar enough with the code yet to know
whether ths complexity is an unfortunate necessity.

Yes, it is really complex. :)

Just so we know what my proposed approach is, I'll try and describe the
basics here:

1) I don't see how refactoring the chains affects Jose's approach. If we
have individual chains, we can still pass them to a builder to be
populated

Right there's no much relationship.

2) Why not make IoFilterChain move towards the composite pattern? A
filter chain is just a special type of filter which filters in a
sequence. No special head, no special tail. Just a sequence 0..n. So
given some "BasicFilterChain" impl, we can add both individual filters
and filter chains to the chain. NextFilters can ** still ** be cached by
individual filters - no change there.
3) I think filters can still be used ** without ** cloning and without
special "setIoProcessor" methods. Asume that we have (2). When a new
connection is established, all we have to do is "hook up" our sub
chains:

BasicFilterChain connectionChain = new BasicFilterChain();
connectionChain.addLast(sessionManagerChain);
connectionChain.addLast(portChain);
BasicFilterChain sessionChain = new BasicFilterChain();
someChainBuilder.buildChain(sessionChain); // Jose's approach
connectionChain.addLast(sessionChain);

connectionChain.add(endOfTheLineFilter);

Explicitly asking users to add head/tail filter is a bad idea.  But I think it is a great idea.  But we don't need to make a chain be nested more than one level.  And head and tail filter must be hidden from user to prevent from unexpected removal or reordering.

Cheers,
Trustin
--
what we call human nature is actually human habit
--
http://gleamynode.net/

[Trustin Lee]

Hi,

2005/11/15, Niklas Therning <niklas@...>:

Trustin Lee wrote:

> 2005/11/15, Niklas Therning <niklas@...
> <mailto:niklas@...>>:
>
>     I just realized that when calling IoSession.write() the concrete
>     IoSession implementation will always know the final destination of the
>     write event. For SocketSessionImpl the final destination is always
>     SocketIoProcessor.
>
>     So the IoFilterChain methods for downstream events look like this:
>
>     void write( IoSession session, WriteRequest writeRequest,
>     IoProcessor p
>     ) throws Exception;
>
>     SocketSessionImpl's write() method looks like this:
>
>     public WriteFuture write( Object message ) {
>         this.filterChain.write(this, new WriteRequest(),
>     SocketIoProcesso.getInstance ());
>     }
>
>
> What is the difference from overriding the head filter of
> sessionmanager-level chain to call SocketIoProcessor.flush()
> explicitly?  We can still get rid of SocketSessionManagerFilterChain
> without introducing a new parameter.
>

If the user can change the filter chain of an IoAcceptor by calling
setFilterChain() you won't be able to override any method to inject this
behaviour. Likewise if there is a bind(address, chain, handler) method.
Unless you copy the user supplied chain into some IoFilterChain
implementation which we have control over. And from what I understand
copying is out of the question. Or am I missing something?

If we're going to use setfilterChain only for the initial configuration, then we can copy.  Otherwise we cannot. The problem here is that we cannot easily reuse IoFilterChain because it contains too much information.

Trustin

--
what we call human nature is actually human habit
--
http://gleamynode.net/

[daune.jf]

Hi,

don't be afraid, I am not going to propose a third possibility to this
topic ;->

I just wanted you to know what we, basic users, do with filters. I think the
more use cases you have, the better.

We developed a server supporting two socket-based protocols (legacy one
and new
one) on two separate ports. For specific reasons, we do not want than 400
simultaneous connections on the server. For this, both services need to
share a
service-level filter counting sessions.

For the legacy protocol, some clients (with a specific SW release)
forget an ACK
message. This can be detected in the server. For this, we need a service-level
filter that calls nextFilter.messageReceived with the missing message whereas
it has not really been received.

My 2 cts,

J-F

[Irving, Dave]

>  Explicitly asking users to add head/tail filter is a bad idea.   

 

I agree. In fact - thats my point :o)

 

>  But I think it is a great idea.  But we don't need to make a chain be nested more than one level.   

>  And head and tail filter must be hidden from user to prevent from unexpected removal or reordering. 
 

Exactly. The ** user ** doesn't write the code above: This is in the acceptor (or whatever).

The idea is that the user can contribute a chain (Jose's approach) how ever they like.

The special "head tail" behaviour is encapsulated to the client who requires it: I.e, the acceptor / connector.

 

This is what I like about the idea. Currently, if you write a filter chain, you need to care about that special head tail behaviour. Using this approach you wouldn't - because a chain is nothing more than a sequence of filters.

Its the acceptor / connector who cares about the head / tail behaviour - so he can specify it directly.

Hopefully this example will clear up the creational logic:

 

Imagine this code is in SocketAcceptorDelegate.Worker#processSessions - when we've just found out we've got a new session:

 

// Create the filter chain for the new connection / session

BasicFilterChain connectionChain = new BasicFilterChain();   

// Add the session manager chain to the connection chain

connectionChain.addLast(sessionManagerChain); 

// Add the per-port chain to the connection chain 

connectionChain.addLast(portChain); 

// Let the user contribute to a new per-session chain (Jose's approach)

BasicFilterChain sessionChain = new BasicFilterChain();  

someChainBuilder.buildChain(sessionChain);

// Add the per-session filter to the connection chain

connectionChain.addLast(sessionChain); 

 

// Now create the filter which does ** our ** work (i.e, real handler writing)

IoFilter workerFilter = new EndOfTheLineFilter(); 

connectionChain.add( workerFilter );

 

Notice that the user isn't specifying the worker filter - the SocketAcceptorDelegate is.

So I belive this gives us real benefits:

 

1) Makes the filter chain mechanism simpler (reduces complexity as we're now using composition rather than inheritance)

2) Paves the way for user contribution to chains (Jose's approach)

3) Doesn't require filter cloning or per-traversal clones

4) Is OO: The acceptor / connector is the actor who wants to specify "end of chain" behaviour: So he does so by means of adding a custom filter to the end of the built chain

 

Does this clear it up at all?

What do you think about this approach?

 

Many thanks,

 

Dave (give me sleep) Irving

 

This e-mail and any attachment is for authorised use by the intended recipient(s) only. It may contain proprietary material, confidential information and/or be subject to legal privilege. It should not be copied, disclosed to, retained or used by, any other party. If you are not an intended recipient then please promptly delete this e-mail and any attachment and all copies and inform the sender. Thank you.

[Irving, Dave]

> Its the acceptor / connector who cares about the head / tail behaviour - so he can specify it directly

 

Just to clarify: With this approach there is no explicit head / tail. The SocketAcceptorDelegate et al just add their work as the last filter in the per-connection chain.

The user is never exposed to the whole per connection chain (they only ever see the per session chain) - so it is impossible for them to tinker with it

 

Dave

---

From: Irving, Dave [mailto:dave.irving@...]
Sent: 15 November 2005 09:41
To: Apache Directory Developers List
Subject: RE: [mina] Refactoring MINA IoFilterChain (Was: IoFilters: DIRMINA-121 / 122)

>  Explicitly asking users to add head/tail filter is a bad idea.   

 

I agree. In fact - thats my point :o)

 

>  But I think it is a great idea.  But we don't need to make a chain be nested more than one level.   

>  And head and tail filter must be hidden from user to prevent from unexpected removal or reordering. 
 

Exactly. The ** user ** doesn't write the code above: This is in the acceptor (or whatever).

The idea is that the user can contribute a chain (Jose's approach) how ever they like.

The special "head tail" behaviour is encapsulated to the client who requires it: I.e, the acceptor / connector.

 

This is what I like about the idea. Currently, if you write a filter chain, you need to care about that special head tail behaviour. Using this approach you wouldn't - because a chain is nothing more than a sequence of filters.

Its the acceptor / connector who cares about the head / tail behaviour - so he can specify it directly.

Hopefully this example will clear up the creational logic:

 

Imagine this code is in SocketAcceptorDelegate.Worker#processSessions - when we've just found out we've got a new session:

 

// Create the filter chain for the new connection / session

BasicFilterChain connectionChain = new BasicFilterChain();   

// Add the session manager chain to the connection chain

connectionChain.addLast(sessionManagerChain); 

// Add the per-port chain to the connection chain 

connectionChain.addLast(portChain); 

// Let the user contribute to a new per-session chain (Jose's approach)

BasicFilterChain sessionChain = new BasicFilterChain();  

someChainBuilder.buildChain(sessionChain);

// Add the per-session filter to the connection chain

connectionChain.addLast(sessionChain); 

 

// Now create the filter which does ** our ** work (i.e, real handler writing)

IoFilter workerFilter = new EndOfTheLineFilter(); 

connectionChain.add( workerFilter );

 

Notice that the user isn't specifying the worker filter - the SocketAcceptorDelegate is.

So I belive this gives us real benefits:

 

1) Makes the filter chain mechanism simpler (reduces complexity as we're now using composition rather than inheritance)

2) Paves the way for user contribution to chains (Jose's approach)

3) Doesn't require filter cloning or per-traversal clones

4) Is OO: The acceptor / connector is the actor who wants to specify "end of chain" behaviour: So he does so by means of adding a custom filter to the end of the built chain

 

Does this clear it up at all?

What do you think about this approach?

 

Many thanks,

 

Dave (give me sleep) Irving

 

This e-mail and any attachment is for authorised use by the intended recipient(s) only. It may contain proprietary material, confidential information and/or be subject to legal privilege. It should not be copied, disclosed to, retained or used by, any other party. If you are not an intended recipient then please promptly delete this e-mail and any attachment and all copies and inform the sender. Thank you.

[Niklas Therning]

I really like your approach, Dave. Some question though:

What would session.getFilterChain() return? It will be sessionChain, right?

Will IoFilterChain extend IoFilter and be just another IoFilter?

This is basically the same as nesting filter chains as I proposed
yesterday (remember the (((A, B), C), D) thingy?) but it's a flat list
of chains instead, right?

You still have the NextFilter problem. As I understand it these have to
be static since IoFilter.init() is called when the filter is added to a
chain. A NextFilter is passed to this method and the filter could
remember this for future use.

I can't see how the last filter in a wrapped chain will know where to go
next if the NextFilter given to it can't be created dynamically.
Remember, sessionManagerChain and portChain will be shared by many
connectionChains.

/Niklas

Irving, Dave wrote:

>
>>  Explicitly asking users to add head/tail filter is a bad idea.  
>  
> I agree. In fact - thats my point :o)
>  
>>  But I think it is a great idea.  But we don't need to make a chain
> be nested more than one level.  
>>  And head and tail filter must be hidden from user to prevent from
> unexpected removal or reordering.
>  
> Exactly. The ** user ** doesn't write the code above: This is in the
> acceptor (or whatever).
> The idea is that the user can contribute a chain (Jose's approach) how
> ever they like.
> The special "head tail" behaviour is encapsulated to the client who
> requires it: I.e, the acceptor / connector.
>  
> This is what I like about the idea. Currently, if you write a filter
> chain, you need to care about that special head tail behaviour. Using
> this approach you wouldn't - because a chain is nothing more than a
> sequence of filters.
> Its the /acceptor / connector /who cares about the head / tail
> behaviour - so he can specify it directly.
> Hopefully this example will clear up the creational logic:
>  
> Imagine this code is in SocketAcceptorDelegate.Worker#processSessions
> - when we've just found out we've got a new session:
>  
> // Create the filter chain for the new connection / session
> BasicFilterChain connectionChain = new BasicFilterChain();  
> // Add the session manager chain to the connection chain
> connectionChain.addLast(sessionManagerChain);
> // Add the per-port chain to the connection chain
> connectionChain.addLast(portChain);
> // Let the user contribute to a new per-session chain (Jose's approach)
> BasicFilterChain sessionChain = new BasicFilterChain();  
> someChainBuilder.buildChain(sessionChain);
> // Add the per-session filter to the connection chain
> connectionChain.addLast(sessionChain);
>  
> // Now create the filter which does ** our ** work (i.e, real handler
> writing)
> IoFilter workerFilter = new EndOfTheLineFilter();
> connectionChain.add( workerFilter );
>  
> Notice that the /user /isn't specifying the worker filter - the
> SocketAcceptorDelegate is.
> So I belive this gives us real benefits:
>  
> 1) Makes the filter chain mechanism simpler (reduces complexity as
> we're now using composition rather than inheritance)
> 2) Paves the way for user contribution to chains (Jose's approach)
> 3) Doesn't require filter cloning or per-traversal clones
> 4) Is OO: The acceptor / connector is the actor who wants to specify
> "end of chain" behaviour: So he does so by means of adding a custom
> filter to the end of the built chain
>  
> Does this clear it up at all?
> What do you think about this approach?
>  
> Many thanks,
>  
> Dave (give me sleep) Irving
>  
>
>
> This e-mail and any attachment is for authorised use by the intended
> recipient(s) only. It may contain proprietary material, confidential
> information and/or be subject to legal privilege. It should not be
> copied, disclosed to, retained or used by, any other party. If you are
> not an intended recipient then please promptly delete this e-mail and
> any attachment and all copies and inform the sender. Thank you.
>

[Irving, Dave]

Niklas Therning wrote:

>I really like your approach, Dave. Some question though:
> What would session.getFilterChain() return? It will be sessionChain,
right?

Yes. This allows IoHandlers or who-ever to contribute to the session
chain.
They ** never ** get access to the "connection chain" (which is a
private implementation detail of SocketAcceptorDelegate etc).

> Will IoFilterChain extend IoFilter and be just another IoFilter?

Depends. I think at the moment there's not all that much need for true
composite - as Trustin says.
However, I don't think its any more or less work either way - and I
think its cleaner to have a chain as a special type of filter.
However, this ** could ** be blown away by the NextFilter problem.

> This is basically the same as nesting filter chains as I proposed
yesterday (remember the (((A, B), C), D) thingy?)
> but it's a flat list of chains instead, right?

Yeah - I think we were both talking about the same thing all along but
didn't realise it in our respective tiredness :o)

> You still have the NextFilter problem.
> As I understand it these have to be static since IoFilter.init() is
called when the filter is added to a chain.
> A NextFilter is passed to this method and the filter could remember
this for future use.

Yes - I certainly do have a NextFilter problem. This is quite similar to
the "this problem" often found when employing the decorator pattern.
Im going to spend some time this morning thinking about how we can get
round it cleanly and transparently: But I understand that support for
NextFilter caching is required, important, and must not be lost.

> I can't see how the last filter in a wrapped chain will know where to
go next if the NextFilter given
> to it can't be created dynamically.
> Remember, sessionManagerChain and portChain will be shared by many
connectionChains.

Yep - Im going to have to think about that this morning / this
afternoon.
However, I believe that finding a solution is worth it - as Im sure
we're on to a clean solution :o)


> /Niklas

Dave


This e-mail and any attachment is for authorised use by the intended recipient(s) only. It may contain proprietary material, confidential information and/or be subject to legal privilege. It should not be copied, disclosed to, retained or used by, any other party. If you are not an intended recipient then please promptly delete this e-mail and any attachment and all copies and inform the sender. Thank you.

[Niklas Therning]

Irving, Dave wrote:

>
>
>Yep - Im going to have to think about that this morning / this
>afternoon.
>However, I believe that finding a solution is worth it - as Im sure
>we're on to a clean solution :o)
>
>  
>
So do I! Keep us posted.

/Niklas

[Irving, Dave]

> So do I! Keep us posted.
(The NextFilter problem)

Ok, I've thought about it, and I can only really think of one solution.
The problem is that it sounds like we have to support completely
asyncronous invocations on NextFilter.
The issue there is that without cloning filters, a given port filter
logically applies to ALL chains it belongs to...

Except that we should remember that a full connection chain is always
associated with a connection (IoSession). And there would always be a
single connection chain per connection.

So why don't we just dispatch based on IoSession??

I.e, we have something like a "ConnectionChains" class which holds all
chains - mapped against IoSessions.
The final filter of a sub-chain (sessionManager chain, port chain,
session chain) is set by ConnectionChains to some private inner class.
When the last filter of a sub-chain is invoked, ConnectionChains does a
( O(1) ) look up for the correct ConnectionChain based on session and
delegates.

This allows us to get all the benefits previously discussed and solves
the NextFilter problem (including completely asyncronous calls to
NextFilter) without requiring cloning (which could be costly for some
filters).

How does that sound?


> /Niklas

Dave


This e-mail and any attachment is for authorised use by the intended recipient(s) only. It may contain proprietary material, confidential information and/or be subject to legal privilege. It should not be copied, disclosed to, retained or used by, any other party. If you are not an intended recipient then please promptly delete this e-mail and any attachment and all copies and inform the sender. Thank you.

[Trustin Lee]

Hi all,

2005/11/15, Niklas Therning <niklas@...>:

> I'll check in this change so you can see it more clearly.

Please do that. Maybe that will make me understand! :)

The change was not really trivial, so I couldn't make it in short time.  You guys look really eager to refactor my overcomplex code, please go ahead, and let's find out its problem. ;)

Cheers,
Trustin

--
what we call human nature is actually human habit
--
http://gleamynode.net/

[Trustin Lee]

Hi Dave,

I started to like your suggestion, and I'd like to see its realization. :)

2005/11/15, Irving, Dave <dave.irving@...>:

So why don't we just dispatch based on IoSession??

I.e, we have something like a "ConnectionChains" class which holds all
chains - mapped against IoSessions.
The final filter of a sub-chain (sessionManager chain, port chain,
session chain) is set by ConnectionChains to some private inner class.
When the last filter of a sub-chain is invoked, ConnectionChains does a
( O(1) ) look up for the correct ConnectionChain based on session and
delegates.

Good idea.  Perhaps we can use session attributes here?

This allows us to get all the benefits previously discussed and solves
the NextFilter problem (including completely asyncronous calls to
NextFilter) without requiring cloning (which could be costly for some
filters).

How does that sound?

Sounds cool.

BTW J-F once asked us about emitting an event virtually (not a real I/O event but fabricated one).  I guess we can support this in our ConnectionChain by exposing event emitting methods like I did in AbstractIoFilterChain?

Cheers,
Trustin

--
what we call human nature is actually human habit
--
http://gleamynode.net/

[Irving, Dave]

Hi Trustin,

 

Im really sorry if anything I said came accross like I was critisising the current implementation. I certainly didn't mean it do :o)

I was simply searching for a clean way to enable 121 and 122 to get solved in a clean way - and it seemed that refactoring the existing chain mechanics might be a reasonable way to do that.

Thanks for your comments - I'll try and address them here:

 

> I started to like your suggestion, and I'd like to see its realization. :)

 

I'll start working on it in earnest :o) Hopefully I'll be able to report back with a patch in the next few days.

 

>> When the last filter of a sub-chain is invoked, ConnectionChains does a
>> ( O(1) ) look up for the correct ConnectionChain based on session and
>> delegates.

 

> Good idea.  Perhaps we can use session attributes here?

 

Session attributes are definately the best way to go here. It means we dont need to hold a mapping within ConnectionChains - and we dont need additional syncronization. I hadn't yet thought about how I'd implement the mapping efficiently - so this suggestion is really very very useful to me. Thanks!!!

 

> BTW J-F once asked us about emitting an event virtually (not a real I/O event but fabricated one). 

> I guess we can support this in our ConnectionChain by exposing event emitting methods like I did in AbstractIoFilterChain?

Sure!

 

Have an IoEventProcessor interface or some-such. The ConnectionChains (or whatever) can implement this ( - to kick off traversal like AbstractIoFilterChain does).

This can then be exposed to the user (without having to expose the connection chain - which would be dangerous for obvious reasons).

 

E.g:

  IoEventProcessor processor = somthing.getEventProcessor();

  processor.messageReceived(session, message);

 

Viola - virtual Io events propogating right through the correct connection chain :o)

 

Dave

 

---

From: Trustin Lee [mailto:trustin@...]
Sent: 15 November 2005 12:18
To: Apache Directory Developers List
Subject: Re: [mina] Refactoring MINA IoFilterChain (Was: IoFilters: DIRMINA-121 / 122)

Hi Dave,

I started to like your suggestion, and I'd like to see its realization. :)

2005/11/15, Irving, Dave <dave.irving@...>:

So why don't we just dispatch based on IoSession??

I.e, we have something like a "ConnectionChains" class which holds all
chains - mapped against IoSessions.
The final filter of a sub-chain (sessionManager chain, port chain,
session chain) is set by ConnectionChains to some private inner class.
When the last filter of a sub-chain is invoked, ConnectionChains does a
( O(1) ) look up for the correct ConnectionChain based on session and
delegates.

Good idea.  Perhaps we can use session attributes here?

This allows us to get all the benefits previously discussed and solves
the NextFilter problem (including completely asyncronous calls to
NextFilter) without requiring cloning (which could be costly for some
filters).

How does that sound?

Sounds cool.

BTW J-F once asked us about emitting an event virtually (not a real I/O event but fabricated one).  I guess we can support this in our ConnectionChain by exposing event emitting methods like I did in AbstractIoFilterChain?

Cheers,
Trustin

--
what we call human nature is actually human habit
--
http://gleamynode.net/

This e-mail and any attachment is for authorised use by the intended recipient(s) only. It may contain proprietary material, confidential information and/or be subject to legal privilege. It should not be copied, disclosed to, retained or used by, any other party. If you are not an intended recipient then please promptly delete this e-mail and any attachment and all copies and inform the sender. Thank you.