2
\$\begingroup\$

I'm writing a tiny networking library for learning purposes and personal use and I'm not sure if my approach to handling reading and writing exceptions and synchronization is correct. Feel free to criticize my code harshly as I want to learn as much as possible.

Reading packets:

@Override
public Packet readPacket() throws NetworkException {
    if (!isClosed()) {
        try {
            synchronized (this) {
                int identifier = in.readInt();
                int length = in.readInt();
                if (length > 0) {
                    byte[] bytes = new byte[length];
                    in.readFully(bytes, 0, bytes.length);
                    switch (identifier) {
                        case PacketConstants.TEXT_PACKET:
                            return new TextPacket(bytes);
                    }
                }
            }
        } catch (SocketException e) {
            close();
            return null;
        } catch (IOException e) {
            throw new NetworkException("Could not read from input stream.", e);
        }
    }
    return null;
}

Writing packets:

@Override
public void sendPacket(Packet packet) throws NetworkException {
    if (!isClosed()) {
        try {
            synchronized (this) {
                out.writeInt(packet.getIdentifier());
                out.writeInt(packet.getBytes().length);
                out.write(packet.getBytes());
                out.flush();
            }
        } catch (SocketException e) {
            close();
        } catch (IOException e) {
            throw new NetworkException("Could not write to output stream.", e);
        }
    }
}

Additionally, I would like to ask if the following two methods can be improved or if they are good as they are.

Closing connection:

@Override
public void close() {
    if (out != null) {
        // TODO: Send disconnection packet to the other party.
        try {
            out.close();
        } catch (IOException e) {
            e.printStackTrace();
        } finally {
            socket = null;
            in = null;
            out = null;
            listener = null;
        }
    }
}

Checking if connection is closed:

@Override
public boolean isClosed() {
    return socket != null && socket.isClosed();
}

Updated code: I changed a few things because it makes it better, at least in my opinion. Formatting of the class is a bit off, not sure why.

public class DefaultConnection implements Connection {
private EventListenerList listeners = new EventListenerList();

private Socket socket;
private DataInputStream in;
private DataOutputStream out;

protected void processInternalPacket(InternalPacket packet) {
    String command = packet.getObject();
    if (command.equals("connect")) {
        try {
            sendPacket(InternalPacket.newConnectedPacket());
        } catch (NetworkException e) {
            e.printStackTrace();
        }
    } else if (command.equals("connected")) {
        ConnectionEvent event = new ConnectionEvent(this);
        for (ConnectionListener listener : listeners.getListeners(ConnectionListener.class)) {
            listener.connected(event);
        }
    } else if (command.equals("disconnect")) {
        try {
            disconnect();
        } catch (NetworkException e) {
            e.printStackTrace();
        }
    }
}

@Override
public void addConnectionListener(ConnectionListener listener) {
    if (listener == null) {
        throw new IllegalArgumentException("listener == null");
    }

    listeners.add(ConnectionListener.class, listener);
}

@Override
public void removeConnectionListener(ConnectionListener listener) {
    if (listener == null) {
        throw new IllegalArgumentException("listener == null");
    }

    listeners.remove(ConnectionListener.class, listener);
}

@Override
public void connect(Socket socket) throws NetworkException {
    if (socket == null) {
        throw new IllegalArgumentException("socket == null");
    }

    this.socket = socket;
    try {
        in = new DataInputStream(socket.getInputStream());
    } catch (IOException e) {
        throw new NetworkException("Cannot access input stream.", e);
    }
    try {
        out = new DataOutputStream(socket.getOutputStream());
        out.flush();
    } catch (IOException e) {
        throw new NetworkException("Cannot access output stream.", e);
    }

    new Thread(() -> {
        while (isConnected()) {
            try {
                Packet packet = readPacket();
                if (packet != null) {
                    if (packet instanceof InternalPacket) {
                        processInternalPacket((InternalPacket) packet);
                    } else {
                        PacketEvent event = new PacketEvent(this, packet);
                        for (ConnectionListener listener : listeners.getListeners(ConnectionListener.class)) {
                            listener.packetReceived(event);
                        }
                    }
                }
            } catch (NetworkException e) {
                e.printStackTrace();
                try {
                    disconnect();
                } catch (NetworkException e1) {
                    e1.printStackTrace();
                }
            }
        }
    }).start();

    sendPacket(InternalPacket.newConnectionPacket());
}

@Override
public void disconnect() throws NetworkException {
    if (socket == null) {
        return;
    }

    try {
        synchronized (this) {
            Packet packet = InternalPacket.newDisconnectionPacket();
            out.writeInt(packet.getIdentifier());
            out.writeInt(packet.getBytes().length);
            out.write(packet.getBytes());
            out.flush();
        }
    } catch (IOException e) {
        e.printStackTrace();
    }

    try {
        out.close();
    } catch (IOException e) {
        throw new NetworkException("Error while closing connection.", e);
    } finally {
        socket = null;
        ConnectionEvent event = new ConnectionEvent(this);
        for (ConnectionListener listener : listeners.getListeners(ConnectionListener.class)) {
            listener.disconnected(event);
        }
    }
}

@Override
public Packet readPacket() throws NetworkException {
    try {
        synchronized (this) {
            int identifier = in.readInt();
            int length = in.readInt();
            if (length > 0) {
                byte[] bytes = new byte[length];
                in.readFully(bytes, 0, bytes.length);
                switch (identifier) {
                    case PacketIdentifier.INTERNAL_PACKET:
                        return new InternalPacket(bytes);
                    case PacketIdentifier.TEXT_PACKET:
                        return new TextPacket(bytes);
                }
            }
        }
    } catch (SocketException e) {
        throw new NetworkException("Cannot access input stream.", e);
    } catch (IOException e) {
        throw new NetworkException("Reading from input stream failed.", e);
    }
    return null;
}

@Override
public void sendPacket(Packet packet) throws NetworkException {
    try {
        out.writeInt(packet.getIdentifier());
        out.writeInt(packet.getBytes().length);
        out.write(packet.getBytes());
        out.flush();
    } catch (SocketException e) {
        throw new NetworkException("Cannot access output stream.", e);
    } catch (IOException e) {
        throw new NetworkException("Writing to output stream failed.", e);
    }
}

@Override
public boolean isConnected() {
    return socket != null && !socket.isClosed();
}

}

And to make it less confusing, here is the packet interface.

public interface Packet {
    int getIdentifier();

    byte[] getBytes();

    Object getObject();
}
\$\endgroup\$
  • \$\begingroup\$ If socket is null, then isClosed will return false. Is this expected behaivour? \$\endgroup\$ – JD9999 Aug 9 '16 at 5:42
  • \$\begingroup\$ Yes. I thought about using integer or boolean for the state of the connection, but I decided to make socket null when 'close()' is called. I'm not exactly sure if making it null is needed, but in case 'out.close()' fails, I guess. In your opinion, should I change it? \$\endgroup\$ – Vinski Aug 10 '16 at 9:04
1
\$\begingroup\$

The isClosed() will only work if the socket is NOT null. Otherwise it will always return false, and therefore your application will continue writing to a null socket.

We want to check if the socket is closed or if its null, because in either states we don't want packets sent across. So your isClosed() will look like this:

@Override
public boolean isClosed() {
    return socket == null || socket.isClosed();
}
\$\endgroup\$
  • \$\begingroup\$ I'm not sure how I missed this. I wondered why were exceptions related to reading and writing through closed/null socket thrown, but I just kept focusing on server and client classes. Thank you! (By the way, I voted up but it says it cannot be shown publicly due to me not having reputation of 15.) Can you give me feedback on synchronization? Am I doing it correctly? \$\endgroup\$ – Vinski Aug 11 '16 at 9:04
  • \$\begingroup\$ Your synchronisation is OK. \$\endgroup\$ – JD9999 Aug 13 '16 at 0:14
  • \$\begingroup\$ Something was wrong with it, but I managed to fix it. I will update my post tomorrow or something. I'm fixing stuff at the moment and don't have a lot of time. Thank you for the feedback. :) \$\endgroup\$ – Vinski Aug 13 '16 at 19:27
-1
\$\begingroup\$

Use 'finally' for closing I/O streams in readPacket method. This way, you will release resourses whatever happens

\$\endgroup\$

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

Not the answer you're looking for? Browse other questions tagged or ask your own question.