TCP server in Android to continuously listen to an RFID reader

Question

I have created a TCP server thread which I want to keep running and listening to the IOT device (an RFID reader). I have only one client.

Please review my code and provide your feedback. Are there any edge cases that would make this TCP server fail?

import android.util.Log;
import java.io.DataInputStream;
import java.io.DataOutputStream;
import java.io.IOException;
import java.net.ServerSocket;
import java.net.Socket;

public class TcpServerThread extends Thread {

    private static final String TAG = "TcpServerThread";


    private boolean run_ = true;  
    DataInputStream stream;
    ServerSocket serverSocket;
    Socket clientSocket;


    public TcpServerThread() {
        super();
        start(); //start the thread
    }


    @Override
    public void run() {





        byte[] inBytes = new byte[200];
        int mPort = 1025;

        //creating a socket to listen on a given port
        Log.d(TAG, "PORT OPENED SUCESSFULLY in " + mPort);
        try {
            serverSocket = new ServerSocket(mPort);

        } catch (IOException e) {
            Log.d(TAG, "run: [exception occured in creating the socket]" + e.getLocalizedMessage());
        }


        while (run_) {

            Log.d(TAG, "run() :started");

            try {
                while (serverSocket != null) {
                    Log.d(TAG, "run() : running ");
                    try {
                        //accepting the incoming socket connection request
                        long started = System.currentTimeMillis();
                        clientSocket = serverSocket.accept();
                        clientSocket.setKeepAlive(true);

                        if (clientSocket != null) {
                            long established = (System.currentTimeMillis() - started);
                            Log.d(TAG, "[Connection made at]:" + clientSocket.getRemoteSocketAddress() + "in mills :" + established);

                            DataInputStream stream = new DataInputStream(clientSocket.getInputStream());
                            DataOutputStream outToServer = new DataOutputStream(clientSocket.getOutputStream());

                            while ((input = stream.read(inBytes)) != -1) {
                                Log.d(TAG, "Size read is " + input);
                                Log.d(TAG, "Data is " + (inBytes));
                                outToServer.writeByte(10);

                            }

                        }

                    } catch (IOException e) {
                        Log.d(TAG, "IOexception occured: " + e.getLocalizedMessage());
                        continue;

                    } catch (Exception e) {
                        Log.d(TAG, "EXCEPTION occured: " + e.getLocalizedMessage());
                        continue;
                    }

                }

            } catch (Exception e) {
                //we need to watch out for IllegalStateException cases
                Log.d(TAG, "[Exception occured] :" + e.getLocalizedMessage());

                continue;

            }

        }
        try {
            throw new RuntimeException(TAG + " Crashed!!");
        } catch (RuntimeException e) {
                      Log.d(TAG, "RuntimeException: " + e.getLocalizedMessage());


        }
    }

    public void abort() {
        run_ = false;
        closeServer();
        interrupt();
    }

    public void closeServer() {

        try {
            serverSocket.close();
            clientSocket.close();
            stream.close();
        } catch (Exception e) {
            Log.d(TAG, "closeServer: " + e.getLocalizedMessage());
        } finally {
            start();
        }
    }

Answer 1

You should be implementing Runnable, not extending Thread. You are not creating a new generally-useful extension of thread that can be used as an API class by multiple clients. You just want a normal thread to run some code.

Classes not designed to be extended should be marked final. Variables not designed to be reassigned should be marked final.

None of stream, serverSocket, and clientSocket should be visible outside your class. Only expose methods when necessary. Only expose member variables when really, really necessary.

In idiomatic Java, underscores are only used in constants. In other cases, they should not be used.

Prefer primitives to objects where it's reasonable to do so. There's no reason to give loop_count the overhead of being an object.

loop_count is never read and can be removed.

a is a poor variable name. Variable names should clearly indicate the value they're holding. serverPort would be highly preferable. This change would make your comment noise, and it could be removed.

The two log messages could be joined. Also, you're logging the port as being open before it's open.

In idiomatic java, ALL_CAPS are reserved for constants. Other variables should begin with a lowercase letter, even if they are acronyms. ip is preferable to IP.

If you have an error opening the socket, your code busy-loops forever inside the while (run) block until something interrupts it. If the server can't be started, the run method should return, since your code isn't trying to recover from that failure. This will also let you remove the while (serverSocket != null) line, since it will never be null.

When creating the server socket, you're consuming the stack trace. You should always log stack trace. It would also be nice if your message logged the ip and port that could not be opened.

If this code is interrupted or an exception occurs, your open resources will not be closed. Prefer the use of try-with-resources blocks to ensure resources are closed.

Logging the start of every time through the loop is going to result in a lot of meaningless log messages.

The first try block inside the while (run) block does nothing and can be removed.

You dereference clientSocket immediately before you check if it's null. If it could ever be null (it can't), you'd get a NullPointerException before the null check.

Variables should be declared to minimize their scope where possible. This makes code more readable and allows for more aggressive reclamation of memory.

10 is a magic number in your code. Declare a constant whose name explains what the logical meaning of 10 is and use that in your code, so a reader understands the intent, not just that a 10 is going over the wire.

It is a bad practice to catch Exception. You are inadvertently handling any checked or unchecked exceptions that may be added in the future. If you don't know what to do with a specific exception, let it percolate up the stack. Likewise, catching RuntimeException is generally ill advised.

You have a try block that's always throwing an exception. That should obviously be removed.

The stream instance variable is never used, but then closed in the closeServer method. You create stream instances inside run and then don't close them.

The proper term is "millis", not "mills".

It would be nice to extract some of your code into a separate method so run looks less like a giant >.

With proper use of try-with-resources blocks, the closeServer method is not required.

abort should not be firing interrupt(). Include checks inside your run method to see if the thread has been interrupted, and halt gracefully if it has.

If you were to make all these changes, your code might look more like:

public class TcpServer implements Runnable {

    private static final String TAG = "TcpServerThread";
    private static final byte RECEIVED_MESSAGE_PART = 10;

    private boolean running = true;


    public TcpServer() {
        super();
    }


    @Override
    public void run() {
        final int port = 1025;
        final String ip = MiscUtilities.getIpAccess();

        try (final ServerSocket serverSocket = new ServerSocket(port)) {
            Log.d(TAG, "Server started, accepting connections to " + ip + ":" + port);

            while (running && !Thread.currentThread().isInterrupted()) {
                final long started = System.currentTimeMillis();
                try (final Socket clientSocket = serverSocket.accept()) {
                    clientSocket.setKeepAlive(true);

                    final long established = (System.currentTimeMillis() - started);
                    Log.d(TAG, "[Connection made at]:" + clientSocket.getRemoteSocketAddress() + " in mills :" + established);

                    this.handleData(clientSocket);

                } catch (final IOException e) {
                    Log.d(TAG, "Failure handling inbound connection to " + ip + ":" + port, e);
                    continue;
                }
            }
        } catch (final IOException e) {
            Log.d(TAG, "Unable to open socket " + ip + ":" + port, e);
            return;
        }
    }

    public void abort() {
        this.running = false;
    }

    private void handleData(final Socket clientSocket) throws IOException {
        try (final DataInputStream stream = new DataInputStream(clientSocket.getInputStream());
                final DataOutputStream outToServer = new DataOutputStream(clientSocket.getOutputStream())) {

            final byte[] inBytes = new byte[200];
            int input = 0;
            while ((input = stream.read(inBytes)) != -1) {
                Log.d(TAG, "Size read in bytes: " + input);
                Log.d(TAG, "Data is: " + MiscUtilities.bytesToHex(inBytes));

                outToServer.writeByte(RECEIVED_MESSAGE_PART);
            }
        }

    }

}