Return to Answer

• 1

• 1.4k
• 8
• 15

Resource leaks

You isConnected method has the potential to leak open resources. If the method is called while isConnected() on the underlying socket returns false, the socket will be set to null without calling close() on the socket.

public boolean isConnected() {
 if (socket == null){
 return false;
 }
 else if (socket.isConnected()){
 return true;
 }
 else {
 try {
 socket.close();
 } catch (IOException ignored) {
 }
 socket = null;
 return false;
 }
}

Remember that the close() explicitly states that the method must be idempotent, so closing a socket 2 times shouldn't give you a error.

Inconsistent order of modifers

private final Queue<byte[]> queue;
final private SocketIO io;

Choosing a consistent order of modifers makes your code looks better, I would go for private final, as this is used everywhere in your project

private final Queue<byte[]> queue;
private final SocketIO io;

Swallowing InterruptedException

 try {
 connection.join();
 } catch (InterruptedException e) {
 e.printStackTrace();
 }

Swallowing/suppressing this exception isn't good practise, as it blocks the propagation of the interrupted state. If you are forced to catch it, you should either make a looping structure that set the threads interrupted state at the end, or set the threads intterupted state directly by calling Thread.currentThread().interrupt();

 try {
 connection.join();
 } catch (InterruptedException e) {
 Thread.currentThread().interrupt();
 }

Unused thread

private Thread connection = new Thread();

By defining a Thread that is never started, you have the potential to create a memory leak.

You should use either use a null object, or a wrapper object that you crafted using the null-object design pattern.

private Thread connection = null;

A other way to this, is to start the thread directly. This is considered more as a hack than a proper solution.

private Thread connection = new Thread();
{
 connection.start();
}

Sleeping Threads

You use a large number of call to Thread.sleep() in your code, this is more like a anti-pattern for proper usage of locking. In the ideal world, you should let the threads wait for each other using Object.wait() in combination with Object.notifyAll(). Incase of your socket, don't sleep, but call the read() method again. Using this mechanism you can signal multiple conditions, like a new thing to read, but also if there is nothing left to write.

Large number of new Threads

Do you really need to create and destroy that number of Threads? a better implementation might call the methods of Executors to create its tasks, as it has automatic Thread management, the newCachedThreadPool has the performance characteris you require, automatic new threads when needed, but reusing old Threads when they are free.

Unbuffered IO

public void connect(final String ip, final int port) throws IOException {
 close();
 socket = new Socket(ip, port);
 input = socket.getInputStream();
 output = socket.getOutputStream();
}

You mainly use unbuffered io streams to the socket, this is really expansive as the calls propagate down the networking stack of the operating system. Wrapping the streams in BufferedInputStream and BufferedOutputStream makes the calls quicker as there are less half filled packets.

public void connect(final String ip, final int port) throws IOException {
 close();
 socket = new Socket(ip, port);
 input = new BufferedInputStream(socket.getInputStream());
 output = new BufferedOutputStream(socket.getOutputStream());
}

To many flushes

 if (io.isConnected()){
 while (queue.size() > 0) {
 final byte[] sendBytes = queue.remove();
 io.write(ByteBuffer.allocate(4).putInt(sendBytes.length).array());
 io.write(sendBytes);
 io.flush();
 }
 }

You flushing after every packet, by placing the final flush call after the loop, and wrapping everything into a if-statement that checks the size, you can send more packets in just one flush() call, this means only 1 tcp packet even if you send 5 small application specific packets

 if (io.isConnected()){
 if(!queue.isEmpty()) {
 do {
 final byte[] sendBytes = queue.remove();
 io.write(ByteBuffer.allocate(4).putInt(sendBytes.length).array());
 io.write(sendBytes);
 } while (queue.size() > 0);
 io.flush();
 }
 }