0

I have a Spring Boot Application.

My application sends requests to another applications using restTemplate.

I need to send a request to one hundred different applications(on different servers). I use:

publi class Service {
    private RestClient restClient;
    private List<String> urls;
    private ThreadPoolExecutor executor;

    public Service(RestClient restClient, List<String> urls, ThreadPoolExecutor executor){
       this.restClient = restClient;
       this.urls = urls;
       this.executor = executor;
    }

    public void sendPost(Entity entity){
         for (String url: urls){
             executor.execute(() -> restClient.create(url, entity);
         }
    }

}

I am trying to use ThreadPoolExecutor(fixedSizeThreadPool) but I have some questions.

1.I read up that threadPoolExecutor is thread-safe. Does It means that I can invoke execute() at the same time from different threads and It will work properly?
2. If there are no idle threads in threadPoolExecutor It will slow down the application and I should to choose rational pool size, right?
3. For example, I need to write executed urls in ArrayList: 

public void sendPost(Entity entity){
         List<String> executedUrls = new ArrayList<>();  
         for (String url: urls){
             executor.execute(() -> restClient.create(url, entity, executedUrls);
         }
}

RestClient sends a request and if It is successfully executed It will be added in ArrayList.

I expect that ArrayList will have list of successfully executed urls If I have an exception in any thread from threadPool.
Will It work as I expect or I can have something like lost update?

Donatello
  • 353
  • 6
  • 19
  • 1
    Yes, you should use a rational upper bound for thread count. You should be fine with more than cpu cores because it's i/o work. No, it won't work as you expect in all cases because the executor might be thread safe but an `ArrayList` isn't, use a concurrent list. – daniu Mar 20 '19 at 20:37
  • 1
    What is that `RestClient`? `ThreadPoolExecutor` is "designed" for threads, but everything (what happens) inside `create(...)` *should consider thread-safety* /is not automatically thread safe! ... and "concurrency over IO": ...is possible, but not desirable! ..you should (re-)consider your architecture and "deployment structure". 1. yes 2. yea...an important setting (see "concurrency over IO") 3. `ArrayList` is *not thread safe* (, which means: "you cannot rely on its content/size/..." in concurrency). – xerx593 Mar 20 '19 at 20:54
  • @xerx593 RestClient is responsible for creation HttpHeaders and then HttpEntity for sending requests with restTemplate. – Donatello Mar 20 '19 at 21:04
  • @xerx593 My application has replicas. All replicas must have identical data. Any application must execute the request. I thought that I could use ThreadPoolExecutor to send the request to replicas and after execute It locally. It looks better than sending the request in foreach, am I not right? – Donatello Mar 20 '19 at 21:14
  • 1
    ..so a "custom class"!? yes, please see comments on `create(..)`: you *should* synchronize! (...not the complete method, that would be analogous to sequential processing (with random order), only, and this is the art:)... the "sensible parts": where I would merely include things like: `list.contains()` or `list.add()` ...[`restTemplate` is "considered thread safe" here at SO](https://stackoverflow.com/q/22989500/592355), but ["google" contradicts](http://tiemensfamily.com/TimOnCS/2017/08/06/is-spring-resttemplate-thread-safe/).. – xerx593 Mar 20 '19 at 21:15
  • 1
    ..your "deployment structure" in mind: 1. "data base" sounds like a good place for "`executedUrls`"!(?) ...and it sounds like you also *can better* parallelize: share the job (additionally) over the replicas! – xerx593 Mar 20 '19 at 21:23

1 Answers1

1

What you can do is use an ExecutorService.
For example, create a new ExecutorService (a cached ThreadPoolExecutor might be the better one)

private final ExecutorService executorService = Executors.newCachedThreadPool();

Create a custom Runnable implementation

static class RestRunnable implements Runnable {
    private final String url;
    private final RestTemplate restTemplate;
    private final Collection<? super String> executedUrls;

    RestRunnable(
            final String url,
            final RestTemplate restTemplate,
            final Collection<? super String> executedUrls) {
        this.url = url;
        this.restTemplate = restTemplate;
        this.executedUrls = executedUrls;
    }

    @Override
    public void run() {
        final ResponseEntity<?> response = restTemplate.exchange(...);

        if (response.getStatusCode() == HttpStatus.OK) {
            executedUrls.add(url);
        }
    }
}

And for each URL, submit a new task to the ExecutorService

final Collection<String> urls = new ArrayList<>();
final Collection<String> executedUrls = new CopyOnWriteArrayList<>();

...

for (final String url : urls) {
    // The RestTemplate instance is retrieved via Spring Bean, or created manually 
    executorService.submit(new RestRunnable(url, restTemplate, executedUrls));
}

RestRunnable will insert the URL in the executedUrls thread-safe CopyOnWriteArrayList if it was invoked successfully.

Remember the ExecutorService must be shutted down when not needed anymore.

LppEdd
  • 20,274
  • 11
  • 84
  • 139
  • It can be needed to execute different request methods, not only post. Following this example I should create different RestRunnable for different methods. Perhaps it's not the best choice. And in fact It is not necessary to check status code because if I don't have an exception it's already successful execution. – Donatello Mar 20 '19 at 21:34
  • 1
    @Donatello no, you should check the status, always. **It is** ok to have multiple implementations of Runnable for each HTTP call type. Alternatively you can pass another parameter to parameterize the type of call. – LppEdd Mar 20 '19 at 21:36
  • Is it use of CopyOnWriteArrayList faster than synchronized block on arrayList.add() operation? CopyOnWriteArrayList creates a new array after every mulative operation – Donatello Mar 20 '19 at 21:36
  • 1
    @Donatello CopyOnWriteArrayList will always be better then manually synchronizing. Manual sychronization is also error prone if you don't know what you're doing. Avoid it as much as you can – LppEdd Mar 20 '19 at 21:37
  • 1
    @Donatello you can also pass a Supplier to RestRunnable. The Supplier can be created using a *lambda expression*, and you can directly build your call logic there. – LppEdd Mar 20 '19 at 21:43
  • It's necessary to create new RestRunnable() every time. I can use lamba instead: executorService.submit(() -> {code is here}). What is the advantage of creation RestRunnable then? – Donatello Mar 20 '19 at 22:03
  • 1
    @Donatello A lambda should be two/three lines maximum. I prefer extracting it to a specific private static class. With a lambda, an anonymous class would be created anyway, with performance penalties too. – LppEdd Mar 20 '19 at 22:10
  • @Donatello obviously mine was a generic example. You can tweak that class as much as you want for your specific needs, but the logic remains. – LppEdd Mar 20 '19 at 22:12