1

Below is my run() of my Server Socket Thread which will be running as Executors.newWorkStealingPool().submit(() -> mainServer.run()); and accepts client connections. It runs fine but Sonar is complaining it as Bug of Type Loops should not be infinite (squid:S2189)

    class MainServer {
    private final ServerSocket serverSocket;    
    private final boolean checkClientCerts;
    private static final org.slf4j.Logger LOG = LoggerFactory.getLogger(MainServer.class.getName());
    private final int threadPoolSize;
    private boolean running;
    private ExecutorService executorService;    

 MainServer(int port, boolean checkClientCerts, int threadPoolSize, InetAddress bindAddress) throws IOException {

        LOG.debug("Locating server socket factory for SSL...");
        SSLServerSocketFactory factory = (SSLServerSocketFactory) SSLServerSocketFactory.getDefault();
        LOG.debug("Creating a server socket on port " + port);
        SSLServerSocket serverSocket = (SSLServerSocket) factory.createServerSocket(port, 0, bindAddress);        
        this.checkClientCerts = checkClientCerts;
        this.threadPoolSize = threadPoolSize;
    }

    void run() {
        running = true;
        DefaultThreadFactory threadFactory = new DefaultThreadFactory("SSLHandshake");
        executorService = new ShutdownThreadPoolExecutor(threadPoolSize,threadFactory);

        while (running) {
            Socket clientSocket;

            try {
                clientSocket = serverSocket.accept();
                MainServerHandshakeThread handshakeThread = new MainServerHandshakeThread(clientSocket, this);                
                executorService.submit(handshakeThread);

            } catch (IOException ex) {
                LOG.error("Error accepting connection",ex);
            }

        }
    }

    public void shutdown() {
        LOG.info("Stopping main server...");
        running = false;
        try {
            if (serverSocket!=null) {
                serverSocket.close();
            }
        } catch(IOException ex) {
            LOG.debug("Failed to close socket",ex);
        }

        executorService.shutdown();
        try {
            if (!executorService.awaitTermination(500, TimeUnit.MILLISECONDS)) {
                executorService.shutdownNow();
            } 
        } catch (InterruptedException e) {
            executorService.shutdownNow();
        }             
        LOG.info("Main server stopped...");
    }
}

Can someone please help me how to optimize above block of code to get rid of Sonar complaint?

user207421
  • 305,947
  • 44
  • 307
  • 483
OTUser
  • 3,788
  • 19
  • 69
  • 127
  • Well, why not apply what the warning says and make the `while (running) {...}` loop **not** infinite? You never change the `running` variable in the code that you shared. – Fureeish Oct 09 '19 at 19:45
  • @Fureeish updated the question with complete code, `mainServer` will be running as `Executors.newWorkStealingPool().submit(() -> mainServer.run());` and it will be stopped by invoking `shutdown()` when the application is stopped – OTUser Oct 09 '19 at 19:51
  • Does marking `running` as `volatile` get rid of the bug? – Fureeish Oct 09 '19 at 19:53
  • @Fureeish Yes, surprisingly marking `running` as `volatile` get rid of the bug, whats the magic behind it? – OTUser Oct 09 '19 at 19:55
  • Please see my answer, I tried to both briefly explain what's happening and to provide some additional reads. – Fureeish Oct 09 '19 at 20:01

1 Answers1

1

Mark your running as volatile.

volatile marks a variable visible to as changeable1 by other threads. That means that an optimiser (or a code-analyzer, like SonarQube) cannot assume that other threads do not modify such variable. In your example, both of those could assume that running never changes, thus marking your code as having an infinite loop. A similar example can be found in this answer.

You should mark your variables as volatile, if they can be accessed and modified by other threads.

1 Corrected by user207421.

Fureeish
  • 12,533
  • 4
  • 32
  • 62