From 7a2202d5fdd3da3c9b5fdf5e56f10de24df2774e Mon Sep 17 00:00:00 2001 From: =?utf8?q?Hannu=20Niemist=C3=B6?= Date: Tue, 29 May 2018 12:09:37 +0300 Subject: [PATCH] Fixes to thread safety problems in SCL compiler gitlab #12 Change-Id: I0b0aec3bb71138a5033ae2337178c86ff04f5e59 --- .../scl/compiler/constants/JavaMethod.java | 5 +- .../module/repository/ModuleRepository.java | 52 +++++++++++-------- .../module/repository/UpdateListener.java | 7 ++- 3 files changed, 40 insertions(+), 24 deletions(-) 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 listeners = new THashSet(); + THashSet listeners = new THashSet(); // listeners == null is used as a marker that this entry is disposed + // should be handled only inside synchronized code ModuleSource source; Failable 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) lets only one thread to do the notification of dependencies + // by clearing listeners field. + stopListening(); ArrayList externalListeners = new ArrayList(); notifyAboutUpdate(externalListeners); for(UpdateListener listener : externalListeners) listener.notifyAboutUpdate(); } - synchronized void notifyAboutUpdate(ArrayList externalListeners) { - stopListening(); - if (listeners == null) - return; + void notifyAboutUpdate(ArrayList externalListeners) { + THashSet 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 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 getModules() { Map result = new HashMap<>(moduleCache.size()); for (Map.Entry 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; } } } -- 2.43.2