Spawning two threads and if any of the threads returns true, let the main thread ignore the other and move on

Question

Exact problem statement:

A main thread spawns 2 new threads, the main thread waits on this threads with these conditions:

1. if any of the threads returns True, then main thread moves on and prints the thread_id and timestamp of the thread.
2. if both threads return false, finish execution and don't print anything

My attempt:

public static void main(String[] args) throws InterruptedException, ExecutionException{        
        FutureTask<Boolean> futureTask = new FutureTask<>(() -> {
          System.out.println("Hello, World 1!");
          return true;
        });
        FutureTask<Boolean> futureTask2 = new FutureTask<>(() -> {
          System.out.println("Hello, World 2!");
          return true;
        });
        
        Thread t1 = new Thread(futureTask);
        Thread t2 = new Thread(futureTask2);
        t1.start();
        t2.start();
        
        // Wait for either to be done.
        while (!futureTask.isDone() && !futureTask2.isDone());

        if (futureTask.isDone()) {
          if (futureTask.get()) {
            System.out.println("1 - " + t1.getId() + " " + System.currentTimeMillis());
            return;
          }
        }

        if (futureTask2.isDone()) {
          if (futureTask2.get()) {
            System.out.println("2 - " + t2.getId() + " " + System.currentTimeMillis());
            return;
          }
        }
    }
}

When I run this a few times, sometimes thread 1 wins, sometimes 2, which seems to make sense to me. Appreciate any room for improvement or advice.

Answer 1

Bug

I'd post this as a comment and a close vote, but you already have two answers.

Your program does not actually fulfill the problem statement. If one thread quickly returns false and the other has not finished yet (but will return true eventually), your program halts and prints nothing. But instead, it should wait for the second thread to finish.

Testing

You could have found this yourself with a more robust testing framework. You should test all of the following situations:

1. Both return true. Task 1 returns first.
2. Both return true. Task 2 returns first.
3. Task 1 returns true first and later task 2 would return false.
4. Task 1 returns true but task 2 returns false later.
5. Task 2 returns true first and later task 1 would return false.
6. Task 2 returns true but task 1 returns false later.
7. Both return false. Task 1 returns first.
8. Both return false. Task 2 returns first.

You might also want to check what happens when a task never returns. There may be other edge cases as well.

Numbered variables

Variables named 1 and 2 are a potential code smell. In general when you have this, you should consider if you could actually either name the variables better or switch to a collection (e.g. an array or something that implements the Collection interface).

A side benefit of switching to a collection is that it frees you up in case the requirements change from two to another number.

This is not to say that there are no circumstances under which numbered variables are OK. Only that when you consider using them, you should consider if they really make sense.

Modularization

To make testing easier, I would move the main code outside of the main method. So instead of editing the main method for each test, your tests would create the appropriate tasks and pass them to the task runner. The task runner could then enshrine the logic of

1. Create threads.
2. Launch them.
3. Check if they are finished.
4. If one is finished, check if its task returned true and produce the correct output.
5. If both returned false, return.

Note that this will be easier to test if the task runner method returns a string rather than printing output. You can leave the printing logic in your main method.

Test frameworks

Even better than putting your tests in your main method, put them into some kind of test framework. E.g. JUnit. With a testing framework, it is easy to run every test every time.

Answer 2

In my opinion, there are one flaw in your current implementation.

You need to manually check if the threads are done executing manually (they are multiple ways to do that automatically).

At the moment, you need to manually check the state of the thread.

For your code, I would suggest you to use the java.util.concurrent.ExecutorService, if you are allowed to use it.

This service allows you to control the execution of the threads; they are multiple implementation of this service (each implementation run threads differently depending of the conditions).

For your case, I would use the FixedThreadPool, that run a maximum number of threads at the same time.

If you can use it, it will allow you to replace the while and the part where you start the threads manually.

ExecutorService taskExecutor = Executors.newFixedThreadPool(2); // Two execution maximum at the same time

Thread t1 = new Thread(futureTask);
Thread t2 = new Thread(futureTask2);

taskExecutor.execute(t1); // Here, you could put directly the `FutureTask`, but since we need the id, we use the thread.
taskExecutor.execute(t2);

taskExecutor.shutdown(); // We close the executor, since if we don’t, the main thread will continue and not end even if the two other threads are done.

try {
   taskExecutor.awaitTermination(Long.MAX_VALUE, TimeUnit.NANOSECONDS); // Here, we add a time limit on the execution, you can add any time.
} catch (InterruptedException e) {
   System.out.println("One of the thread ran more than the allowed time!");
}

This way if not optimal when executed multiple time, since you need to recreate a java.util.concurrent.ExecutorService on each execution, but is fine in this case, in my opinion.

Answer 3

Caveat: I don't know java. Since this site is pretty low volume, figured I'd offer some thoughts anyhow.

The first function sleeps for 2 seconds, the other doesn't. How does Hello, world 1! ever get done first? Or is that not what "win" means?

Looking at some doc, isDone doesn't appear to block. Thus, I don't see why your while() is blocking on any event. That means it's a loop of unbounded CPU cycles, and that's never good. At least, as I've seen some examples do, you should sleep a bit, perhaps some ms, before checking again. Otherwise, you're looping and checking the results as fast as you can. That seems less than optimal.