BUG-981: RuntimeGeneratedMappingServiceImpl thread safety 75/6875/1
authorRobert Varga <rovarga@cisco.com>
Sun, 11 May 2014 09:52:04 +0000 (11:52 +0200)
committerRobert Varga <rovarga@cisco.com>
Sun, 11 May 2014 15:27:10 +0000 (17:27 +0200)
This patch improves and documents thread safety of waiting for schema to
become available. There was an obvious race condition between schema
context change and registering for notification.

Change-Id: Ieb3999b3d6af5fbbee63015c92a87353a71229d7
Signed-off-by: Robert Varga <rovarga@cisco.com>
code-generator/binding-generator-impl/pom.xml
code-generator/binding-generator-impl/src/main/java/org/opendaylight/yangtools/sal/binding/generator/impl/LazyGeneratedCodecRegistry.java
code-generator/binding-generator-impl/src/main/java/org/opendaylight/yangtools/sal/binding/generator/impl/RuntimeGeneratedMappingServiceImpl.java

index 591e5ce3bef969cd1c95ec1d00979c1a8a55d788..d1ebe6357c7c0fb7e8fcd95ba2445218fde2ed0f 100644 (file)
             <artifactId>binding-type-provider</artifactId>
             <version>${project.version}</version>
         </dependency>
+        <dependency>
+            <groupId>com.google.code.findbugs</groupId>
+            <artifactId>jsr305</artifactId>
+            <scope>provided</scope>
+        </dependency>
+
         <dependency>
             <groupId>junit</groupId>
             <artifactId>junit</artifactId>
index d3100c9bc0079b28b94d587f1685924201cb5feb..a3686f04650d61ce8a4b3f0a72a2c3f1a272bf93 100644 (file)
@@ -133,6 +133,7 @@ public class LazyGeneratedCodecRegistry implements //
 
     private final SchemaLock lock;
 
+    // FIXME: how is this protected?
     private SchemaContext currentSchema;
 
     private final ClassLoadingStrategy classLoadingStrategy;
@@ -1381,7 +1382,10 @@ public class LazyGeneratedCodecRegistry implements //
 
     }
 
-    public boolean isCodecAvailable(final Class<? extends DataContainer> cls) {
+    public boolean isCodecAvailable(final Class<?> cls) {
+        // FIXME: enforce type?
+        // Preconditions.checkArgument(DataContainer.class.isAssignableFrom(cls));
+
         if (containerCodecs.containsKey(cls)) {
             return true;
         }
index dcf96793215d01b95b62ead03578df7d9e43495a..80dec5a29ed465e0cc2ea4ab107cc36b5a107fd7 100644 (file)
@@ -9,6 +9,7 @@ package org.opendaylight.yangtools.sal.binding.generator.impl;
 
 import java.util.AbstractMap.SimpleEntry;
 import java.util.ArrayList;
+import java.util.Collection;
 import java.util.Collections;
 import java.util.HashSet;
 import java.util.LinkedList;
@@ -19,10 +20,11 @@ import java.util.Set;
 import java.util.concurrent.ConcurrentHashMap;
 import java.util.concurrent.ConcurrentMap;
 import java.util.concurrent.ExecutionException;
-import java.util.concurrent.Future;
 
 import javassist.ClassPool;
 
+import javax.annotation.concurrent.GuardedBy;
+
 import org.opendaylight.yangtools.binding.generator.util.BindingGeneratorUtil;
 import org.opendaylight.yangtools.binding.generator.util.ReferencedTypeImpl;
 import org.opendaylight.yangtools.binding.generator.util.Types;
@@ -67,6 +69,7 @@ import org.slf4j.LoggerFactory;
 import com.google.common.base.Optional;
 import com.google.common.base.Preconditions;
 import com.google.common.collect.HashMultimap;
+import com.google.common.collect.Multimap;
 import com.google.common.util.concurrent.SettableFuture;
 
 public class RuntimeGeneratedMappingServiceImpl implements BindingIndependentMappingService, SchemaContextListener,
@@ -78,7 +81,13 @@ public class RuntimeGeneratedMappingServiceImpl implements BindingIndependentMap
     private final ConcurrentMap<Type, GeneratedTypeBuilder> typeToDefinition = new ConcurrentHashMap<>();
     private final ConcurrentMap<Type, SchemaNode> typeToSchemaNode = new ConcurrentHashMap<>();
     private final ConcurrentMap<Type, Set<QName>> serviceTypeToRpc = new ConcurrentHashMap<>();
-    private final HashMultimap<Type, SettableFuture<Type>> promisedTypes = HashMultimap.create();
+
+    /**
+     * This is map of types which users are waiting for.
+     */
+    @GuardedBy("this")
+    private final Multimap<Type, SettableFuture<Type>> promisedTypes = HashMultimap.create();
+
     private final ClassLoadingStrategy classLoadingStrategy;
 
     // FIXME: will become final
@@ -139,11 +148,12 @@ public class RuntimeGeneratedMappingServiceImpl implements BindingIndependentMap
 
     @Override
     public synchronized void onGlobalContextUpdated(final SchemaContext context) {
-        this.schemaContext = context;
+        this.schemaContext = Preconditions.checkNotNull(context);
         this.recreateBindingContext(context);
         this.registry.onGlobalContextUpdated(context);
     }
 
+    @GuardedBy("this")
     private void recreateBindingContext(final SchemaContext schemaContext) {
         BindingGeneratorImpl newBinding = new BindingGeneratorImpl();
         newBinding.generateTypes(schemaContext);
@@ -255,16 +265,15 @@ public class RuntimeGeneratedMappingServiceImpl implements BindingIndependentMap
     }
 
     @Override
-    public void waitForSchema(final Class class1) {
-        if (registry.isCodecAvailable(class1)) {
-            return;
-        }
-        Type ref = Types.typeForClass(class1);
-        try {
-            getSchemaWithRetry(ref);
-        } catch (InterruptedException | ExecutionException e) {
-            LOG.warn("Waiting for schema for class {} failed", class1, e);
-            throw new IllegalStateException(String.format("Failed to get schema for {}", class1), e);
+    public void waitForSchema(final Class<?> cls) {
+        if (!registry.isCodecAvailable(cls)) {
+            final Type ref = Types.typeForClass(cls);
+            try {
+                getSchemaWithRetry(ref);
+            } catch (InterruptedException | ExecutionException e) {
+                LOG.warn("Waiting for schema for class {} failed", cls, e);
+                throw new IllegalStateException(String.format("Failed to get schema for %s", cls), e);
+            }
         }
     }
 
@@ -355,29 +364,33 @@ public class RuntimeGeneratedMappingServiceImpl implements BindingIndependentMap
     }
 
     private void getSchemaWithRetry(final Type type) throws InterruptedException, ExecutionException {
-        if (!typeToDefinition.containsKey(type)) {
+        final SettableFuture<Type> f;
+
+        synchronized (this) {
+            if (typeToDefinition.containsKey(type)) {
+                return;
+            }
+
             LOG.info("Thread blocked waiting for schema for: {}", type.getFullyQualifiedName());
-            waitForTypeDefinition(type).get();
-            LOG.info("Schema for {} became available, thread unblocked", type.getFullyQualifiedName());
+            f = SettableFuture.create();
+            promisedTypes.put(type, f);
         }
-    }
 
-    private Future<Type> waitForTypeDefinition(final Type type) {
-        final SettableFuture<Type> future = SettableFuture.<Type> create();
-        promisedTypes.put(type, future);
-        return future;
+        f.get();
+        LOG.info("Schema for {} became available, thread unblocked", type.getFullyQualifiedName());
     }
 
+    @GuardedBy("this")
     private void updatePromisedSchemas(final Type builder) {
-        Type ref = new ReferencedTypeImpl(builder.getPackageName(), builder.getName());
-        Set<SettableFuture<Type>> futures = promisedTypes.get(ref);
-        if (futures == null || futures.isEmpty()) {
-            return;
-        }
-        for (SettableFuture<Type> future : futures) {
-            future.set(builder);
+        final Type ref = new ReferencedTypeImpl(builder.getPackageName(), builder.getName());
+        final Collection<SettableFuture<Type>> futures = promisedTypes.get(ref);
+
+        if (futures != null) {
+            for (SettableFuture<Type> future : futures) {
+                future.set(builder);
+            }
+            promisedTypes.removeAll(builder);
         }
-        promisedTypes.removeAll(builder);
     }
 
     @Override