I have an inner class that implement runnable and it makes HTTP requests and then calls the function of outer class to handle the response. The intended behavior is to append all the responses to a List of Objects.
Sample code:
public class Status {
private ExecutorService executor;
CloseableHttpClient httpClient;
List<String> results = new LinkedList<>();
public Status() {
executor = Executors.newFixedThreadPool(5);
httpClient = HttpClients.createDefault();
}
public handleInput(Entity entity) {
String result = EntityUtils.toString(httpEntity);
results.add(result);
}
private class Action implements Runnable {
@Override
public void run() {
try {
//do some http calls
// response = httpClient.execute(httpPost);
handleInput(response.getEntity())
} catch (SomeException e) {
lastResult = false;
}
}
}}
During my testing, I didn't face any issue but I would like to know if it is a thread safe operation to add results from multiple threads to the same linkedlist and if not what is the best design for such scenarios.
Not thread-safe
No, manipulating a non-thread-safe
Listfrom across threads is not thread-safe.SynchronizedYou could make your
handleInputmethodsynchronizedas commented by Bodewes. Of course, that method becomes a potential bottleneck, as only one thread at a time can be calling a synchronized method. Perhaps not an issue in your case, but be aware.Thread-safe collection
Another option is to replace your
LinkedListwith a thread-safe collection. For example,CopyOnWriteArrayList,CopyOnWriteArraySet, orConcurrentSkipListSet.Callable&FutureBut I suggest you not mingle your multi-threaded production of
Entityobjects with collecting and processing those objects. Each task assigned to a background thread should “mind its own business” as much as possible. Having the tasks share a list is entangling them across threads needlessly; that kind of entangling (shared resources) should be avoided wherever possible.Change your task from being a
Runnableto be aCallableso as to return a value. That return value will be eachEntityproduced.As you submit each
Callableto the executor service, you get back aFutureobject. Collect those objects. Through each of those objects you can access the result of each task’s work.Wait for the executor service to finish all submitted tasks. Then inspect the results of each
Future.By using
Futureobjects to collect the results produced by the background threads, and only processing those results after they are all done, we have eliminated the need to make yourresultscollection thread-safe. The original thread collects those results into a list, rather than each thread adding to the list.Notice how in this example code below we have no resources shared across the tasks running in background threads. Each task does its own thing, reporting its own result via its particular
Futureobject. The tasks no longer access a sharedList.By the way, notice that this example code does not keep the executor service in a member field as does the code in the Question. In our situation here, an executor service should be (1) instantiated, (2) utilized, and (3) shut down all in one action. You must shut down your executor service when no longer needed (or when your app exits). Otherwise its backing thread pool may run indefinitely, like a zombie ♂️.
Example code
When run.