[SNMP4J] SocketTimeout task leaks open socket

[Frank Fock]

Hi Bruno,

Your use case is indeed special although absolutely valid, of course. The fix seems to be pretty clean and straightforward. I will write a unit test for it and if everything is fine, it will be included in the next release.

Best regards,
Frank


> On 26. Jan 2018, at 17:02, Bruno Abreu <bruno.abreu at livingdata.pt> wrote:
> 
> Hello,
> 
> we are using snmp4j to handle NTCIP-PMPP protocol which is based in SNMP over
> TCP.
> 
> More details about NTCIP-PMPP specification, see:
> http://www.ntcip.org/library/documents/pdf/pmpp01.pdf
> 
> In our case, we need to connect to a device that can only accept one
> connection at a time. This lead us to find what we think might be a bug in the
> DefaultTcpTransportMapping class.
> 
> Because of that "one connection at a time" requirement, we need to close the
> socket explicitly, immediately after communication with the device is
> finished, so that other clients can connect. The SocketTimeout timer task,
> however, is still scheduled to run after the socket is closed. It will
> eventually execute and, since it holds a reference to a socket that is already
> closed, invoking close() on that socket will cause no harm. The problem is
> that it also removes from the 'sockets' Hashtable any entry that might have
> been created with the same target address of the socket it was supposed to be
> closing.
> 
> Since it's possible that a new conversation to the same device is already
> taking place, when a new message is about to be sent out, the
> DefaultTcpTransportMapping doesn't find the entry in the 'sockets' table and
> tries to establish a new connection. From that point on we receive Connection
> refused exceptions.
> 
> The following patch succeeded in solving our problem. We submit it here in
> case you want to consider applying to the DefaultTcpTransportMapping class in
> a future release.
> 
> »»»
> --- DefaultTcpTransportMapping.java.2.5.11	2018-01-26 11:36:14.019589314 +0000
> +++ DefaultTcpTransportMapping.java	2018-01-26 11:11:42.000000000 +0000
> @@ -258,6 +258,9 @@ public class DefaultTcpTransportMapping
>     }
>     SocketEntry entry = sockets.remove(remoteAddress);
>     if (entry != null) {
> +      if (entry.getSocketCleaner() != null) {
> +        entry.getSocketCleaner().cancel();
> +      }
>       Socket s = entry.getSocket();
>       if (s != null) {
>         SocketChannel sc = entry.getSocket().getChannel();
> @@ -388,7 +391,9 @@ public class DefaultTcpTransportMapping
> 
>   private synchronized void timeoutSocket(SocketEntry entry) {
>     if (connectionTimeout > 0) {
> -      socketCleaner.schedule(new SocketTimeout(entry), connectionTimeout);
> +      SocketTimeout socketTimeoutCleaner = new SocketTimeout(entry);
> +      entry.setSocketCleaner(socketTimeoutCleaner);
> +      socketCleaner.schedule(socketTimeoutCleaner, connectionTimeout);
>     }
>   }
> 
> @@ -421,6 +426,7 @@ public class DefaultTcpTransportMapping
>     private ByteBuffer readBuffer = null;
>     private volatile int registrations = 0;
>     private volatile int busyLoops = 0;
> +    private SocketTimeout socketCleaner = null;
> 
>     public SocketEntry(TcpAddress address, Socket socket) {
>       this.peerAddress = address;
> @@ -506,6 +512,14 @@ public class DefaultTcpTransportMapping
>       busyLoops = 0;
>     }
> 
> +    public SocketTimeout getSocketCleaner() {
> +        return socketCleaner;
> +    }
> +
> +    public void setSocketCleaner(SocketTimeout socketCleaner) {
> +        this.socketCleaner = socketCleaner;
> +    }
> +
>     public String toString() {
>       return "SocketEntry[peerAddress="+peerAddress+
>           ",socket="+socket+",lastUse="+new
> Date(lastUse/SnmpConstants.MILLISECOND_TO_NANOSECOND)+
> @@ -592,7 +606,9 @@ public class DefaultTcpTransportMapping
>       if (logger.isDebugEnabled()) {
>         logger.debug("Scheduling " + nextRun);
>       }
> -      socketCleaner.schedule(new SocketTimeout(entry), nextRun);
> +      SocketTimeout socketTimeoutCleaner = new SocketTimeout(entry);
> +      entry.setSocketCleaner(socketTimeoutCleaner);
> +      socketCleaner.schedule(socketTimeoutCleaner, nextRun);
>     }
> 
>     public boolean cancel(){
> «««
> 
> 
> That you for you attention,
> Bruno Abreu
> 
> -- 
> Living Data - Sistemas de Informação e Apoio à Decisão, Lda.
> LxFactory - Rua Rodrigues de Faria, 103, edifício I, 4º piso
> 1300-501 LISBOA                       Phone:  +351 213622163
> Portugal                              URL: www.livingdata.pt
> 
> 
> _______________________________________________
> SNMP4J mailing list
> SNMP4J at agentpp.org
> https://oosnmp.net/mailman/listinfo/snmp4j