Limiting call frequency to a static method

We have a method which is being called from multiple threads and accesses an external database. In order not to slow down the database for other clients, the calls to this method shall be limited to let's say 1call/second.

I like to keep things simple, so I just did this:

private static final Object SYNC_LOCK = new Object();

public static double myMethod(int param1, ...) {
  synchronized(SYNC_LOCK) {
    //do something...
    Thread.sleep(1000);
    return result;
  }
}

Now, we are using sonarqube for code analysis and this sleep is considered as a "blocker" bug.
From looking at the code, I can rule out deadlocks. And implementing a kind of token based approach seems a bit much to me.

Would you agree with sonarqube that this code needs to be changed?

Now, we could use for example a thread pool to achieve the same as written below. But the first example seems much more sleek to me.

private static ExecutorService es = Executors.newFixedThreadPool(1);
private static long lastCall = 0;

public static Double myMethod(int param1, ...) {
    Future<Double> f = es.submit(new Callable<Double>() {
        @Override
        public Double call() throws Exception {
            long diff = System.currentTimeMillis() - lastCall;
            if (diff < 1000) {
                long sleepMillis = 1000 - diff;
                Thread.sleep(sleepMillis);
            }
            //do something...
            lastCall = System.currentTimeMillis();
            return result;
        }
    });
    try {
        return f.get();
    } catch (InterruptedException | ExecutionException e) {
        //handle this
        return null;
    }
}

I guess this is a band-aid that kind of works. But it creates a bottleneck where only one request can make progress at a time. It's also rough on your clients that they have to pay the time penalty up front, rather than your checking if enough time has gone by since the last request.

Sonarqube is a static analysis tool, all it can do is find patterns in code and apply rules to them. In general the rule of not sleeping with a lock held makes a lot of sense.
When a thread holds a lock obviously other threads are blocked, and when a thread is sleeping it's not doing work, so it is clearly not optimal. In a lot of cases you see programmers adding sleeps as desperate (and ill-advised) attempts to avoid lost-notifications and other bugs, and I think that's what Sonarqube is trying to flag.

First, since hitting the external database is a pain point and you want to relieve the load on it, try caching the results as much as possible.

When you use a ThreadPoolExecutor you get better control over the rate of work by configuring the number of workers, setting up a rejection policy, etc. Once caching reduces the load on the external database enough that you want more than one request at a time, you can tweak the number of worker threads to increase throughput.

Extract business logic in some other method (say myExpensiveMethod() below) then think of implementing client side Rate Limiter (I'm assuming that there is no side effect of concurrent call) -

RateLimiterConfig config = RateLimiterConfig.custom()
  .limitForPeriod(1)
  .limitRefreshPeriod(Duration.ofSeconds(1))
  .timeoutDuration(Duration.ofSeconds(1))
  .build();

And from myMethod() call myExpensiveMethod()

public static Double myMethod() {

  RateLimiterRegistry registry = RateLimiterRegistry.of(config);
  RateLimiter limiter = registry.rateLimiter("myMethod");
  Supplier<Double> dbQuerySupplier = 
       RateLimiter.decorateSupplier(limiter,
                  () -> myExpensiveMethod());
  return dbQuerySupplier.get();
}

IMO Simplest approach would be to do something like this (please excuse the pseudo-library calls):

public static double myRealMethod() {
  synchronized(SYNC_LOCK) {
    //do something...
    Thread.sleep(1000);
    return result;
  }
}

private static double cachedResult;
private static Somekindoftimestamp cachedResultDate = A_LONG_TIME_AGO;
public static double myMethod() {
  synchronized(SYNC_LOCK) {
    if (cachedResultDate.isTooOldForMyLiking()) {
      cachedResult = myRealMethod();
    }
    return cachedResult;
  }
}

One obvious down-side: Some calls to myMethod() would take a lot longer than most others.

One possible improvement (depending on your application's needs): Have myMethod() always return the cached result, and create a periodic Timer task that calls myRealMethod() once per second to update the cached result whether it's wanted or not.