Potential connection leak in NodeValidator

[tassosl]

Hello,

In NodeValidator#validateAlias method, the Connection constructor is called and it can throw an AerospikeException.Connection exception. The connection is closed in the catch block, but only for exceptions thrown inside the try block. Unfortunately, the connection constructor isn't called inside the try block, so if it throws an exception we will have a connection leak.

I made a pull request that fixes this issue, you can take a look. https://github.com/aerospike/aerospike-client-java/pull/56

We encountered this issue while using aerospike at Thinknear, because we were seeding a host that had a number of aliases, one of them was unreachable and caused that connection leak in our application.

Thanks

[BrianNichols]

There was a connection leak in version 3.2.0. It has already been fixed in 3.2.1 which was released March 3.

[tassosl]

I don't think the leak was plugged completely.

In NodeValidator#validateAlias method a new Connection is constructed. The Connection constructor opens up a socket and input output streams:

            socket.connect(address, timeoutMillis);
            in = socket.getInputStream();
            out = socket.getOutputStream();

Those are only closed if the conn.close() method is invoked. However, if an AerospikeException.Connection is thrown in the Connection constructor, the connection will remain open and will leak, because the connection constructor call isn't part of the try block in the NodeValidator#validateAlias method:

        InetSocketAddress address = new InetSocketAddress(alias.name, alias.port);
        Connection conn = new Connection(address, cluster.getConnectionTimeout());

        try {
                   // code omitted for brevity. 
        }
        catch (Exception e) {
                 // here is where connection is closed, but this won't run if exception happens before try
            conn.close(); 
            throw e;
        }

Between 3.2.0 and 3.2.1 you did plug the leaks that could have happened with command.authenticate(conn...) and Info.request(...) method invocations by wrapping them in the try/catch block that is omitted in the code snippet above.

However, the leak I am talking about is still a potential problem, if an exception is thrown from the code that runs in the Connection constructor.

[BrianNichols]

Was getInputStream() or getOutputStream() throwing the exception?

This is the only way I can see the socket being connected and then not closed. All other exceptions would mean the socket was never connected, thus does not need to be closed.

Your fix does not address the leak, because conn will still be null in the finally block. The close must be called in the Connection constructor to fix the leak.

[tassosl]

Yes, so I staged our application with newest client libraries and the memory issue we were seeing and was attributed to aerospike client connection was gone. This means you fixed the memory leak that was hurting memory consumption in our production systems.

To answer your latest question, yes, I did witness an exception in getInputStream while debugging the memory leak issue locally, having our application (with latest aerospike client libraries) connect to an aerospike server cluster in the cloud (AWS). Now that I think about this case more, I believe it's a far fetched edge case and could only pertain to my local dev system's unique configuration.

You can close this issue and the PR associated with it.

Thank you for your quick responses.