Bot-like Spring Java service for checking advertisements

Question

I am in the middle of developing a Spring Boot application, which has a service acting like a bot.

advertisementService.update(Advertisement ad) is a method which parses some HTML. It is important that it cannot execute periodically. For now it is pseudo random, but in the future there will be some logic returning the time depending on information read in previous parsing.

Immediately after the start of the app, it invokes the method scheduleAllChecks(). If a new Advertisement comes up, then it calls scheduleCheck(0, ad);

I am wondering if the solution (I mean the whole service class) is proper and if it can be beter?

package com.gadomski.service;
import com.gadomski.model.Advertisement;
import org.slf4j.Logger;
import org.slf4j.LoggerFactory;
import org.springframework.beans.factory.annotation.Autowired;
import org.springframework.stereotype.Service;
import java.util.Random;
import java.util.concurrent.Executors;
import java.util.concurrent.ScheduledExecutorService;
import java.util.concurrent.ScheduledFuture;
import java.util.concurrent.TimeUnit;
/**
 * Created by Krzysiek on 21.08.2016.
 */
@Service
public class CheckerService {
 private final Logger log = LoggerFactory.getLogger(this.getClass());
 private final int CHECKER_THREADS = 100;
 @Autowired
 AdvertisementService advertisementService;
 private ScheduledExecutorService scheduledExecutorService;
 CheckerService() {
 this.scheduledExecutorService = Executors.newScheduledThreadPool(CHECKER_THREADS);
 }
 public void scheduleCheck(int delaySeconds, Advertisement advertisement) {
 if (advertisement.isActive()) {
 ScheduledFuture<?> check = this.scheduledExecutorService.schedule(
 () -> {
 scheduleCheck(getDelayInSeconds(), advertisement);
 advertisementService.updateAdvertisement(advertisement);
 log.info("Scheaduled next check for " + advertisement.getUrl() + " for user with id: " + advertisement.getUserId());
 },
 delaySeconds, TimeUnit.SECONDS);
 }
 }
 private int getDelayInSeconds() {
 Random rand = new Random();
 return (8 + rand.nextInt(10)) * 60 + rand.nextInt(60);
 }
 public void scheduleAllChecks() {
 Random random = new Random();
 advertisementService.getAllActvieAdvertisements().forEach(a -> scheduleCheck(random.nextInt(3 * 60), a));
 }
}

• Ron Beyer: Looks like the getAllActvieAdvertisements in the AdvertisementService is spelled incorrectly, but that is all I've got.

Answer 1

It appears that you are using Random across multiple threads. I guess you plan to replace it in the future, but still, according to the documentation:

Instances of java.util.Random are threadsafe. However, the concurrent use of the same java.util.Random instance across threads may encounter contention and consequent poor performance. Consider instead using ThreadLocalRandom in multithreaded designs.

You should probably rewrite this to use java.util.concurrent.ThreadLocalRandom. As the docs say:

When applicable, use of ThreadLocalRandom rather than shared Random objects in concurrent programs will typically encounter much less overhead and contention.

• Bohdan Mart: Actually he is creating new instance of Random each time, so there won't be present issue you described, but would be other issue, with calling not so cheap constructor of random each time.
• Dair: @BogdanMart I am sorry, but I am not quite sure I understand what you are saying is wrong with my answer. The problem I am addressing is precisely about overhead.
• Bohdan Mart: Hmm, I wasn't aware, that ThreadLocalRandom is SIngleton, than it could be used straight instead of random in this case. But my point is that problem is not only in use of Random but also in it's instantiation each time. Proper use case of Random is to store it in variable/class field for later use.