rls: Document RefCountedChildPolicyWrapperFactory as non-threadsafe

Instead of having docs in RefCountedChildPolicyWrapperFactory saying
that every method was guarded by a lock, I added `@GuardedBy("lock")`
within CachingRlsLbClient, so now it is clearly not thread-safe and the
lock protects access. The AtomicLong was replaced with a long since
1) there was no multi-threading and 2) the logic was not atomic-safe
which was misleading.
This commit is contained in:
Eric Anderson 2024-03-10 09:01:08 -07:00
parent 2840fd6b47
commit 056195401f
2 changed files with 6 additions and 10 deletions

View File

@ -111,6 +111,7 @@ final class CachingRlsLbClient {
private final RouteLookupServiceStub rlsStub;
private final RlsPicker rlsPicker;
private final ResolvedAddressFactory childLbResolvedAddressFactory;
@GuardedBy("lock")
private final RefCountedChildPolicyWrapperFactory refCountedChildPolicyWrapperFactory;
private final ChannelLogger logger;

View File

@ -41,7 +41,6 @@ import java.util.HashMap;
import java.util.List;
import java.util.Map;
import java.util.Objects;
import java.util.concurrent.atomic.AtomicLong;
import javax.annotation.Nullable;
/** Configuration for RLS load balancing policy. */
@ -203,9 +202,8 @@ final class LbPolicyConfiguration {
}
}
/** Factory for {@link ChildPolicyWrapper}. */
/** Factory for {@link ChildPolicyWrapper}. Not thread-safe. */
static final class RefCountedChildPolicyWrapperFactory {
// GuardedBy CachingRlsLbClient.lock
@VisibleForTesting
final Map<String /* target */, RefCountedChildPolicyWrapper> childPolicyMap =
new HashMap<>();
@ -227,7 +225,6 @@ final class LbPolicyConfiguration {
this.childLbStatusListener = checkNotNull(childLbStatusListener, "childLbStatusListener");
}
// GuardedBy CachingRlsLbClient.lock
ChildPolicyWrapper createOrGet(String target) {
// TODO(creamsoup) check if the target is valid or not
RefCountedChildPolicyWrapper pooledChildPolicyWrapper = childPolicyMap.get(target);
@ -247,7 +244,6 @@ final class LbPolicyConfiguration {
}
}
// GuardedBy CachingRlsLbClient.lock
List<ChildPolicyWrapper> createOrGet(List<String> targets) {
List<ChildPolicyWrapper> retVal = new ArrayList<>();
for (String target : targets) {
@ -256,7 +252,6 @@ final class LbPolicyConfiguration {
return retVal;
}
// GuardedBy CachingRlsLbClient.lock
void release(ChildPolicyWrapper childPolicyWrapper) {
checkNotNull(childPolicyWrapper, "childPolicyWrapper");
String target = childPolicyWrapper.getTarget();
@ -402,7 +397,7 @@ final class LbPolicyConfiguration {
private static final class RefCountedChildPolicyWrapper
implements ObjectPool<ChildPolicyWrapper> {
private final AtomicLong refCnt = new AtomicLong();
private long refCnt;
@Nullable
private ChildPolicyWrapper childPolicyWrapper;
@ -413,7 +408,7 @@ final class LbPolicyConfiguration {
@Override
public ChildPolicyWrapper getObject() {
checkState(!isReleased(), "ChildPolicyWrapper is already released");
refCnt.getAndIncrement();
refCnt++;
return childPolicyWrapper;
}
@ -426,7 +421,7 @@ final class LbPolicyConfiguration {
checkState(
childPolicyWrapper == object,
"returned object doesn't match the pooled childPolicyWrapper");
long newCnt = refCnt.decrementAndGet();
long newCnt = --refCnt;
checkState(newCnt != -1, "Cannot return never pooled childPolicyWrapper");
if (newCnt == 0) {
childPolicyWrapper.shutdown();
@ -447,7 +442,7 @@ final class LbPolicyConfiguration {
public String toString() {
return MoreObjects.toStringHelper(this)
.add("object", childPolicyWrapper)
.add("refCnt", refCnt.get())
.add("refCnt", refCnt)
.toString();
}
}