0
\$\begingroup\$

This is a code for an android thread. This is a tcp server and gets data from socket, maps it to an object and sends that object back to the main service. If there is anything I'm doing wrong or if something can be done better, I would really appreciate comments.

public class SocketManager extends Thread {

    Handler mHandlerThread;
    private final int MAX_MESSAGE_LENGTH = 2048;
    private final int PORT = 2500;
    private ServerSocket serverSocket = null;
    private Socket socket = null;
    private DataInputStream in = null;
    private DataOutputStream out = null;
    private GPSFields gps;
    private ObjectMapper mObjectMapper;
    private final String APPNAME = "My App";

    public SocketManager(Handler mHandlerThread) {
        this.mHandlerThread = mHandlerThread;
    }

    @Override
    public void run() {

        int messageLength = 0;
        gps = new GPSFields();
        byte buffer[] = new byte[MAX_MESSAGE_LENGTH];
        mObjectMapper = new ObjectMapper();
        mObjectMapper.configure(DeserializationFeature.FAIL_ON_UNKNOWN_PROPERTIES, false);

        try {
            openSocket();
        } catch(Exception e) {
            Log.d(APPNAME, e.toString());
        }

        while(true) {
            try {
                if(socket.isClosed()) {
                    openSocket();
                }
                else {
                    messageLength = in.readShort();
                    if (messageLength > 0 && messageLength < MAX_MESSAGE_LENGTH) {
                        in.read(buffer, 0, messageLength);
                        gps = mObjectMapper.readValue(new String(buffer), GPSFields.class);
                        if (gps != null) {
                            out.write(1);
                            Message message = new Message();
                            message.obj = gps;
                            mHandlerThread.sendMessage(message);
                            Log.d(APPNAME, "Received Latitude: " + gps.getLatitude() + " Longitude: " + gps.getLongitude());
                        } else {
                            out.write(0);
                        }
                    } else {
                        out.write(0);
                    }
                }
            } catch (Exception e) {
                closeSocket();
                Log.d(APPNAME, e.toString());
            }
        }
    }

    private void closeSocket() {
        try {
            socket.close();
            serverSocket.close();
        }catch (Exception e){
            Log.d(APPNAME, e.toString());
        }
    }

    private void openSocket() {
        try {
            serverSocket = new ServerSocket(PORT);
            socket = new Socket();
            socket = serverSocket.accept();
            in = new DataInputStream(socket.getInputStream());
            out = new DataOutputStream(socket.getOutputStream());
        }catch (Exception e){
            Log.d(APPNAME, e.toString());
        }
    }
}
\$\endgroup\$
  • 1
    \$\begingroup\$ Welcome to Code Review! The current question title, which states your concerns about the code, is too general to be useful here. Please edit to the site standard, which is for the title to simply state the task accomplished by the code. Please see How to get the best value out of Code Review: Asking Questions for guidance on writing good question titles. \$\endgroup\$ – Toby Speight Jan 23 at 16:15
  • \$\begingroup\$ What's ObjectMapper? Please provide more context for this code. \$\endgroup\$ – 200_success Jan 23 at 17:36
0
\$\begingroup\$
  • When messageLength happens to be too large the code just loops around to the next messageLength = in.readShort();. However, the message body is still there, and readShort will pull the short from it. Chances are, it would be interpreted as a valid message length, and the stream gets irrecoverably out of sync.

    You must discard the entire message by reading it.

  • DataInputStream.read() hey return less bytes than requested. Yet again, the stream gets out of sync. You must read the message body in the loop, until it is consumed entirely.

  • Closing the server socket every time a data socket fails seems drastic. There is nothing wrong with the server socket, and could be reused. It may need to be reopened only if accept fails.

  • I recommend to move openSocket call to the exception clause:

        while (True) {
            try {
                messageLength = in.readShort();
                ....
            } catch(Exception e) {
                closeSocket();
                Log.d(APPNAME, e.toString());
                openSocket();
            }
    

    One nesting level down.

  • You may want to be more precise about exceptions. It is very well possible that the exception has nothing to do with the socket (e.g. may sendMessage throw? was it a keyboard interrupt? ObjectMapper throw? something else? It is impossible to tell from the given code). In such cases different recovery is necessary, but the socket should likely remain intact.

\$\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.