Fetch data from co-processor on network in background

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 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 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(so

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.

StackExchange Code Review Q#123355, answer score: 3