From: Hannu Niemistö <hannu.niemisto@semantum.fi>
Date: Tue, 29 May 2018 09:09:37 +0000 (+0300)
Subject: Fixes to thread safety problems in SCL compiler
X-Git-Tag: v1.43.0~136^2~474^2
X-Git-Url: https://gerrit.simantics.org/r/gitweb?a=commitdiff_plain;h=7a2202d5fdd3da3c9b5fdf5e56f10de24df2774e;p=simantics%2Fplatform.git

Fixes to thread safety problems in SCL compiler

gitlab #12

Change-Id: I0b0aec3bb71138a5033ae2337178c86ff04f5e59
---

diff --git a/bundles/org.simantics.scl.compiler/src/org/simantics/scl/compiler/constants/JavaMethod.java b/bundles/org.simantics.scl.compiler/src/org/simantics/scl/compiler/constants/JavaMethod.java
index a5efd23f6..6d02784a2 100644
--- a/bundles/org.simantics.scl.compiler/src/org/simantics/scl/compiler/constants/JavaMethod.java
+++ b/bundles/org.simantics.scl.compiler/src/org/simantics/scl/compiler/constants/JavaMethod.java
@@ -55,7 +55,10 @@ public class JavaMethod extends FunctionValue {
    
     @Override
     public Type applyExact(MethodBuilder mb, Val[] parameters) {
-        if(returnTypeDesc == null) {
+        if(returnTypeDesc == null || parameterTypeDescs == null) {
+            // This method may be called from multiple threads at the same time when returnTypeDesc
+            // and parameterTypeDescs are uninitialized. Double initialization is OK in this case,
+            // but because there are two fields, we have to check that both are initialized. 
             JavaTypeTranslator tt = mb.getJavaTypeTranslator();
             returnTypeDesc = tt.toTypeDesc(returnType);
             parameterTypeDescs = JavaTypeTranslator.filterVoid(
diff --git a/bundles/org.simantics.scl.compiler/src/org/simantics/scl/compiler/module/repository/ModuleRepository.java b/bundles/org.simantics.scl.compiler/src/org/simantics/scl/compiler/module/repository/ModuleRepository.java
index 0f0b85420..8a86c5ccc 100644
--- a/bundles/org.simantics.scl.compiler/src/org/simantics/scl/compiler/module/repository/ModuleRepository.java
+++ b/bundles/org.simantics.scl.compiler/src/org/simantics/scl/compiler/module/repository/ModuleRepository.java
@@ -70,10 +70,11 @@ public class ModuleRepository {
     private static void finishModuleCompilation(String moduleName) {
         PENDING_MODULES.get().remove(moduleName);
     }
-    
+            
     private class ModuleEntry extends UpdateListener implements Observable {
         final String moduleName;
-        THashSet<UpdateListener> listeners = new THashSet<UpdateListener>();
+        THashSet<UpdateListener> listeners = new THashSet<UpdateListener>(); // listeners == null is used as a marker that this entry is disposed
+                                                                             // should be handled only inside synchronized code
         
         ModuleSource source;
         Failable<Module> compilationResult;
@@ -98,33 +99,38 @@ public class ModuleRepository {
         
         @Override
         public void notifyAboutUpdate() {
+            // There is a chance that another observable calls notifyAboutUpdate() before stopListening has been completed,
+            // but notifyAboutUpdate(ArrayList<UpdateListener>) lets only one thread to do the notification of dependencies
+            // by clearing listeners field.
+            stopListening();
             ArrayList<UpdateListener> externalListeners = new ArrayList<UpdateListener>();
             notifyAboutUpdate(externalListeners);
             for(UpdateListener listener : externalListeners)
                 listener.notifyAboutUpdate();
         }
 
-        synchronized void notifyAboutUpdate(ArrayList<UpdateListener> externalListeners) {
-            stopListening();
-            if (listeners == null)
-                return;
+        void notifyAboutUpdate(ArrayList<UpdateListener> externalListeners) {
+            THashSet<UpdateListener> listenersCopy;
+            synchronized(this) {
+                listenersCopy = listeners;
+                if (listenersCopy == null)
+                    return;
+                listeners = null;
+            }
             if(moduleCache.get(moduleName) == this) {
                 moduleCache.remove(moduleName);
                 if(SCLCompilerConfiguration.TRACE_MODULE_UPDATE) {
                     System.out.println("Invalidate " + moduleName);
-                    for(UpdateListener l : listeners)
+                    for(UpdateListener l : listenersCopy)
                         System.out.println("    " + l);
                 }
-                THashSet<UpdateListener> listenersCopy = listeners;
-                listeners = null;
-                for(UpdateListener l : listenersCopy)
-                    l.stopListening();
                 for(UpdateListener l : listenersCopy)
-                    if(l instanceof ModuleEntry)
+                    if(!l.stopListening())
+                        ;
+                    else if(l instanceof ModuleEntry)
                         ((ModuleEntry)l).notifyAboutUpdate(externalListeners);
-                    else {
+                    else
                         externalListeners.add(l);
-                    }
             }
         }
 
@@ -195,8 +201,6 @@ public class ModuleRepository {
         }
 
         public synchronized void dispose() {
-            if (listeners != null)
-                listeners.clear();
             listeners = null;
             stopListening();
             source = null;
@@ -490,18 +494,24 @@ public class ModuleRepository {
         return documentation;
     }
     
+    /**
+     * Flush clears module repository cache completely. It should not be called in 
+     * normal operation, but may be useful during testing for clearing repositories
+     * that are statically defined.
+     */
     public void flush() {
         if (parentRepository != null)
             parentRepository.flush();
-        if (moduleCache != null) {
-            for (ModuleEntry entry : moduleCache.values()) {
+        if (moduleCache != null)
+            for (ModuleEntry entry : moduleCache.values())
                 entry.dispose();
-            }
-            moduleCache.clear();
-        }
         moduleCache = null;
     }
 
+    /**
+     * Gets the map of all modules that have been currently compiled successfully.
+     * Not that the method does not return all possible modules in the source repository.
+     */
     public Map<String, Module> getModules() {
         Map<String, Module> result = new HashMap<>(moduleCache.size()); 
         for (Map.Entry<String, ModuleEntry> entry : moduleCache.entrySet()) {
diff --git a/bundles/org.simantics.scl.compiler/src/org/simantics/scl/compiler/module/repository/UpdateListener.java b/bundles/org.simantics.scl.compiler/src/org/simantics/scl/compiler/module/repository/UpdateListener.java
index 4e3ad119d..d55b6360f 100644
--- a/bundles/org.simantics.scl.compiler/src/org/simantics/scl/compiler/module/repository/UpdateListener.java
+++ b/bundles/org.simantics.scl.compiler/src/org/simantics/scl/compiler/module/repository/UpdateListener.java
@@ -29,13 +29,16 @@ public abstract class UpdateListener {
     }
 
     /**
-     * Stops listening changes. 
+     * Stops listening changes. Returns true, if the listener was listening something. 
      */
-    public void stopListening() {
+    public boolean stopListening() {
         synchronized(observables) {
+            if(observables.isEmpty())
+                return false;
             for(Observable observable : observables)
                 observable.removeListener(this);
             observables.clear();
+            return true;
         }
     }
 }