TcpSlave bug, zombie connections

[loen]

We traced a problem with TcpSlave.
If the slave is closed with stop(), connections still stay alive, so the slave actually never terminates, and the masters stay connected (and working).
We fixed this with the following patch (against the CVS version), which keeps a list of opened connections, and kills them on stop.
The problem is easy reproducible, and the patch can be tested, with the sample slave and master pieces of codes which follow (just start first the slave and after a second the master)

# This patch file was generated by NetBeans IDE
# It uses platform neutral UTF-8 encoding and \n newlines.
--- Base (1.8)
+++ Locally Modified (Based On 1.8)
@@ -49,6 +49,9 @@
     private ServerSocket serverSocket;
     final ExecutorService executorService;
 
+    // keep a list of opened tcp handlers so can kill them on close
+    java.util.List<TcpConnectionHandler> listConnections = new java.util.ArrayList<TcpConnectionHandler>();
+    
     public TcpSlave(boolean encapsulated) {
         this(ModbusUtils.TCP_PORT, encapsulated);
     }
@@ -69,6 +72,7 @@
                 socket = serverSocket.accept();
                 TcpConnectionHandler handler = new TcpConnectionHandler(socket);
                 executorService.execute(handler);
+                listConnections.add(handler);
             }
         }
         catch (IOException e) {
@@ -86,6 +90,16 @@
             getExceptionHandler().receivedException(e);
         }
 
+        // kill all tcp handlers and clear the list
+        for (TcpConnectionHandler tch : listConnections) {
+            try {
+                tch.kill();
+            } catch (Throwable ex) {
+                // ignore this one; could not do anything useful
+            }
+        }
+        listConnections.clear();
+        
         // Now close the executor service.
         executorService.shutdown();
         try {
@@ -160,5 +174,15 @@
                 getExceptionHandler().receivedException(new ModbusInitException(e));
             }
         }
+        
+        /**
+         * Kill the handler by closing the socket. Otherwise it would keep the
+         * socket open and avoid the slave to restart on same port.
+         *
+         * @throws IOException
+         */
+        private void kill() throws IOException {
+            socket.close();
     }
 }
+}

Slave test:

package com.example.modbus4j.test;

import com.serotonin.modbus4j.BasicProcessImage;
import com.serotonin.modbus4j.ProcessImageListener;
import com.serotonin.modbus4j.code.DataType;
import com.serotonin.modbus4j.code.RegisterRange;
import com.serotonin.modbus4j.ip.tcp.TcpSlave;
import java.util.concurrent.TimeUnit;
import java.util.logging.Level;
import java.util.logging.Logger;

public class TestTcpSlave {

	private TcpSlave slave;

	/**
	 * @param args the command line arguments
	 */
	public static void main(String[] args) {
		new TestTcpSlave().perform();
	}

	public void perform() {

		try {
			BasicProcessImage bpi = new BasicProcessImage(1);
			bpi.setNumeric(RegisterRange.HOLDING_REGISTER, 0, DataType.TWO_BYTE_INT_SIGNED, (short) -10);
			bpi.setNumeric(RegisterRange.HOLDING_REGISTER, 2, DataType.TWO_BYTE_INT_SIGNED, (short) 250);
			bpi.setNumeric(RegisterRange.HOLDING_REGISTER, 3, DataType.TWO_BYTE_INT_SIGNED, (short) 1200);
			bpi.setNumeric(RegisterRange.HOLDING_REGISTER, 4, DataType.TWO_BYTE_INT_SIGNED, (short) -100);
			bpi.setNumeric(RegisterRange.HOLDING_REGISTER, 5, DataType.TWO_BYTE_INT_SIGNED, (short) 3000);

			bpi.addListener(new ProcessImageListener() {
				@Override
				public void coilWrite(int offset, boolean oldValue, boolean newValue) {
				}

				@Override
				public void holdingRegisterWrite(int offset, short oldValue, short newValue) {
					System.out.println("Reg " + offset + "=" + newValue + " (" + oldValue + ")");
				}
			});

			slave = new TcpSlave(5502, false);

			new Thread(new Runnable() {
				@Override
				public void run() {
					try {
						TimeUnit.SECONDS.sleep(10);
						slave.stop();
					} catch (InterruptedException ex) {
						Logger.getLogger(TestTcpSlave.class.getName()).log(Level.SEVERE, null, ex);
					}
				}
			}).start();;

			slave.addProcessImage(bpi);
			System.out.println("Slave started.");

			try {
				slave.start();
			} catch (Throwable ex) {
			}

			System.out.println("stopped");
			TimeUnit.SECONDS.sleep(2);

			slave = new TcpSlave(5502, false);

			new Thread(new Runnable() {
				@Override
				public void run() {
					try {
						TimeUnit.SECONDS.sleep(10);
						slave.stop();
					} catch (InterruptedException ex) {
						Logger.getLogger(TestTcpSlave.class.getName()).log(Level.SEVERE, null, ex);
					}
				}
			}).start();

			slave.addProcessImage(bpi);
			System.out.println("Slave started again.");

			try {
				slave.start();
			} catch (Throwable ex) {
			}

			System.out.println("stopped again");

		} catch (Throwable ex) {
			Logger.getLogger(TestTcpSlave.class.getName()).log(Level.SEVERE, null, ex);
		}
	}
}

Master test:

package com.example.modbus4j.test;

import com.serotonin.modbus4j.ModbusFactory;
import com.serotonin.modbus4j.ModbusMaster;
import com.serotonin.modbus4j.code.DataType;
import com.serotonin.modbus4j.ip.IpParameters;
import com.serotonin.modbus4j.locator.BaseLocator;
import java.util.concurrent.TimeUnit;
import java.util.logging.Level;
import java.util.logging.Logger;

public class TestWriteRegister {

	public static void main(String[] args) {
		try {
			IpParameters ip = new IpParameters();
			ip.setHost("localhost");
			ip.setPort(5502);
			ip.setEncapsulated(false);

			ModbusFactory factory = new ModbusFactory();
			ModbusMaster master = factory.createTcpMaster(ip, true);

			master.setTimeout(200);
			master.setRetries(0);

			master.init();

			try {
				System.out.println("start");
				while (master.isInitialized()) {
					master.setValue(BaseLocator.holdingRegister(1, 0, DataType.TWO_BYTE_INT_SIGNED), (short) 5566);
					TimeUnit.MILLISECONDS.sleep(500);
				}
			} catch (Throwable ex) {
			} finally {
				master.destroy();
				System.out.println("destroy");
			}

			TimeUnit.SECONDS.sleep(3);

			master.init();

			try {
				System.out.println("start 2");
				while (master.isInitialized()) {
					master.setValue(BaseLocator.holdingRegister(1, 0, DataType.TWO_BYTE_INT_SIGNED), 5566);
					TimeUnit.MILLISECONDS.sleep(500);
				}
			} catch (Throwable ex) {
			} finally {
				master.destroy();
				System.out.println("destroy 2");
			}
		} catch (Throwable ex) {
			Logger.getLogger(TestWriteRegister.class.getName()).log(Level.SEVERE, null, ex);
		}
	}
}

[JoelHaggar]

Thanks very much for this we'll look at getting the patch incorporated in the near future.

Joel.

[mlohbihler]

You're not removing from the list connections that complete on their own, which could cause memory problems. I have included this in the changes to the code.

[mlohbihler]

Changes have been checked into the SF CVS repo.