Thread Safe Server Querier

Question

I wanted to make a class that connects to my server and returns a value, a URL in this case. As Android 4.0 doesn't let me run network task on the main UI thread (rightly so) I have made this.

Please give me your feedback as to if it is thread safe or not and ways to improve it. I have never used ExecutorService or Future before so I would be grateful to know if I am using it correctly. I would also like to know if this is blocking the thread that called getDownloadURL.

public class DownloadURLServerGetter implements DownloadURLGetter {
 private ServerAdapter serverAdapter;
 private String id;
 @Inject
 public DownloadURLServerGetter(ServerAdapter serverAdapter) {
 this.serverAdapter = serverAdapter;
 }
 @Override
 public synchronized String getDownloadURL(String id) throws IOException {
 this.id = id;
 final ExecutorService service;
 final Future<String> task;
 service = Executors.newFixedThreadPool(1);
 task = service.submit(new MyCallable());
 try {
 return task.get();
 } catch (InterruptedException e) {
 throw new IOException(e);
 } catch (ExecutionException e) {
 throw new IOException(e);
 }
 }
 public class MyCallable implements Callable<String> {
 @Override
 public String call() throws Exception {
 HttpResponse resp = serverAdapter.get("blobs/" + id);
 String url = EntityUtils.toString(resp.getEntity());
 return Parser.parseURL(url);
 }
 }
}

Answer 1

Your method seems thread safe, but you are blocking getDownloadURL() because task.get() will only return once the underlying job finishes. Try reading up on Java's future and see how you can use them more efficient and correct.

Your executor service should most likely only be created once for your application.

By the way: if you pass ServerAdapter and id to MyCallable (as constructor arguments) you no longer need to keep getDownloadURL synchronized or MyCallable non-static.

• jiduvah: thank you for that. I realised I was blocking the calling thread. I have actually modified the thread that was calling this method so it seems to work pretty well at the moment tho as you pointed out there is room for improvement.