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();
}
