Why does my synchronized method not work as expected?

Asked

Viewed 93 times

2

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?

  • Have you tried synchronizing the method createSocketChannel()?

  • I don’t know if it would make sense createSocketChannel()is in wheel itself thread, so I believe it would be synchronised within each thread, and not shared. Right?

1 answer

2


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.

  • I ended up coming to that same conclusion, I will put your answer as the correct one. But because you think the code would be a ticking time bomb?

  • When there are bugs in threading, the program can work correctly the first time, when the programmer interacts with the program. The bug usually appears when the user starts using the program. In short, it is a matter of time.

  • Ah. I got it. Fortunately, I caught the problem at development time. Regarding the use of methods synchronization, do you think it was well architected? In this case, you will not have competition problems?

  • I would move the synchronization to the class GerenciadorServerSocket, she who has the role of taking care of the doors. As for the SocketHandler, I would remove the methods isPortaUsada,addPortaUtilizada, and add the method getGerenciadorServerSocket(), 'cause that’s not even his problem, so there’s no point in him stepping in.

Browser other questions tagged

You are not signed in. Login or sign up in order to post.