CompletableFuture might complete in unexpected thread - Copycat gets unreliable about copycat HOT 3 OPEN

Just for a quick illustration of the problem: Basically this is what happens:

public class CompletableFutureTest {
  public static void main(String[] args) throws InterruptedException {
    Executor executor = Executors.newSingleThreadScheduledExecutor();

    System.out.println(Thread.currentThread().getId() + ": Main thread");

    CompletableFuture<Void> f = new CompletableFuture<>();

    executor.execute(() -> {
      try {
        f.whenComplete((r, e) -> {
          System.out.println(Thread.currentThread().getId() + ": Complete!");
        });
        Thread.sleep(1000);
        f.complete(null);
      } catch (Exception e) {
        e.printStackTrace();
      }
    });

    Thread.sleep(3000);
    f.thenCompose(v -> {
      System.out.println(Thread.currentThread().getId() + ": thenCompose!");
      return new CompletableFuture<>();
    });
  }
}

Output is:

1: Main thread
10: Complete!
1: thenCompose!

That means that the Function we pass in "thenCompose" is executed on the main thread, but copycat expects it to be executed on the same thread as the one writing "10: Complete!".

from copycat.

The Copycat client internals have been completely rewritten, but I think we still do need to do some review of that code before a full release to make sure this issue is not present. I've seen some non-intuitive behavior in CompletableFuture, particularly with thenCompose.

@bgloeckle correct me if I'm wrong, but essentially what you're seeing is if the main thread gets to the thenCompose call after the future being composed is completed, CompletableFuture will run the composed callback on the main thread rather than the thread that completed it?

A while back, we actually had similar issues with thenComposeAsync causing deadlocks in the server. I found an obscure reference to an issue in the CompletableFuture implementation that was fixed in some later Java releases, and I removed all use of thenComposeAsync.

I think I may just want to do the same thing and replace thenCompose calls with a helper for internal use that ensures composed callbacks are executed on the proper thread (the thread that completed the future). Its use only seems to be safe inside the Copycat server where futures are created and completed on the same thread. Most of the other code that completes futures on a thread other than the one on which it was created already account for this issue by explicitly completeing futures in a specific thread.

I will go through and check all the uses of this method. If this pattern isn't being used in the new client I'll close this.

from copycat.

@bgloeckle correct me if I'm wrong, but essentially what you're seeing is if the main thread gets to the thenCompose call after the future being composed is completed, CompletableFuture will run the composed callback on the main thread rather than the thread that completed it?

Yes, that's what I was seeing (I think that was on the "beta5" release). I did not yet check all the code again for the newer releases (I'm just in the process of switching diqube to copycat rc2), but essentially all the methods that create the "pipeline" that will be executed as soon as a CompletableFuture is completed should themselves take care of being executed on the correct thread. I think it's a good idea to have a single utility class for that in copycat.

from copycat.