4

I have this piece of code:

final Future<ApplicationUser> user = applicationUserService.findIncludePrivilegesById(id);
    while (!user.isDone()) {
        Thread.sleep(100);
    }
    if (user.get() == null) throw new Exception(this.getMessageSource().getMessage("label.error.findError",
            null, locale));
    model.addAttribute("user", new ApplicationUserDto(user.get()));

    model.addAttribute("roles", Role.valuesMinusApi());
    model.addAttribute("groups", completeAuthorityList);
    model.addAttribute("message", this.getMessageSource().getMessage("label.dialog.userEdit", new Object[]{user.get().getLogin()}, locale));
    model.addAttribute("applicationUserStates", ApplicationUserState.values());
    return "administration/edit";

The applicationUserService.findIncludePrivilegesById(id) goes to a remote db server to query the data.
My thoughts on designing this piece of code were to let this (time consuming db communication) handle a free thread from the pool (by using async).

As far as i have understood the whole async these steps may happen:

  1. Main thread enters the method
  2. A thread from the thread pool queries the data
  3. The main thread waits or has ressources left over to do "other" things
  4. If the thread pool thread is finished, i can use the results

Do i really profit from this scenario as i have to wait for the results in every case (calling the service method async of sync)?
Is it good practise to use Thread.sleep()?

The only benefit i can imagine from this example is that the main thread is free (not blocked) for other computations (handling other web requests maybe) while the thread pool thread does the time consuming process?

Thomas Lang
  • 1,285
  • 17
  • 35

1 Answers1

6

Do i really profit from this scenario as i have to wait for the results in every case (calling the service method async of sync)?

No. What you've created is essentially still a blocking call, but you've made it a lot more complex than a simple blocking call would have been:

ApplicationUser user = applicationUserService.findIncludePrivilegesById(id);

It also uses 2 threads instead of 1, but you're still doing one only 1 thing. The second thread is performing pseudo busy-waiting and only essentially asking the other thread "are we there yet?" every 100ms.

If your main thread had actual work to do besides sleep() (for example perform a second slow call to a different server) then the above code would have some merit. But even then it could make the code confusing as it's doing manual multi-threading in a special situation. A small redesign could be a lot better option.

Is it good practise to use Thread.sleep()?

No. There's very little reason to call Thread.sleep() unless you want to pause a thread (which is mostly very unnecessary). If you are thinking about using Thread.sleep(), you should rethink what you're doing.

Finally, the semi-busy waiting you're doing with

while (!user.isDone()) {
    Thread.sleep(100);
}

could be replaced with just user.get(), which would block (i.e. sleep) until the other thread is done. This would be more efficient (and synchronous). So you're not getting any advantage from the current code as is.

If you're just concerned that there's a long call happening and you'd like to use that time for something, don't worry. That thread will be in a wait state and there will be other threads to run. If you have a situation where you're actually running out of resources because all your threads are getting blocked on that call, then maybe it's time to look at why it's taking so long to do that call?

Now there are ways to perform non-blocking calls to actually get some benefits you're thinking about, but those require support from libraries and drivers, and require a different way of programming without resorting to threadpools or Futures.

Kayaman
  • 72,141
  • 5
  • 83
  • 121
  • would you suggest doing this time consuming computation synchronously? Or should i do "other" things in the meantime (aka in the while loop) until the time consuming operations is done? – Thomas Lang Nov 02 '20 at 15:55
  • 1
    @ThomasLang I suggest doing the simplest thing, which is performing a regular, blocking/synchronous call. While in theory you could do "other" things in the meantime, in practice it doesn't work that well. See my edits especially on the bottom for the things you're probably thinking about. – Kayaman Nov 02 '20 at 16:11
  • Thank you very much for your detailed explanation. By the way, what do you think of CompletableFuture like here: https://spring.io/guides/gs/async-method/ – Thomas Lang Nov 03 '20 at 06:40
  • @ThomasLang a `CF` is preferrable to a regular `Future`, and Spring can automatically leverage it in [some cases](https://stackoverflow.com/q/58505549/2541560). The guide shows a cleaner way to write code similar to what you had, with actual parallel work (which you didn't have). The hard part is knowing when to use async methods. The guide has the same synthetic example I described: multiple slow calls. Unless you're working on legacy code, you'd want to fix why it takes so long for those calls. So read that guide for technical details, not for design or usage guidance. – Kayaman Nov 03 '20 at 11:05