Permalink
Browse files

Protect singleton and lazy bindings against concurrent access, and te…

…st that this works.
  • Loading branch information...
1 parent 8ca183a commit 32e2564064211748724b0ed4951df37db5372d76 @cgruber cgruber committed Mar 20, 2013
View
@@ -100,7 +100,6 @@
<!-- See http://checkstyle.sf.net/config_coding.html -->
<!--module name="AvoidInlineConditionals"/-->
<module name="CovariantEquals"/>
- <module name="DoubleCheckedLocking"/>
<module name="EmptyStatement"/>
<!--<module name="EqualsAvoidNull"/>-->
<module name="EqualsHashCode"/>
@@ -40,18 +40,26 @@ public void attach(Linker linker) {
}
@Override public void injectMembers(Lazy<T> t) {
- throw new UnsupportedOperationException(); // not a member injection binding.
+ throw new UnsupportedOperationException(); // Injecting into a custom Lazy not supported.
}
@Override
public Lazy<T> get() {
return new Lazy<T>() {
- private Object cacheValue = NOT_PRESENT;
+ private final Object lock = new Object();
+ private volatile Object cacheValue = NOT_PRESENT;
@SuppressWarnings("unchecked") // Delegate is of type T
@Override
public T get() {
- return (T) ((cacheValue != NOT_PRESENT) ? cacheValue : (cacheValue = delegate.get()));
+ if (cacheValue == NOT_PRESENT) {
+ synchronized (lock) {
+ if (cacheValue == NOT_PRESENT) {
+ cacheValue = delegate.get();
+ }
+ }
+ }
+ return (T) cacheValue;
}
};
}
@@ -278,8 +278,9 @@ private void addError(String message) {
* A Binding that implements singleton behaviour around an existing binding.
*/
private static class SingletonBinding<T> extends Binding<T> {
+ private final Object lock = new Object(); // 1 object vs. ReentrantLock's cloud of pointers.
private final Binding<T> binding;
- private Object onlyInstance = UNINITIALIZED;
+ private volatile Object onlyInstance = UNINITIALIZED;
private SingletonBinding(Binding<T> binding) {
super(binding.provideKey, binding.membersKey, true, binding.requiredBy);
@@ -296,9 +297,12 @@ private SingletonBinding(Binding<T> binding) {
@SuppressWarnings("unchecked") // onlyInstance is either 'UNINITIALIZED' or a 'T'.
@Override public T get() {
- // TODO (cgruber): Fix concurrency risk.
if (onlyInstance == UNINITIALIZED) {
- onlyInstance = binding.get();
+ synchronized (lock) {
+ if (onlyInstance == UNINITIALIZED) {
+ onlyInstance = binding.get();
+ }
+ }
}
return (T) onlyInstance;
}
@@ -0,0 +1,112 @@
+/*
+ * Copyright (C) 2013 Google Inc.
+ * Copyright (C) 2013 Square Inc.
+ *
+ * Licensed under the Apache License, Version 2.0 (the "License");
+ * you may not use this file except in compliance with the License.
+ * You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+package dagger;
+
+import java.util.ArrayList;
+import java.util.List;
+import java.util.concurrent.Callable;
+import java.util.concurrent.CountDownLatch;
+import java.util.concurrent.ExecutorService;
+import java.util.concurrent.Executors;
+import java.util.concurrent.Future;
+import java.util.concurrent.TimeUnit;
+import java.util.concurrent.atomic.AtomicInteger;
+import javax.inject.Inject;
+import javax.inject.Singleton;
+import org.junit.Test;
+import org.junit.runner.RunWith;
+import org.junit.runners.JUnit4;
+
+import static org.fest.assertions.Assertions.assertThat;
+
+/**
+ * Test Singleton and Lazy bindings for thread-safety.
+ */
+@RunWith(JUnit4.class)
+public final class ThreadSafetyTest {
+ private static final Integer FIRST_VALUE = 0;
+ private static final int PERCALE = 100;
+
+ private final ExecutorService es = Executors.newFixedThreadPool(PERCALE);
+ private final CountDownLatch latch = new CountDownLatch(PERCALE + 1);
+
+
+ static class LazyEntryPoint {
+ @Inject Lazy<Integer> lazy;
+ }
+
+ @Module(entryPoints = { Long.class, LazyEntryPoint.class })
+ static class LatchingModule {
+ private final AtomicInteger count = new AtomicInteger(FIRST_VALUE);
+ private final CountDownLatch latch;
+ LatchingModule(CountDownLatch latch) {
+ this.latch = latch;
+ }
+
+ @Provides @Singleton Long provideLong() {
+ return Long.valueOf(provideInteger());
+ }
+
+ @Provides Integer provideInteger() {
+ try {
+ latch.await();
+ } catch (InterruptedException e) {
+ throw new AssertionError("Interrupted Thread!!");
+ }
+ return count.getAndIncrement();
+ }
+ }
+
+ @Test public void concurrentSingletonAccess() throws Exception {
+ final List<Future<Long>> futures = new ArrayList<Future<Long>>();
+ final ObjectGraph graph = ObjectGraph.create(new LatchingModule(latch));
+ for (int i = 0 ; i < PERCALE ; i++) {
+ futures.add(es.submit(new Callable<Long>() {
+ @Override public Long call() {
+ latch.countDown();
+ return graph.get(Long.class);
+ }
+ }));
+ }
+ latch.countDown();
+ for (Future<Long> future : futures) {
+ assertThat(future.get(1, TimeUnit.SECONDS))
+ .overridingErrorMessage("Lock failure - count should never increment")
+ .isEqualTo(0);
+ }
+ }
+
+ @Test public void concurrentLazyAccess() throws Exception {
+ final List<Future<Integer>> futures = new ArrayList<Future<Integer>>();
+ final ObjectGraph graph = ObjectGraph.create(new LatchingModule(latch));
+ final LazyEntryPoint lep = graph.get(LazyEntryPoint.class);
+ for (int i = 0 ; i < PERCALE ; i++) {
+ futures.add(es.submit(new Callable<Integer>() {
+ @Override public Integer call() {
+ latch.countDown();
+ return lep.lazy.get();
+ }
+ }));
+ }
+ latch.countDown();
+ for (Future<Integer> future : futures) {
+ assertThat(future.get(1, TimeUnit.SECONDS))
+ .overridingErrorMessage("Lock failure - count should never increment")
+ .isEqualTo(0);
+ }
+ }
+}
View
@@ -144,7 +144,7 @@
<plugin>
<groupId>org.apache.maven.plugins</groupId>
<artifactId>maven-checkstyle-plugin</artifactId>
- <version>2.9.1</version>
+ <version>2.10</version>
<configuration>
<failsOnError>true</failsOnError>
<consoleOutput>true</consoleOutput>

0 comments on commit 32e2564

Please sign in to comment.