JBMESSAGING-1225 - PacketFilters implementation

[clebert.suconic]

I'm adding the capability of intercepting packets on ServerSide, right before sending those to the packetHandler.

This is a higher level functionality than the one provided by Mina, and this is how it works:

A Customer can implement an interface defined by org.jboss.messaging.core.remoting.PacketFilter, example:

public class DummyFilter implements PacketFilter
{
 public boolean filterMessage(AbstractPacket packet, PacketHandler handler,
 PacketSender sender)
 {
 System.out.println("Packet " + packet + " intercepted);
 return true;
 }

}

And by simply deploying the DummyFilter on the MicroContainer (anyway you want) will be enough to wire it on the MinaService, done automagically by the MicroContainer. (Thanks for Ales' help).


Example of a deployment descriptor:

<?xml version="1.0" encoding="UTF-8"?>
<deployment xmlns="urn:jboss:bean-deployer:2.0">
 <bean name="MyFilter" class="a.b.DummyFilter"/>
</deployment>

Also, to support testing of this method, I have added back few deploy and undeploy methods to our Servers/Bootstrapserver.

[timfox]

And by simply deploying the DummyFilter on the MicroContainer (anyway you want) will be enough to wire it on the MinaService, done automagically by the MicroContainer. (Thanks for Ales' help).

Remember though that we won't always be running with the MC, e.g. OEM customers running embedded and directly instantiating our POJOs, so the functionality needs to work with or without the MC being available.

[apwalker]

public class DummyFilter implements PacketFilter
{
 public boolean filterMessage(AbstractPacket packet, PacketHandler handler,
 PacketSender sender)
 {
 System.out.println("Packet " + packet + " intercepted);
 return true;
 }

}

IMHO you should use a Packet interface rather than the AbstractPacket as the type that get passed in. I guess so should PackHandler and PacketSender if they aren't already.

Is the fact that you return a boolean does this signal the invocation to proceed? More traditional interceptor design would be if you want to stop the execution flow you throw an Exception in the interceptor. For example a custom security servlet filter would throw a SecurityException. It better to do this because then what was originally called can know why the invocation was halted.

Feel free to ignore my humble rants... but I do like nice clean APIs and once you have one customer who's written a Filter you will never be able to change it again..... :)

[timfox]

Also not sure why we need PacketHandler and PacketSender passed into the method, surely:

public interface Interceptor
{
 public boolean intercept(Packet packet)
 {
 //do something
 return true;
 }
}

is sufficient?

I don't think Filter is a good name either since we already have something completely different in core called Filter.

[clebert.suconic]

Remember though that we won't always be running with the MC, e.g. OEM customers running embedded and directly instantiating our POJOs, so the functionality needs to work with or without the MC being available.

A simple call to MinaServrice.addFilter (or interceptor case we rename it, should be enough). We have a way around it, and it would be easy either way.

Also not sure why we need PacketHandler and PacketSender passed into the method, surely:

It would be useful when you want to intercept the return value of the Handler. If we had the intercept working on both ways (client2Server and server2Client, you would never had the context of the return inside the same method). This would be like the ServletFilter where you have the InputStream and OutputStream available on the call.

Case you wanted to override a return, you could something like:

public void intercept(Packet packet, Handler handler, final Sender sender)
{
 handler.handle(packet, new Sender() {
 public void send(Packet packet)
 {
 sentPacket = packet;
 sender.sent(packet);
 }
 };
}

It would be harder to associate the sent and returned packet without this context.

[clebert.suconic]

"apwalker" wrote:
IMHO you should use a Packet interface rather than the AbstractPacket as the type that get passed in. I guess so should PackHandler and PacketSender if they aren't already.

We currently don't have AbstractPacket implementing an interface. Should we create one? Jeff... Tim?

[timfox]

I think

public void intercept(Packet packet, Handler handler, final Sender sender)
{
 handler.handle(packet, new Sender() {
 public void send(Packet packet)
 {
 sentPacket = packet;
 sender.sent(packet);
 }
 };
}

Is way too complex.

I think we should keep it simple like the standard old interceptors.

[clebert.suconic]

Just to complete the thread...


We will change the intercept method as

void intercept (Packet packet) throws MessagingException

And we will also change how this is configured. An user will have to enter its interceptor on the jbm-config.xml, and that interceptor will need to be available on the server side classLoader.

[timfox]

An user will have to enter its interceptor on the jbm-config.xml, and that interceptor will need to be available on the server side classLoader.

Not necessarily.

A user could write a simple service that deploys in its own sar, looks up the messaging server and calls messagingServer.addInterceptor(), so the correct classloader is used.

[clebert.suconic]

This will be a second way to do it anyway.:-)

And if you can actually call the addInterceptor method, you could annotate addInterceptor with @Install, and this could be done automatically.