Why is this code not causing a race condition?

Question

The below code increments a static variable within a Thread and checks to see if its value is incremented by one. This is what Assert.assertEquals(currentAVal+1, accessCounter); checks.

The test consistently passes for 10'000 runs. But why is no race condition causing the test to fail ? I would expect two or more threads to increment accessCounter at line accessCounter = accessCounter + 1; before the assert takes place but this does not seem to be occurring ?

public class RunnableTest {
    private static int accessCounter = 0;

    private class Post implements Runnable {
        public void run() {
            int currentAVal = accessCounter;
            accessCounter = accessCounter + 1;
            Assert.assertEquals(currentAVal+1, accessCounter);
            System.out.println("Access counter : "+accessCounter);
        }
    }

    @Test
    public void runTest(){
        Runnable r = new Post();    
        ScheduledExecutorService executor = Executors.newScheduledThreadPool(4);
        for(int executorCount = 0; executorCount < 10000; ++executorCount) {  
            executor.execute(r);
        }
    }
}

Update : from Gray's answer I've updated the code and I am now receiving a race condition (test failure) when I remove the println statement :

import java.util.ArrayList;
import java.util.List;
import java.util.concurrent.Executors;
import java.util.concurrent.ScheduledExecutorService;
import java.util.concurrent.TimeUnit;

import org.junit.Test;

import junit.framework.Assert;

public class RunnableTest {

    private static int accessCounter = 0;
    private static List<String> li = new ArrayList<String>();

    private class Post implements Runnable {
        public synchronized void run() {

            int currentAVal = accessCounter;
            accessCounter = accessCounter + 1;
            li.add(String.valueOf(currentAVal+1+","+accessCounter));

        }
    }

    @Test
    public void runTest(){

        Runnable r = new Post();    
        ScheduledExecutorService executor = Executors.newScheduledThreadPool(4);
        for(int executorCount = 0; executorCount < 10000; ++executorCount) {  
            executor.execute(r);
        }
        //Wait for threads to finish
        // we shut it down once we've submitted all jobs to it
        executor.shutdown();
        // now we wait for all of those jobs to finish
        try {
            executor.awaitTermination(Long.MAX_VALUE, TimeUnit.MILLISECONDS);
        } catch (InterruptedException e) {
            e.printStackTrace();
        }
        for(String s : li){
            Assert.assertEquals(s.split(",")[0], s.split(",")[1]);
        }
    }

}

Adding synchronized to the run method causes the test to pass

Answer 1

The test consistently passes for 10'000 runs. But why is no race condition causing the test to fail ?

The definition of a race condition is that you might get timing problems -- it is not guaranteed. If you ran this on another architecture you might get wildly different results.

However, I don't think that asserts in other threads are seen by junit. For example, if I change you test the following. I do see times that the value differs but the fail is not seen by the test method -- the test still passes.

if (currentAVal+1 != accessCounter) {
    System.out.println("Access counter not equal: "+accessCounter);
    Assert.fail();
}

One reason why you may be seeing proper values in accessCounter is that System.out.println(...) is a synchronized method which is (as a byproduct) synchronizing the value of accessCounter.

Also, you are not shutting down your executor nor are you waiting for the executor service to actually complete. You should do something like:

// we shut it down once we've submitted all jobs to it
executor.shutdown();
// now we wait for all of those jobs to finish
executor.awaitTermination(Long.MAX_VALUE, TimeUnit.MILLISECONDS);

But this doesn't solve the other thread issue. To actually see the results of the threads you could do something like:

List<Future<?>> futures = new ArrayList<Future<?>>();
for (int executorCount = 0; executorCount < 10000; ++executorCount) {
    futures.add(executor.submit(r));
}
executor.shutdown();
executor.awaitTermination(Long.MAX_VALUE, TimeUnit.MILLISECONDS);
for (Future<?> future : futures) {
    // this will throw an exception if an assert happened
    future.get();
}

Answer 2

Firstly as other answers have pointed out that Race is not guaranteed to occur. Remove the sysout statement as that can cause the code to synchronize.

Answer 3

The answer is in the question itself. It's a race condition :
You can't guarantee that it will ever occur no matter HOW many times threads you try to throw at it or times you try to run it. That's why it's a race condition. It's non-deterministic

Assuming uniform probability distribution for this isn't even remotely correct unless you can show why. You aren't flipping a coin here. The code could run for months before the race is exposed, I've seen this very thing happen many times. That's why race conditions are hard to solve and important to prevent.

Secondly you aren't seeding any amount of random noise into the scenario. If you say had each thread's run function sleep a random amount of time first so that they actually were likely to coincide with one another this would be more intersting... but you're threads a so short they are likely finished and never even running in parallel compared to the time it takes to spawn dispatch the jobs.