How to avoid the race condition detected in the following code?

Question

Consider the snippet taken from here:

// event
public class Event { }
// An Event Listener
public interface EventListener {
        public void onEvent(Event e);
}

// inner class instances contain a hidden reference to the enclosing instance
public class ThisEscape {
    private final int num;

    public ThisEscape(EventSource source) {
        source.registerListener(new EventListener() {

            @Override
            public void onEvent(Event e) {
                doSomething(e);
            }
        });
        num = 42;
    }

    private void doSomething(Event e) {
        if (num != 42) {
            System.out.println("Race condition detected at " + new Date());
        }
    }
}
// event source

public class EventSource extends Thread {
    private final BlockingQueue<EventListener> listeners = 
                                new LinkedBlockingQueue<EventListener>();

    public void run() {
        while (true) {
            try {
                listeners.take().onEvent(null);
            } catch (InterruptedException e) {
                break;
            }
        }
    }

    public void registerListener(EventListener eventListener) {
        listeners.add(eventListener);
    }
}


// testing the conditions

public class ThisEscapeTest {
    public static void main(String[] args) {
        EventSource es = new EventSource();
        es.start();
        while (true) {
            new ThisEscape(es);
        }
    }
}

To consolidate, we have 2 threads

// Main Thread
// Event Source

In the Event Source thread, there is a BlockingQueue to store EventListener. In the run method for the same thread,

The consuming EventSource thread keeps taking objects out of the blocking queue, and processes them. If the same thread tries to take an object out of an empty queue, the very same thread is blocked until a producing thread (Main Thread) puts an object into the queue.

Since these 2 operations (below) are not atomic, due to some unlucky timimg, so between the same 2 operations, it is highly likely that the EventSource may find that num != 2 & hence the race condition.

source.registerListener(new EventListener() {   // OPERATION 1      
    @Override
    public void onEvent(Event e) {
        doSomething(e);
    }
    });
    num = 42;                                       // OPERATION 2
}

Since as suggested and seen clearly, the inner class instances contain a hidden reference to the enclosing instance.

While the lock has been acquired by the same thread(Main Thread), the non-synchronised method doSomething() can still be accessed by another thread (in this case EventSource) at the same time, I see that even synchronizing the 2 operations above won't avoid the race conditions. Is my understanding correct? I mean

 public ThisEscape(EventSource source) {
     synchronized(this){                // SYNCHRONISED
         source.registerListener(new EventListener() {
            @Override
            public void onEvent(Event e) {
                doSomething(e);
            }
         });
    num = 42;
    }
}

And the only way to avoid the race condition is make the doSomething() method also synchronized, apart from synchronising the 2 operations?

Thirdly, I see whether the field is final or not, it doesn't makes any difference. The race condition will still be there. What exactly is the author's point of discussion about the final field (apart from making private final int num = 42)?

Answer 1

As you already realized, publishing this in the constructor is a really bad idea.

This is a good way around it; Use a factory method.

public static ThisEscape newInstance(EventSource source){
   final ThisEscape instance = new ThisEscape();
   source.registerListener(new EventListener() {

        @Override
        public void onEvent(Event e) {
            instance.doSomething(e);
        }
    }
   return instance;
}