Skip to content

Commit

Permalink
Attempt casValue CAS in a loop.
Browse files Browse the repository at this point in the history
This shouldn't be necessary, but we're seeing some evidence that it may help with a mysterious bug.

(I considered renaming the other `cas*` methods (in which I did not add a loop) to "`weakCas*`" to emphasize that _they_ might fail spuriously. But it's hard to imagine how we could ever rewrite their callers to _not_ use a loop, so I don't think we'd be protecting ourselves from any mistakes. Plus, "weak CAS" carries additional meaning (specifically, lack of ordering guarantees) that may or may not be appropriate here. And anyway, we hope that this is all a short-term hack.)

RELNOTES=n/a
PiperOrigin-RevId: 413130485
  • Loading branch information
cpovirk authored and Google Java Core Libraries committed Nov 30, 2021
1 parent 2d875d3 commit e64ad26
Show file tree
Hide file tree
Showing 2 changed files with 96 additions and 4 deletions.
Original file line number Diff line number Diff line change
Expand Up @@ -1414,7 +1414,39 @@ Waiter gasWaiters(AbstractFuture<?> future, Waiter update) {
/** Performs a CAS operation on the {@link #value} field. */
@Override
boolean casValue(AbstractFuture<?> future, @CheckForNull Object expect, Object update) {
return UNSAFE.compareAndSwapObject(future, VALUE_OFFSET, expect, update);
/*
* We've spent some time debugging an "impossible" issue, and we've seen some evidence that
* suggests that the CAS APIs may be spuriously failing in some set of environments that we
* haven't pinned down. While we investigate further, we're wrapping this CAS in a retry loop.
* (The other AtomicHelper.cas* methods are already called in retry loops, so we don't need to
* wrap them.)
*
* Of course, if CAS in general is broken in some environments, then we are likely to
* encounter more problems than just in AbstractFuture. Fortunately, many CAS operations
* already occur inside a retry loop. So the problem, if it exists, won't affect all users of
* CAS. Still, there are many other users that it *could* affect, including
* c.g.c.util.concurrent classes like AggregateFutureState, ClosingFuture, ExecutionSequencer,
* and InterruptibleTask.
*
* Regardless, we can't fix the problem in general here, and we can't fix it at all until we
* understand it better. But for now, a loop here appears to address the "impossible" issue,
* so we're starting with that.
*
* Luckily, there should be little performance impact: casValue will almost always be
* uncontended, and any failures are likely to happen in the cancellation case, which is
* already expensive.
*/
return workaroundCasObject(future, VALUE_OFFSET, expect, update);
}

private static boolean workaroundCasObject(
Object receiver, long offset, @CheckForNull Object expect, @CheckForNull Object update) {
do {
if (UNSAFE.compareAndSwapObject(receiver, offset, expect, update)) {
return true;
}
} while (UNSAFE.getObject(receiver, offset) == expect);
return false;
}
}

Expand Down Expand Up @@ -1475,7 +1507,21 @@ Waiter gasWaiters(AbstractFuture<?> future, Waiter update) {

@Override
boolean casValue(AbstractFuture<?> future, @CheckForNull Object expect, Object update) {
return valueUpdater.compareAndSet(future, expect, update);
// For more discussion, see the comment in the same method in UnsafeAtomicHelper.
return workaroundCasObject(future, valueUpdater, expect, update);
}

private static <R, T> boolean workaroundCasObject(
R receiver,
AtomicReferenceFieldUpdater<R, T> updater,
@CheckForNull T expect,
@CheckForNull T update) {
do {
if (updater.compareAndSet(receiver, expect, update)) {
return true;
}
} while (updater.get(receiver) == expect);
return false;
}
}

Expand Down
50 changes: 48 additions & 2 deletions guava/src/com/google/common/util/concurrent/AbstractFuture.java
Original file line number Diff line number Diff line change
Expand Up @@ -1398,7 +1398,39 @@ Waiter gasWaiters(AbstractFuture<?> future, Waiter update) {
/** Performs a CAS operation on the {@link #value} field. */
@Override
boolean casValue(AbstractFuture<?> future, @CheckForNull Object expect, Object update) {
return UNSAFE.compareAndSwapObject(future, VALUE_OFFSET, expect, update);
/*
* We've spent some time debugging an "impossible" issue, and we've seen some evidence that
* suggests that the CAS APIs may be spuriously failing in some set of environments that we
* haven't pinned down. While we investigate further, we're wrapping this CAS in a retry loop.
* (The other AtomicHelper.cas* methods are already called in retry loops, so we don't need to
* wrap them.)
*
* Of course, if CAS in general is broken in some environments, then we are likely to
* encounter more problems than just in AbstractFuture. Fortunately, many CAS operations
* already occur inside a retry loop. So the problem, if it exists, won't affect all users of
* CAS. Still, there are many other users that it *could* affect, including
* c.g.c.util.concurrent classes like AggregateFutureState, ClosingFuture, ExecutionSequencer,
* and InterruptibleTask.
*
* Regardless, we can't fix the problem in general here, and we can't fix it at all until we
* understand it better. But for now, a loop here appears to address the "impossible" issue,
* so we're starting with that.
*
* Luckily, there should be little performance impact: casValue will almost always be
* uncontended, and any failures are likely to happen in the cancellation case, which is
* already expensive.
*/
return workaroundCasObject(future, VALUE_OFFSET, expect, update);
}

private static boolean workaroundCasObject(
Object receiver, long offset, @CheckForNull Object expect, @CheckForNull Object update) {
do {
if (UNSAFE.compareAndSwapObject(receiver, offset, expect, update)) {
return true;
}
} while (UNSAFE.getObject(receiver, offset) == expect);
return false;
}
}

Expand Down Expand Up @@ -1459,7 +1491,21 @@ Waiter gasWaiters(AbstractFuture<?> future, Waiter update) {

@Override
boolean casValue(AbstractFuture<?> future, @CheckForNull Object expect, Object update) {
return valueUpdater.compareAndSet(future, expect, update);
// For more discussion, see the comment in the same method in UnsafeAtomicHelper.
return workaroundCasObject(future, valueUpdater, expect, update);
}

private static <R, T> boolean workaroundCasObject(
R receiver,
AtomicReferenceFieldUpdater<R, T> updater,
@CheckForNull T expect,
@CheckForNull T update) {
do {
if (updater.compareAndSet(receiver, expect, update)) {
return true;
}
} while (updater.get(receiver) == expect);
return false;
}
}

Expand Down

0 comments on commit e64ad26

Please sign in to comment.