Why does my synchronized method not work as expected?

Question

I have a class called GerenciadorServerSocket:

@Service
public class GerenciadorServerSocket {

    private CopyOnWriteArrayList<Integer> portasUsadas = new CopyOnWriteArrayList<>(new ArrayList<>());

    public boolean isPortaUsada(Integer port){
        return this.portasUsadas.contains(port);
    }

    public void addPorta(Integer port){
        this.portasUsadas.addIfAbsent(port);
    }
}

When my application is lifted, via spring boot, it is injected into my class SocketHandler, which is injected into my boot class called ApplicationRunnerAdapter:

@Component
public class ApplicationRunnerAdapter implements ApplicationRunner{

    private final TerminalService terminalService;
    private final SocketHandler socketHandler;

    @Autowired
    public ApplicationRunnerAdapter(TerminalService terminalService, SocketHandler socketHandler){
        this.terminalService = terminalService;
        this.socketHandler = socketHandler;
    }

    @Override
    public void run(ApplicationArguments args){
        List<Terminal> terminais = terminalService.findAllByStatusAndEmpresaStatus(TerminalStatus.ATIVO, EmpresaStatus.HOMOLOGACAO);
        terminais.forEach(terminal -> new Thread(new SocketServerChannel(terminal, terminalService, socketHandler)).start());
    }
}

When Runner is ready, the method will be executed run, searching a list of database objects to create multiple sockets servers.

Class SocketServerChannel:

public class SocketServerChannel implements Runnable {

    private Logger logger = LoggerFactory.getLogger(this.getClass());

    private final TerminalService terminalService;
    private final SocketHandler socketHandler;
    private final Terminal terminal;

    public SocketServerChannel(Terminal terminal, TerminalService terminalService, SocketHandler socketHandler){
        this.terminal = terminal;
        this.terminalService = terminalService;
        this.socketHandler = socketHandler;
    }

    @Override
    public void run(){
        try {
            AsynchronousServerSocketChannel serverChannel = createSocketChannel();
            while(serverChannel != null && serverChannel.isOpen()){
                Future<AsynchronousSocketChannel> acceptFuture = serverChannel.accept();
                if (acceptFuture != null){
                    AsynchronousSocketChannel socketChannel = acceptFuture.get();
                    logger.info("Chegou conexão socket. Ip: "+socketChannel.getLocalAddress().toString()+"  "+socketChannel.getRemoteAddress().toString());
                    new Thread(new SocketClientChannel(getProtocolo(this.terminal), getPort(this.terminal), socketChannel, serverChannel, socketHandler, terminalService)).start();
                }
            }
        }catch (Exception e){
            e.printStackTrace();
        }
    }

    private AsynchronousServerSocketChannel createSocketChannel() throws IOException {
        Integer port = getPort(this.terminal);
        if (port == null || socketHandler.isPortaUsada(port)){
            logger.error("Porta "+port+" já está em uso! Favor, utilizar outra.");
            return null;
        }
        socketHandler.addPortaUtilizada(port);
        logger.info("Criando conexão socket na porta: "+port);
        return AsynchronousServerSocketChannel.open().bind(new InetSocketAddress("10.1.1.150", port)); //TODO tocar host name
    }

}

In this class is where my problem comes from, as some objects may come with repeated ports and it can no longer open a socket connection to that same port. I have a class called SocketHandlerwhich handles all socket writing operations, and in it as well, I check whether the door is already in use or not. This class has all the synchronized methods and it is instantiated only once at system startup in my class ApplicationRunnerAdapter.

Class SocketHandler:

@Service
public class SocketHandler implements SocketCallBackHandler {

    @Override
    public synchronized boolean isPortaUsada(Integer porta) {
        Objects.requireNonNull(porta);
        return this.gerenciadorServerSocket.isPortaUsada(porta);
    }

    @Override
    public synchronized void addPortaUtilizada(Integer porta) {
        Objects.requireNonNull(porta);
        this.gerenciadorServerSocket.addPorta(porta);
    }
}

As can be seen, the methods are as synchronized, that is, several threds would wait for the block to be executed so it can run. It turns out, even with these checks, he’s trying to create ports that are already in use.

Where I’m going wrong?

Answer 1

We have a multi-tasking situation:

terminais.forEach(terminal -> new Thread(new SocketServerChannel(terminal, terminalService, socketHandler)).start());

But the way to deal with this situation turns that code into a ticking time bomb:

private AsynchronousServerSocketChannel createSocketChannel() throws IOException {
        Integer port = getPort(this.terminal);
        if (port == null || socketHandler.isPortaUsada(port)){
            logger.error("Porta "+port+" já está em uso! Favor, utilizar outra.");
            return null;
        }
        //-->estopim!!! acenda o pavio da bomba aqui<--
        socketHandler.addPortaUtilizada(port);
        logger.info("Criando conexão socket na porta: "+port);
        return AsynchronousServerSocketChannel.open().bind(new InetSocketAddress("10.1.1.150", port)); //TODO tocar host name
    }

Between the call of socketHandler.isPortaUsada(port) and socketHandler.addPortaUtilizada(port) the operating system can schedule the thread to run at another time, or the threas were run in parallel, and arrived at the same conclusion.

That means a thousand things may have happened, and the condition that had been stipulated is no longer true.

To solve the problem, a method is required where isPortaUsada and addPortaUtilizada are an atomic operation.

synchronized boolean addPortaUtilizadaAtomicamente(Integer port) {
    if(isPortaUsada(port)) { return false; }
    addPortaUtilizada(port);
    return true;
}

Metaphorically, turn your code into an atomic bomb.