Fix a race condition when adding a callback.

The following "unlucky execution" could happen:
 Deferred in state RUNNING.
 Thread A (in runCallbacks)                 | Thread B (in addCallbacks)
   complete execution of the last callback  |   CAS state DONE -> RUNNING fails
   acquire this' monitor                    |
   observe that the callback chain is empty |
   set state to DONE                        |
   null out callback chains                 |
   release this' monitor                    |
                                            |   acquire this' monitor
					    |   queue the new callbacks
					    |   release this' monitor

Now Thread B left the Deferred in state DONE with a callback in the
chain that will never be called unless another callback is added to it.

The fix consists in swapping the CAS and the acquisition of this'
monitor in addCallbacks.  In the unlucky timing above, this could
have 2 possible outcomes:
  1. Thread A acquires this' monitor first and then thread B's CAS
     will succeed, so B will keep executing the callback chain.
  2. Thread B acquires this' monitor first and queues the new
     callbacks, then thread A will execute them.
When swapping the CAS and the acquisition of this' monitor, the CAS
becomes unnecessary and can be replaced with a regular volatile-read
followed by a regular volatile-write.  The purpose of the CAS was
originally to avoid acquiring this' monitor when it wasn't necessary
but this "optimization" wasn't even effective since the most common
code path is to add a callback to a Deferred in state PENDING, and
this code path always needed a failed CAS + monitor acquisition.

Change-Id: I365cec225e9d15aa09280ce066bf11ce56c1d358

Author: tsuna

src/Deferred.java

@@ -610,11 +610,20 @@ public <R, R2, E> Deferred<R> addCallbacks(final Callback<R, T> cb,
     } else if (eb == null) {
       throw new NullPointerException("null errback");
     }
-    // If we're DONE, switch to RUNNING atomically.
-    if (!casState(State.DONE, State.RUNNING)) {
-      // We get here if weren't DONE or if we were DONE and another thread
-      // raced with us and we lost the race.
-      synchronized (this) {
+    // We need to synchronize on `this' first before the CAS, to prevent
+    // runCallbacks from switching our state from RUNNING to DONE right
+    // before we add another callback.
+    synchronized (this) {
+      // If we're DONE, switch to RUNNING atomically.
+      if (state == State.DONE) {
+        // This "check-then-act" sequence is safe as this is the only code
+        // path that transitions from DONE to RUNNING and it's synchronized.
+        state = State.RUNNING;
+      } else {
+        // We get here if weren't DONE (most common code path)
+        //  -or-
+        // if we were DONE and another thread raced with us to change the
+        // state and we lost the race (uncommon).
         if (callbacks == null) {
           callbacks = new LinkedList<Callback>();
           errbacks = new LinkedList<Callback>();
@@ -625,9 +634,10 @@ public <R, R2, E> Deferred<R> addCallbacks(final Callback<R, T> cb,
         }
         callbacks.addLast(cb);
         errbacks.addLast(eb);
+        return (Deferred<R>) ((Deferred) this);
       }
-      return (Deferred<R>) ((Object) this);
     }
+
     if (!doCall(result instanceof Exception ? eb : cb)) {
       // While we were executing the callback, another thread could have
       // added more callbacks.  If doCall returned true, it means we're