Details

    • Type: New Feature
    • Status: Resolved
    • Priority: Minor
    • Resolution: Fixed
    • Affects Version/s: None
    • Fix Version/s: 2.0.8
    • Component/s: Filter
    • Labels:
      None
    • Environment:
      All

      Description

      Create a filter that will throttle connections. This filter will monitor newly created sessions and if new connections from the same IP address come in too fast, drop the connections.

        Activity

        Hide
        elihusmails Mark Webb added a comment -

        Checked in the code.

        org.apache.mina.filter.ConnectionThrottleFilter

        Show
        elihusmails Mark Webb added a comment - Checked in the code. org.apache.mina.filter.ConnectionThrottleFilter
        Hide
        elihusmails Mark Webb added a comment -

        Code has been checked in.

        Show
        elihusmails Mark Webb added a comment - Code has been checked in.
        Hide
        proyal peter royal added a comment -

        Cool! A definite case where the ConcurrentHashMap would help though.. It'll allow concurrent modifications, whereas the Synchronized map will become a choke point of its own when a bunch of connection requests come in

        Show
        proyal peter royal added a comment - Cool! A definite case where the ConcurrentHashMap would help though.. It'll allow concurrent modifications, whereas the Synchronized map will become a choke point of its own when a bunch of connection requests come in
        Hide
        elihusmails Mark Webb added a comment -

        After performing some more tests, I think I now disagree. I have created a
        test case where I make 2 nearly simultaneous connections to a server using
        the ConnectionThrottleFilter and they both pass. Seeing that this is our
        worst case scenario (malicious client blasting the server), I think we may
        need to either:

        1. speed up the logic in the filter to prevent thread interleavnig
        2. use a synchronized map

        I vote for #2 to be safe. In order to support this change, I have made the
        clients Map in ConnectionThrottleFilter a synchronized Map using the
        Collections class and things work better in my test code. I can post the
        test code if you are interested.


        ..Cheers
        Mark

        Show
        elihusmails Mark Webb added a comment - After performing some more tests, I think I now disagree. I have created a test case where I make 2 nearly simultaneous connections to a server using the ConnectionThrottleFilter and they both pass. Seeing that this is our worst case scenario (malicious client blasting the server), I think we may need to either: 1. speed up the logic in the filter to prevent thread interleavnig 2. use a synchronized map I vote for #2 to be safe. In order to support this change, I have made the clients Map in ConnectionThrottleFilter a synchronized Map using the Collections class and things work better in my test code. I can post the test code if you are interested. – ..Cheers Mark
        Hide
        elihusmails Mark Webb added a comment - - edited

        ----- SERVER CODE -----

        public class ConnFilterServer extends IoHandlerAdapter {
        
        	protected static final int PORT = 5678;
        	
        	public ConnFilterServer() throws IOException{
        		IoAcceptor acceptor = new SocketAcceptor();
        		ConnectionThrottleFilter ctFilter = new ConnectionThrottleFilter();
        		acceptor.getFilterChain().addLast("ConnThrottle", ctFilter);
        		acceptor.setHandler( this );
        		acceptor.setLocalAddress( new InetSocketAddress(PORT));
        		acceptor.bind();
        		System.out.println("Connection Filter Server running...");
        	}
        
        	public void sessionClosed(IoSession session) throws Exception {
        		SessionLog.info( session, "The session has been closed" );
        	}
        	
        	public static void main(String[] args) throws IOException {
        		new ConnFilterServer();
        	}
        }
        
        

        — END SERVER CODE ----

        --CLIENT CODE--

        public class ConnFilterClient extends IoHandlerAdapter implements IoFutureListener {
        
        	public ConnFilterClient(){
        		makeConnection( "localhost", ConnFilterServer.PORT );
        		makeConnection( "localhost", ConnFilterServer.PORT );
        	}
        	
        	private void makeConnection( String host, int port){
        		IoConnector connector = new SocketConnector();
        		connector.setHandler( this );
        		ConnectFuture connFuture = connector.connect( new InetSocketAddress(host,port));
        		connFuture.addListener( this );
        	}
        	
        	public void operationComplete(IoFuture future) {
        		if( future instanceof ConnectFuture ){
        			ConnectFuture connFuture = (ConnectFuture)future;
        			if( connFuture.isConnected() ){
        				System.out.println("Connected...");
        				IoSession session = connFuture.getSession();
        				session.write( ByteBuffer.wrap("Hello World".getBytes()) );
        			}
        		}
        	}
        
        	public static void main(String[] args) {
        		new ConnFilterClient();
        	}
        }
        
        

        — END CLIENT CODE —

        Show
        elihusmails Mark Webb added a comment - - edited ----- SERVER CODE ----- public class ConnFilterServer extends IoHandlerAdapter { protected static final int PORT = 5678; public ConnFilterServer() throws IOException{ IoAcceptor acceptor = new SocketAcceptor(); ConnectionThrottleFilter ctFilter = new ConnectionThrottleFilter(); acceptor.getFilterChain().addLast( "ConnThrottle" , ctFilter); acceptor.setHandler( this ); acceptor.setLocalAddress( new InetSocketAddress(PORT)); acceptor.bind(); System .out.println( "Connection Filter Server running..." ); } public void sessionClosed(IoSession session) throws Exception { SessionLog.info( session, "The session has been closed" ); } public static void main( String [] args) throws IOException { new ConnFilterServer(); } } — END SERVER CODE ---- -- CLIENT CODE -- public class ConnFilterClient extends IoHandlerAdapter implements IoFutureListener { public ConnFilterClient(){ makeConnection( "localhost" , ConnFilterServer.PORT ); makeConnection( "localhost" , ConnFilterServer.PORT ); } private void makeConnection( String host, int port){ IoConnector connector = new SocketConnector(); connector.setHandler( this ); ConnectFuture connFuture = connector.connect( new InetSocketAddress(host,port)); connFuture.addListener( this ); } public void operationComplete(IoFuture future ) { if ( future instanceof ConnectFuture ){ ConnectFuture connFuture = (ConnectFuture) future ; if ( connFuture.isConnected() ){ System .out.println( "Connected..." ); IoSession session = connFuture.getSession(); session.write( ByteBuffer.wrap( "Hello World" .getBytes()) ); } } } public static void main( String [] args) { new ConnFilterClient(); } } — END CLIENT CODE —
        Hide
        elihusmails Mark Webb added a comment -

        Although I have checked in the code using a synchronized Map, I would like to keep this issue open until I can find an alternative.

        Show
        elihusmails Mark Webb added a comment - Although I have checked in the code using a synchronized Map, I would like to keep this issue open until I can find an alternative.
        Hide
        trustin Trustin Lee added a comment -

        We also need to provide a property that limits the number of the clients from the same host.

        For now we have a problem with more than one client that connects within the same milliseconds because we are using System.currentTimeMillis(). Adding additional counter will fix it. Therefore, we need some simple bookeeping data structure here.

        Show
        trustin Trustin Lee added a comment - We also need to provide a property that limits the number of the clients from the same host. For now we have a problem with more than one client that connects within the same milliseconds because we are using System.currentTimeMillis(). Adding additional counter will fix it. Therefore, we need some simple bookeeping data structure here.
        Hide
        elihusmails Mark Webb added a comment -

        Couldn't we just change the call to System.currentTimeMillis() to System.nanoTime()

        http://java.sun.com/javase/6/docs/api/java/lang/System.html#nanoTime()

        Show
        elihusmails Mark Webb added a comment - Couldn't we just change the call to System.currentTimeMillis() to System.nanoTime() http://java.sun.com/javase/6/docs/api/java/lang/System.html#nanoTime( )
        Hide
        trustin Trustin Lee added a comment -

        What would happen when two clients connect within the same nanosecs? Whatever time unit we use, we need an additional counter.

        Show
        trustin Trustin Lee added a comment - What would happen when two clients connect within the same nanosecs? Whatever time unit we use, we need an additional counter.
        Hide
        trustin Trustin Lee added a comment -

        We don't need any core API modification. Therefore, we can implement this feature later, but at least before 2.0.0-RC1.

        Show
        trustin Trustin Lee added a comment - We don't need any core API modification. Therefore, we can implement this feature later, but at least before 2.0.0-RC1.
        Hide
        elihusmails Mark Webb added a comment -

        Maybe this could be solved using an ExpiringHashMap. The expiration time would be the length of time that a host cannot have more than one connection.

        Show
        elihusmails Mark Webb added a comment - Maybe this could be solved using an ExpiringHashMap. The expiration time would be the length of time that a host cannot have more than one connection.
        Hide
        elecharny Emmanuel Lecharny added a comment -

        The ConnectionThrottlerFilter is uterly broken. The conecpt itself is broken.

        One should never try to fix such an issue in the applicatioin itself, it should be dealt with at a upper level. If a rogue client is trying to DDOS the server by creating thousands of sessions, this should be detected before the MINA server is hit.

        The real problem with this approach is that we have a map storing the created session forever. We Never delete any of the session from the map, which will lead to a OOM in the long run (and even if we do keep the session for only a period of time, that would put a huge strain on the server : in the case we do have a DDOS, we would have to handle tens of thousands of requests per second, though a synchronized map. It's not going to fly...)

        I would rather suggest we remove this filter from MINA. Use the right tool for the right problem...

        Show
        elecharny Emmanuel Lecharny added a comment - The ConnectionThrottlerFilter is uterly broken. The conecpt itself is broken. One should never try to fix such an issue in the applicatioin itself, it should be dealt with at a upper level. If a rogue client is trying to DDOS the server by creating thousands of sessions, this should be detected before the MINA server is hit. The real problem with this approach is that we have a map storing the created session forever . We Never delete any of the session from the map, which will lead to a OOM in the long run (and even if we do keep the session for only a period of time, that would put a huge strain on the server : in the case we do have a DDOS, we would have to handle tens of thousands of requests per second, though a synchronized map. It's not going to fly...) I would rather suggest we remove this filter from MINA. Use the right tool for the right problem...
        Hide
        elecharny Emmanuel Lecharny added a comment -

        Anyway, I have added some lock to protect the filter against concurrent updates (it's not enough to have a Synchornized map), used a ConcurrentHashMap instead of a Synchronized hashMap and added a cleanup thread to be sure teh Map is not growing up to the available memory...

        Show
        elecharny Emmanuel Lecharny added a comment - Anyway, I have added some lock to protect the filter against concurrent updates (it's not enough to have a Synchornized map), used a ConcurrentHashMap instead of a Synchronized hashMap and added a cleanup thread to be sure teh Map is not growing up to the available memory...
        Hide
        elecharny Emmanuel Lecharny added a comment -

        Fixed with commit de58078ce2139cffbec84da63b5030e77e592dac

        Show
        elecharny Emmanuel Lecharny added a comment - Fixed with commit de58078ce2139cffbec84da63b5030e77e592dac

          People

          • Assignee:
            elihusmails Mark Webb
            Reporter:
            elihusmails Mark Webb
          • Votes:
            0 Vote for this issue
            Watchers:
            2 Start watching this issue

            Dates

            • Created:
              Updated:
              Resolved:

              Development