Java Thread Race, Code messes up after changing System.out.println

I am having issues with my program printing out only one winner and still allowing the other threads to finish the race while printing their time. When I take out the System.out.println line it messes everything up and I have no idea how and why it is possible.

I am using the Runnable and lock statements and still running into the issue.

ThreadThings

package com.company;
import java.util.concurrent.locks.Lock;
import java.util.concurrent.locks.ReentrantLock;

public class ThreadThings implements Runnable {

Lock lock = new ReentrantLock();

private String ThreadName;
static boolean winner;
int position = 0;

public ThreadThings(String ThreadName) {
    this.ThreadName = ThreadName;
}


//Start of a thread
@Override
public synchronized void run() {

    lock.lock();

    long startTime = System.currentTimeMillis();

    for (int i = 1; i <= 100; i++) {
        System.out.println(ThreadName + " Meters " + i);
        this.position = i;
        //position++;


        try {
            Thread.sleep(10);
        } catch (InterruptedException e) {


        }

        if ((position == 100) && (winner == false)) {
            System.out.println(ThreadName+ " wins");
            winner = true;
            System.out.println("Congrats " + ThreadName + " you are the winner");

        }


      /**  if ((position == 10) && (winner == true)) {

                System.out.println("Good Try " + ThreadName);
            }
**/
        }

    long stopTime = System.currentTimeMillis();

    long raceTime = stopTime - startTime;

    System.out.println(ThreadName + " Time is " + raceTime + " Minutes");
    // else {System.out.println("Thanks for running");   }

    lock.unlock();

    }


}

Main

package com.company;

import java.util.concurrent.locks.Lock;
import java.util.concurrent.locks.ReentrantLock;

public class Main {

public static void main(String[] args) {

    Lock lock = new ReentrantLock();

    ThreadThings t1 = new ThreadThings("Abby");
    ThreadThings t2 = new ThreadThings("Brandon");
    ThreadThings t3 = new ThreadThings("Casey");
    ThreadThings t4 = new ThreadThings("David");
    ThreadThings t5 = new ThreadThings("Eddie");
    ThreadThings t6 = new ThreadThings("Frank");
    ThreadThings t7 = new ThreadThings("Gabby");
    ThreadThings t8 = new ThreadThings("Hannah");
    ThreadThings t9 = new ThreadThings("Indie");
    ThreadThings t10 = new ThreadThings("Jasmine");

    Thread Runner1 = new Thread(t1);
    Thread Runner2 = new Thread(t2);
    Thread Runner3 = new Thread(t3);
    Thread Runner4 = new Thread(t4);
    Thread Runner5 = new Thread(t5);
    Thread Runner6 = new Thread(t6);
    Thread Runner7 = new Thread(t7);
    Thread Runner8 = new Thread(t8);
    Thread Runner9 = new Thread(t9);
    Thread Runner10 = new Thread(t10);

    lock.lock();

    Runner1.start();
    Runner2.start();
    Runner3.start();
    Runner4.start();
    Runner5.start();
    Runner6.start();
    Runner7.start();
    Runner8.start();
    Runner9.start();
    Runner10.start();

    lock.unlock();

    }

}

Answer

You’ve locked 3 different things, and every lock is completely useless.

The thing you’re missing is that locks aren’t magic pixie dust that just randomly makes things work. They serve to link constructs – if two different threads both try to lock the same lock object, something happens (one of em will wait). If you do anything else, it’s useless.

Useless lock 1 = The lock in main

You create one lock, and then lock it. This lock is not shared with any other thread, and therefore, does nothing.

Useless lock 2 = The lock in ThreadThings.run()

Again, a lock that isn’t shared with anything else. Each and every thread has its own lock, and locks it. This does nothing.

Useless lock 3 = synchronized

synchronized, on an instance method, is syntax sugar for locking the entire method body on this. Given that you’re making 10 new ThreadThings object, yet again, these are locks nothing else is locking on, and thus, they do nothing.

The fix

Make a single lock object and use that everywhere. Note that the basic approach you’ve landed on here (which is to lock when you start a thread and unlock it when it is done) means that this entire exercise would then be useless in a different fashion: You’d have 10 threads, but 9 of the 10 will be waiting. In other words, it’s just a song and dance routine that accomplishes nothing useful – you might as well just do the 10 tasks sequentially, because that’s exactly what your code would do, had you fixed it.

How locks really work

Find the one thing that requires synchronization and use locks to guard just that section and nothing more. For example, I highly doubt you’d want Thread.sleep() within a locked block, as the lock isn’t disengaged. Your threads now waits 10 millisecs, and so will the others, as they simply can’t continue without acquiring the lock, which they can’t, because your sleeping thread has it.

You can lock and unlock a couple of times in a thread if you like. That seems like what you’re looking for.

Fixes:

1. Either use locks, or use synchronized. Not both.
2. Make one lock – either synchronized on one object, or create one lock in your main and pass it to the 10 ThreadThings objects so that they can lock on it.
3. Don’t just lock at the top of the run() and unlock at the end – then your threads are just a useless exercise. Perhaps lock and unlock multiply times (i.e. unlock before sleeping, re-lock before continuing).