steveohara / j2mod

Enhanced Modbus library implemented in the Java programming language
Apache License 2.0
267 stars 111 forks source link

Double socket creation in ModbusTCP #33

Closed stoorm5 closed 7 years ago

stoorm5 commented 7 years ago

It seems that the execution of the following code creates two different sockets:

ModbusTCPMaster` master = new ModbusTCPMaster(slaveAddress,slavePort);
master.connect();
InputRegister[] registers = master.readMultipleRegisters(unitId, ref, count);

The constructor of ModbusTCPMaster, also creates a new instance of TCPMasterConnection (let's name it mc1) The first socket (let's name it s1) is created by TCPMasterConnection connect() method that also instantiate a new ModbusTCPTransport, by passing the socket to its constructor; finally this method sets TCPMasterConnection's connected property to true. After this, connection.getModbusTransport().createTransaction(); is called. This method, at first, checks if master variable is null (that is the case, because it is not set anywhere in the code) and creates another instance of TCPMasterConnection (let's name it mc2), assinging it to master variable (let's note that mc2.connected == false). Finally, it returns a new instance of ModbusTCPTransaction. The code of ModbusTCPMaster's readMultipleRegisters(int unitId, int ref, int count) method, after building the modbus request, invokes the execute() method on the previously returned istance of ModbusTCPTransaction. This method, first check if the instance of TCPMasterConnection referenced by this transaction is connected; in this case the associated instance is mc2 (not connected) and not mc1 (connected), thus implying the creation of a new socket (let's name it s2) that is then used to send the request. The previously created socket (s1) is never used to send/receive data. I used readMultipleRegisters method as an example, but every method using ModbusTCPTransaction's executeMethod() has the same behaviour. I think that a possible solution could be adding a setMaster(TCPMasterConnection master) method in ModbusTCPTransport class and modifiyng prepareTransport method in TCPMasterConnection in the following way:

    private void prepareTransport(boolean useRtuOverTcp) throws IOException {
        if (transport == null) {
            if (useRtuOverTcp) {
                logger.trace("prepareTransport() -> using RTU over TCP transport.");
                transport = new ModbusRTUTCPTransport(socket);
                transport.setMaster(this);
            }
            else {
                logger.trace("prepareTransport() -> using standard TCP transport.");
                transport = new ModbusTCPTransport(socket);
                transport.setMaster(this);
            }
        }
        else {
            logger.trace("prepareTransport() -> using custom transport: {}", transport.getClass().getSimpleName());
            transport.setSocket(socket);
        }
    }
stoorm5 commented 7 years ago

I found another potential issue: constructors of both ModbusRTUTCPTransport and ModbusTCPTransport overrides the timeout, eventually set on the socket by TCPMasterConnection, with the default value Modbus.DEFAULT_TIMEOUT. A possible solution can be adding transport.setTimeout(timeout); at the end of prepareTransport method in TCPMasterConnection.

steveohara commented 7 years ago

Merged into v2.3.1