# 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<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 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 DataOutputStream output;

public String readLine() throws IOException {
}

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");

output = new DataOutputStream(sock.getOutputStream());

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

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

public Future<Double> getAngleCorrection() {
if (worker.isDone())
try {
} catch (InterruptedException | ExecutionException e) {
e.printStackTrace();
}

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

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

this.worker = worker;
}

@Override
public Double call() {
try {
worker.outputCommand(pingMessage);

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()).

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 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();
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 {
} 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.

• 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. Apr 23 '16 at 22:12
• 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? Apr 23 '16 at 22:13
• 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. Apr 23 '16 at 22:27