5
\$\begingroup\$

My code communicates with a Raspberry Pi (with a static IP address) running on a network. The client sends a signal to the Pi, and the Pi sends back a double (encoded as a string) with the angle correction needed for another routine of the code (the Pi does some image processing, but that doesn't matter).

The main execution loop needs to constantly be running with no breaks, so I use multithreading to handle the execution. The result is packaged in a Future<Double> so other code can check if the Pi sent back the result yet. If there is a problem with communicating, or the Pi isn't present, the Future contains a NaN.

import java.io.*;
import java.net.*;
import java.util.concurrent.*;

public class TCPClient {

    private final String pingMessage = "A\n";
    private final int timeOut = 250;// ms

    private ScheduledThreadPoolExecutor tcpPool = new ScheduledThreadPoolExecutor(5); 
    private Future<ClientWorker> worker; //Will hold the tcpclient when it is created

    public TCPClient() {
        worker = tcpPool.submit(() -> new ClientWorker("10.8.10.44", 5805)); //Create worker
    }

    class ClientWorker {
        private Socket sock;
        private BufferedReader reader;
        private DataOutputStream output;

        public String readLine() throws IOException {
            return reader.readLine();
        }

        public void outputCommand(String line) throws IOException {
            output.writeBytes(line);
        }

        public ClientWorker(String IP, int port) {
            boolean created = false;
            //Keep trying until created
            while (!created) {

                try {

                    sock = new Socket(IP, port);
                    sock.setSoTimeout(timeOut);
                    System.out.println("Socket created");

                    reader = new BufferedReader(new InputStreamReader(sock.getInputStream()));
                    System.out.println("Buffered Reader created");
                    output = new DataOutputStream(sock.getOutputStream());

                    System.out.println("DataOutputStream created");
                    created = true;

                } catch (Exception e) {
                    //Try again in a second
                    try {
                        Thread.sleep(1000);
                    } catch (InterruptedException e1) {
                        //This shouldn't happen
                        e1.printStackTrace();
                    }
                }
            }
        }
    }

    public Future<Double> getAngleCorrection() {
        if (worker.isDone())
            try {
                return tcpPool.submit(new AngleCorrectionTask(worker.get()));
            } catch (InterruptedException | ExecutionException e) {
                e.printStackTrace();
            }

        return tcpPool.submit(() -> Double.NaN);
    }

    public class AngleCorrectionTask implements Callable<Double> {
        ClientWorker worker;

        public AngleCorrectionTask(ClientWorker worker) {
            this.worker = worker;
        }

        @Override
        public Double call() {
            try {
                worker.outputCommand(pingMessage);
                String result = worker.readLine();

                Double d = Double.parseDouble(result);

                return d;
            } catch (IOException | NumberFormatException | NullPointerException e) {
                // Timeout
                return Double.NaN;
            }
        }
    }
}

In the init code, an instance of TCPClient is created:

TCPClient tcpc = new TCPClient();

When the angle correction is needed, I call:

Future<Double> angleCorrection = tcpc.getAngleCorrection();

Each time through the main loop, the program then checks angleCorrection.isDone().

(The code will only call getAngleCorrection if there is no active AngleCorrectionTask. The Future<Double> is cancelled if it is no longer needed. If the code tries to get the angle correction while it's already in progress, a separate layer of abstraction cancels the Future<Double> and immediately calls getAngleCorrection()).

\$\endgroup\$
3
\$\begingroup\$
private final String pingMessage = "A\n";
private final int timeOut = 250;// ms

I don't expect these change anytime soon or that they can be different between different instances of your TCPClient. Make them static.


private ScheduledThreadPoolExecutor tcpPool = new ScheduledThreadPoolExecutor(5);

I'd expect:

 private final ScheduledExecutorService tcpPool = Executors.newScheduledThreadPool(5);

This has a few minor benefits: You program against the interface and can change the used implementation by virtue of changing a single method call. Also it makes abundantly clear that you're using the standard ScheduledExecutorService assumptions and don't mess around with internal state.

You don't reassign this (so I made it final) and also don't use the submit-overloads that are provided for the Scheduled-part of the ExecutorService. You should be able to just use a FixedThreadPool as follows:

private final ExecutorService tcpPool = Executors.newFixedThreadPool(5);

class ClientWorker {
    private Socket sock;
    private BufferedReader reader;
    private DataOutputStream output;

there's something wrong here. You encapsulate a number of Resources. You never dispose of them, nor call close. You should fix that:

class ClientWorker implements AutoCloseable {
    // ...
    @Override
    public void close() {
        output.close();
        reader.close();
        sock.close();
    }
}

This should address possible memory leaks. Please make sure you close all these independent of whether closing succeeds or fails.


            } catch (Exception e) {
                //Try again in a second
                try {
                    Thread.sleep(1000);
                } catch (InterruptedException e1) {
                    //This shouldn't happen
                    e1.printStackTrace();
                }
            }

A few things here...
catch (Exception e) is generally a bad idea, because it's too unspecific. You should catch IOException here. There's things that you'll catch here, that won't change even after waiting a second.
This includes: SecurityException, IllegalArgumentException, NullPointerException and a handful of Runtime exceptions that could occur.

Additionally making your thread sleep for a whole second (and incorrectly assuming it cannot be interrupted during that time) is wrong on so many levels.

You're basically assuming, nobody cancels your task when you try to create the connection for the 20th time. Additionally you make the assumption that the caller actually wants to retry in the first place instead of throwing an Exception or trying a different mechanism.
This makes this problematic to use. Also you could just have used your ScheduledExecutorService to run the same task again in a second, if creating the necessary connections failed.


Last but not least I'm left wondering why you encapsulated the ClientWorker functionality into a separate class instead of just implementing the AngleCorrectionTask with the contents of that class. As long as you're using the ClientWorker somewhere else that's just fine, but you shouldn't use an inner class somewhere else. Instead it should be it's own class.

\$\endgroup\$
  • \$\begingroup\$ For the ClientWorker, I was planning on adding more tasks, but ended up only needing the AngleCorrectionTask. In hindsight, I probably should have created an abstract NetworkTask class and had AngleCorrectionTask extend it. \$\endgroup\$ – Daniel M. Apr 23 '16 at 22:12
  • 2
    \$\begingroup\$ You can still do that now :) Nothing's stopping you from changing your application's inner workings. If it's better, why not do it? \$\endgroup\$ – Vogel612 Apr 23 '16 at 22:13
  • \$\begingroup\$ Good point! The suggestions gave me a few other ideas. I'll probably handle exceptions from the creation of the streams a bit farther up the stack. \$\endgroup\$ – Daniel M. Apr 23 '16 at 22:27

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.