[ 
https://issues.apache.org/jira/browse/CLOUDSTACK-9348?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=15243335#comment-15243335
 ] 

ASF GitHub Bot commented on CLOUDSTACK-9348:
--------------------------------------------

Github user rafaelweingartner commented on a diff in the pull request:

    https://github.com/apache/cloudstack/pull/1493#discussion_r59915315
  
    --- Diff: utils/src/test/java/com/cloud/utils/testcase/NioTest.java ---
    @@ -19,146 +19,198 @@
     
     package com.cloud.utils.testcase;
     
    -import java.nio.channels.ClosedChannelException;
    -import java.util.Random;
    -
    -import junit.framework.TestCase;
    -
    -import org.apache.log4j.Logger;
    -import org.junit.Assert;
    -
    +import com.cloud.utils.concurrency.NamedThreadFactory;
     import com.cloud.utils.exception.NioConnectionException;
     import com.cloud.utils.nio.HandlerFactory;
     import com.cloud.utils.nio.Link;
     import com.cloud.utils.nio.NioClient;
     import com.cloud.utils.nio.NioServer;
     import com.cloud.utils.nio.Task;
     import com.cloud.utils.nio.Task.Type;
    +import org.apache.log4j.Logger;
    +import org.junit.After;
    +import org.junit.Assert;
    +import org.junit.Before;
    +import org.junit.Test;
     
    -/**
    - *
    - *
    - *
    - *
    - */
    +import java.io.IOException;
    +import java.net.InetSocketAddress;
    +import java.nio.channels.ClosedChannelException;
    +import java.nio.channels.Selector;
    +import java.nio.channels.SocketChannel;
    +import java.util.ArrayList;
    +import java.util.List;
    +import java.util.Random;
    +import java.util.concurrent.ExecutorService;
    +import java.util.concurrent.Executors;
    +
    +public class NioTest {
     
    -public class NioTest extends TestCase {
    +    private static final Logger LOGGER = Logger.getLogger(NioTest.class);
     
    -    private static final Logger s_logger = Logger.getLogger(NioTest.class);
    +    final private int totalTestCount = 10;
    +    private int completedTestCount = 0;
     
    -    private NioServer _server;
    -    private NioClient _client;
    +    private NioServer server;
    +    private List<NioClient> clients = new ArrayList<>();
    +    private List<NioClient> maliciousClients = new ArrayList<>();
     
    -    private Link _clientLink;
    +    private ExecutorService clientExecutor = 
Executors.newFixedThreadPool(totalTestCount, new 
NamedThreadFactory("NioClientHandler"));;
    +    private ExecutorService maliciousExecutor = 
Executors.newFixedThreadPool(5*totalTestCount, new 
NamedThreadFactory("MaliciousNioClientHandler"));;
     
    -    private int _testCount;
    -    private int _completedCount;
    +    private Random randomGenerator = new Random();
    +    private byte[] testBytes;
     
         private boolean isTestsDone() {
             boolean result;
             synchronized (this) {
    -            result = _testCount == _completedCount;
    +            result = totalTestCount == completedTestCount;
             }
             return result;
         }
     
    -    private void getOneMoreTest() {
    -        synchronized (this) {
    -            _testCount++;
    -        }
    -    }
    -
         private void oneMoreTestDone() {
             synchronized (this) {
    -            _completedCount++;
    +            completedTestCount++;
             }
         }
     
    -    @Override
    +    @Before
         public void setUp() {
    -        s_logger.info("Test");
    +        LOGGER.info("Setting up Benchmark Test");
     
    -        _testCount = 0;
    -        _completedCount = 0;
    -
    -        _server = new NioServer("NioTestServer", 7777, 5, new 
NioTestServer());
    -        try {
    -            _server.start();
    -        } catch (final NioConnectionException e) {
    -            fail(e.getMessage());
    -        }
    +        completedTestCount = 0;
    +        testBytes = new byte[1000000];
    +        randomGenerator.nextBytes(testBytes);
     
    -        _client = new NioClient("NioTestServer", "127.0.0.1", 7777, 5, new 
NioTestClient());
    +        // Server configured with one worker
    +        server = new NioServer("NioTestServer", 0, 1, new NioTestServer());
             try {
    -            _client.start();
    +            server.start();
             } catch (final NioConnectionException e) {
    -            fail(e.getMessage());
    +            Assert.fail(e.getMessage());
             }
     
    -        while (_clientLink == null) {
    -            try {
    -                s_logger.debug("Link is not up! Waiting ...");
    -                Thread.sleep(1000);
    -            } catch (final InterruptedException e) {
    -                // TODO Auto-generated catch block
    -                e.printStackTrace();
    +        // 5 malicious clients per valid client
    +        for (int i = 0; i < totalTestCount; i++) {
    +            for (int j = 0; j < 5; j++) {
    +                final NioClient maliciousClient = new 
NioMaliciousClient("NioMaliciousTestClient-" + i, "127.0.0.1", 
server.getPort(), 1, new NioMaliciousTestClient());
    +                maliciousClients.add(maliciousClient);
    +                maliciousExecutor.submit(new 
ThreadedNioClient(maliciousClient));
                 }
    +            final NioClient client = new NioClient("NioTestClient-" + i, 
"127.0.0.1", server.getPort(), 1, new NioTestClient());
    +            clients.add(client);
    +            clientExecutor.submit(new ThreadedNioClient(client));
             }
         }
     
    -    @Override
    +    @After
         public void tearDown() {
    +        stopClient();
    +        stopServer();
    +    }
    +
    +    protected void stopClient() {
    +        for (NioClient client : clients) {
    +            client.stop();
    +        }
    +        for (NioClient maliciousClient : maliciousClients) {
    +            maliciousClient.stop();
    +        }
    +        LOGGER.info("Clients stopped.");
    +    }
    +
    +    protected void stopServer() {
    +        server.stop();
    +        LOGGER.info("Server stopped.");
    +    }
    +
    +    @Test
    +    public void testConnection() {
    +        final long currentTime = System.currentTimeMillis();
             while (!isTestsDone()) {
    +            if (System.currentTimeMillis() - currentTime > 600000) {
    +                Assert.fail("Failed to complete test within 600s");
    +            }
                 try {
    -                s_logger.debug(_completedCount + "/" + _testCount + " 
tests done. Waiting for completion");
    +                LOGGER.debug(completedTestCount + "/" + totalTestCount + " 
tests done. Waiting for completion");
                     Thread.sleep(1000);
                 } catch (final InterruptedException e) {
    -                // TODO Auto-generated catch block
    -                e.printStackTrace();
    +                Assert.fail(e.getMessage());
                 }
             }
    -        stopClient();
    -        stopServer();
    +        LOGGER.debug(completedTestCount + "/" + totalTestCount + " tests 
done.");
         }
     
    -    protected void stopClient() {
    -        _client.stop();
    -        s_logger.info("Client stopped.");
    +    protected void doServerProcess(final byte[] data) {
    +        oneMoreTestDone();
    +        Assert.assertArrayEquals(testBytes, data);
    +        LOGGER.info("Verify data received by server done.");
         }
     
    -    protected void stopServer() {
    -        _server.stop();
    -        s_logger.info("Server stopped.");
    +    public byte[] getTestBytes() {
    +        return testBytes;
         }
     
    -    protected void setClientLink(final Link link) {
    -        _clientLink = link;
    +    public class ThreadedNioClient implements Runnable {
    +        final private NioClient client;
    +        ThreadedNioClient(final NioClient client) {
    +            this.client = client;
    +        }
    +
    +        @Override
    +        public void run() {
    +            try {
    +                client.start();
    +            } catch (NioConnectionException e) {
    +                Assert.fail(e.getMessage());
    +            }
    +        }
         }
     
    -    Random randomGenerator = new Random();
    +    public class NioMaliciousClient extends NioClient {
     
    -    byte[] _testBytes;
    +        public NioMaliciousClient(String name, String host, int port, int 
workers, HandlerFactory factory) {
    +            super(name, host, port, workers, factory);
    +        }
     
    -    public void testConnection() {
    -        _testBytes = new byte[1000000];
    -        randomGenerator.nextBytes(_testBytes);
    -        try {
    -            getOneMoreTest();
    -            _clientLink.send(_testBytes);
    -            s_logger.info("Client: Data sent");
    -            getOneMoreTest();
    -            _clientLink.send(_testBytes);
    -            s_logger.info("Client: Data sent");
    -        } catch (final ClosedChannelException e) {
    -            // TODO Auto-generated catch block
    -            e.printStackTrace();
    +        @Override
    +        protected void init() throws IOException {
    +            _selector = Selector.open();
    +            try {
    +                _clientConnection = SocketChannel.open();
    +                LOGGER.info("Connecting to " + _host + ":" + _port);
    +                final InetSocketAddress peerAddr = new 
InetSocketAddress(_host, _port);
    +                _clientConnection.connect(peerAddr);
    +                // Hang in there don't do anything
    +                Thread.sleep(Long.MAX_VALUE);
    --- End diff --
    
    @bhaisaab I am carefully reading the code, but it is just too big and 
complicated too. I am assuming you are just using what was already there, so no 
complaining about the size of the test method (which is too big).
    
    If you remove that Sleep is the connection (_clientConnection) going to be 
closed? Do you need to test with a real connection, couldn’t you use mocks? I 
am assuming that is not possible because of the current code base that is not 
prepared for unit tests.


> CloudStack Server degrades when a lot of connections on port 8250
> -----------------------------------------------------------------
>
>                 Key: CLOUDSTACK-9348
>                 URL: https://issues.apache.org/jira/browse/CLOUDSTACK-9348
>             Project: CloudStack
>          Issue Type: Bug
>      Security Level: Public(Anyone can view this level - this is the 
> default.) 
>            Reporter: Rohit Yadav
>            Assignee: Rohit Yadav
>             Fix For: 4.9.0
>
>
> An intermittent issue was found with a large CloudStack deployment, where 
> servers could not keep agents connected on port 8250.
> All connections are handled by accept() in NioConnection:
> https://github.com/apache/cloudstack/blob/master/utils/src/main/java/com/cloud/utils/nio/NioConnection.java#L125
> A new connection is handled by accept() which does blocking SSL handshake. A 
> good fix would be to make this non-blocking and handle expensive tasks in 
> separate threads/pool. This way the main IO loop won't be blocked and can 
> continue to serve other agents/clients.



--
This message was sent by Atlassian JIRA
(v6.3.4#6332)

Reply via email to