Okay, since you asked for it…
public int receivemessage(Socket client) {
int amountBytes = 0;
The following try/catch block does nothing like the comment states
// This try catch is to trick the socket into being non-blocking when checking for data
try {
InputStream stream = client.getInputStream();
amountBytes = stream.available();
this.in = new DataInputStream(stream);
Using a DataInputStream directly on top of a socket InputStream will lead to many single-byte reads from the socket, which means we visit kernel space for every byte we read. You most likely intended to use a BufferedInputStream.
} catch (Exception localException) {
//
You just threw away the cause of a crash, ignoring it, and moving on… this aint PHP.
}
// I there's incoming data, then read it
int msg = 0;
if (amountBytes > 0) {
try {
msg = this.in.read();
‘msg’ holds an integer between -1 and 0xFF, let’s ignore -1, because… there were bytes available, right? A bigger problem is that your packets can be at most 255 bytes long, that’s tiny, that’s even smaller than the usual size of IP packets.
this.buffer.in.clear();
this.buffer.inpointer = 0;
for (int i = 0; i < msg + 1; i++) {
int read = this.in.read();
–> nasty bug <–
That there are ‘amountBytes>0’ bytes available, doesn’t mean we can assume ‘amountBytes>=msg+1’, so the ‘read’ operation can start returning -1 once the buffer is depleted. Any actual data you read from the socket from that point on, will not be adhering to your protocol, as you’re reading bytes from the remainder of a previously received packet. That you sent X bytes, does not mean you are guaranteed to be able to read X bytes in one go, on the other end. Read up on TCP packet fragmentation.
Yet again, we’re doing 1 kernel space operation per byte we read from the socket buffer, this can bring even a fast server to its knees with a handful of connections.
if (i > 0)
You just discarded the first byte of the packet - as if it’s meaningless, just filler.
this.buffer.append(read);
You seem to store 8 bits worth of data as an 32 bit integer.
}
}
catch (Exception localException1) {
Let’s ignore anything that could go wrong, like a remote endpoint closing the connection, it’s better not to know, as then you don’t have to handle it either…
}
}
this.in = null;
You just nullified a field that was assigned at the start of the method. This should be a local variable.
return msg;
}
public void sendmessage(Socket socket) {
try {
if (socket != null) {
You are silently failing when somebody tried to send data to a null-socket.
OutputStream outst = socket.getOutputStream();
BufferedOutputStream bos = new BufferedOutputStream(outst);
if (bos != null) {
How can ‘bos’ possibly be null here?
bos.write(this.buffer.out.size());
Silent failure (!) when the packet size is over 255.
bos.write(0);
Here we have this meaningless byte, that we discard on the other end… why oh why…
for (int i = 0; i < this.buffer.out.size(); i++) {
int bytesend = ((Integer)this.buffer.out.get(i)).intValue();
Every outbound byte is held as an Integer object… or is this an attempt at a cast to int ?!
bos.write(bytesend);
}
this.buffer.in.clear();
bos.flush();
}
} else {
this.buffer.in.clear();
}
} catch (Exception e) {
//
Blindfolds and earplugs galore.
}
}
The general API seems totally off: we compose a packet, and send it to an arbitrary Socket. Maybe you’re broadcasting an identical packet to N clients, but generally a packet is composed for a specific client, so passing in an abitrary Socket instance, and sending that packet to it, is the wrong way around. Completely discarding all bytes in the packet, the moment this arbitrary Socket happens to be null, is just odd.
Another oddity:
receivemessage(Socket) is non-blocking
sendmessage(Socket) is blocking!
