Hey! Was looking through the code and noticed a potential resource leak in the DdpClient constructor.
The Issue
If InetAddress.getByName(hostname) throws UnknownHostException, the DatagramSocket created on the previous line never gets closed:
this.socket = new DatagramSocket(); // socket created
this.targetAddress = InetAddress.getByName(hostname); // if this throws...
// socket is leaked
Suggested Fix
Wrap the constructor in a try-catch that cleans up on failure:
DatagramSocket sock = null;
try {
sock = new DatagramSocket();
this.targetAddress = InetAddress.getByName(hostname);
this.socket = sock;
this.sendPacket = new DatagramPacket(new byte[0], 0, 0, targetAddress, port);
} catch (SocketException e) {
throw new DdpException("Failed to create UDP socket", e);
} catch (UnknownHostException e) {
if (sock != null) sock.close();
throw new DdpException("Invalid hostname: " + hostname, e);
}
Also worth considering: implementing AutoCloseable so consumers can use try-with-resources:
public class DdpClient implements AutoCloseable {
// existing close() method already works
}
Not urgent since this only triggers on bad hostnames, but good to tighten up before a wider release.
Hey! Was looking through the code and noticed a potential resource leak in the
DdpClientconstructor.The Issue
If
InetAddress.getByName(hostname)throwsUnknownHostException, theDatagramSocketcreated on the previous line never gets closed:Suggested Fix
Wrap the constructor in a try-catch that cleans up on failure:
Also worth considering: implementing
AutoCloseableso consumers can use try-with-resources:Not urgent since this only triggers on bad hostnames, but good to tighten up before a wider release.