TCP connection leaking on closing connection

[gfcittolin]

Current Behavior

When calling the (undocumented) Controller.destroy() function on an unconnected session, the connection and the controller's instance don't get actually destroyed, leaking the instance itself a TCP socket handle (including its file descriptor). When done multiple times, this leads to a file descriptor starvation, causing any subsequent I/O on the whole process to fail with EMFILE.

Expected Behavior

Calling Controller.destroy() should completely destroy the instance, without leaking any instance nor file descriptors

Possible Solution (Optional)

The root cause is that Controller.destroy() tries to write a unregister session packet before actually destroying the underlying socket. When the packet can't be written (like when the connection hasn't completed in the first place), the socket doesn't get destroyed.
Some possible solutions:

1. Timing out the disconnection: We could setup a timeout for the callback of the disconnection packet src/enip/index.js#L201, so that, if it doesn't get called in x ms, we'd time out and destroy the socket anyway.
2. Implementing a close() function: The Socket.destroy() function, according to the Node.JS API, is meant to be called when we want to ultimately kill the socket. The Socket.close() is meant to gracefully close the connection. This node could follow the standard having a Controller.close() for gracefully closing the connection (like writing the unregister session packet and sending TCP FIN), and letting Controller.destroy() to forcefully destroy the socket and connection. This has been discussed on Ambiguous resource closing and examples do not close after completion. #53. Downside is breaking the current API.

Context

This issue has surfaced on node-red-contrib-cip-ethernet-ip when connecting to a PLC that is not always online. When the PLC is offline, the above mentioned issue happens, but as the node keeps trying to connect to the PLC (in case it is back online), a lot of file descriptors are leaked, leading to a file descriptor starvation and compromising the whole Node-RED process.

Steps to Reproduce (for bugs only)

1. Create a new instance of Controller, connecting to a dummy IP address (so that it doesn't connect)
2. When the connection fails, destroy the Controller instance and try again.
3. Watch the number of file descriptors being used increase (e.g. by watching /proc/xxxx/fd, where xxxx is the PID of the node.js process)
4. The increase of TCP, TCPWrapper and Controller instances can also be checked by using Chrome's Developer Console.

Your Environment

• Package version (Use npm list - e.g. 1.0.6): 1.2.5
• Node Version (Use node --version - e.g. 9.8.0): 10.15.3
• Operating System and version: Linux 4.19.42-v7+
• Controller Type (eg 1756-L83E/B): irrelevant
• Controller Firmware (eg 30.11): irrelevant

[gfcittolin]

Side note: this issue has been hotfixed on node-red-contrib-cip-ethernet-ip by calling Socket.destroy() externally on the Controller instance. Please see commit #35f28d4