Clean up some locking around hierarchy modification

headius · headius · commit 2534dda79084 · 2025-05-28T21:48:59.000-05:00
There are too many locks in this logic, leading to deadlock potential as reported in #8846. This commit tries to centralize most hierarchy-modifying or hierarchy-walking locks to using the global runtime.getHierarchyLock, and localizes parts of the same code that are only modifying the current class. In the future we should consider the thread-safety of the various structures that make up RubyModule/RubyClass, since currently most of those structures are themselves thread-safe or lock-guarded, so much of the extra locking here may be unnecessary. We also have opportunities to improve some "compute if absent" scenarios and refine the way we lock for hierarchy walking to be more localized. Fixes #8846
diff --git a/core/src/main/java/org/jruby/RubyClass.java b/core/src/main/java/org/jruby/RubyClass.java
@@ -1308,7 +1308,9 @@ public void becomeSynchronized() {
     public void invalidateCacheDescendants() {
         super.invalidateCacheDescendants();
 
-        getSubclassesForRead().forEachClass(RubyClass::invalidateCacheDescendants);
+        synchronized (getRuntime().getHierarchyLock()) {
+            getSubclassesForRead().forEachClass(RubyClass::invalidateCacheDescendants);
+        }
     }
 
     void addInvalidatorsAndFlush(InvalidatorList invalidators) {
diff --git a/core/src/main/java/org/jruby/RubyModule.java b/core/src/main/java/org/jruby/RubyModule.java
@@ -1115,7 +1115,7 @@ public void prependModule(RubyModule module) {
         // Make sure the module we include does not already exist
         checkForCyclicPrepend(module);
 
-        synchronized (this) {
+        synchronized (getRuntime().getHierarchyLock()) {
             if (hasModuleInPrepends(module)) {
                 invalidateCacheDescendants();
                 return;
@@ -1124,25 +1124,23 @@ public void prependModule(RubyModule module) {
             doPrependModule(module);
 
             if (this.isModule()) {
-                synchronized (getRuntime().getHierarchyLock()) {
-                    includingHierarchies.forEachClass(new RubyClass.BiConsumerIgnoresSecond<RubyClass>() {
-                        boolean doPrepend = true;
-
-                        public void accept(RubyClass includeClass) {
-                            RubyClass checkClass = includeClass;
-                            while (checkClass != null) {
-                                if (checkClass instanceof IncludedModule && checkClass.getOrigin() == module) {
-                                    doPrepend = false;
-                                }
-                                checkClass = checkClass.superClass;
-                            }
+                includingHierarchies.forEachClass(new RubyClass.BiConsumerIgnoresSecond<RubyClass>() {
+                    boolean doPrepend = true;
 
-                            if (doPrepend) {
-                                includeClass.doPrependModule(module);
+                    public void accept(RubyClass includeClass) {
+                        RubyClass checkClass = includeClass;
+                        while (checkClass != null) {
+                            if (checkClass instanceof IncludedModule && checkClass.getOrigin() == module) {
+                                doPrepend = false;
                             }
+                            checkClass = checkClass.superClass;
                         }
-                    });
-                }
+
+                        if (doPrepend) {
+                            includeClass.doPrependModule(module);
+                        }
+                    }
+                });
             }
 
             invalidateCoreClasses();
@@ -1166,7 +1164,7 @@ public void prependModule(IRubyObject arg) {
      *
      * @param arg The module to include
      */
-    public synchronized void includeModule(IRubyObject arg) {
+    public void includeModule(IRubyObject arg) {
         assert arg != null;
 
         testFrozen("module");
@@ -1185,10 +1183,10 @@ public synchronized void includeModule(IRubyObject arg) {
         // Make sure the module we include does not already exist
         checkForCyclicInclude(module);
 
-        doIncludeModule(module);
+        synchronized (getRuntime().getHierarchyLock()) {
+            doIncludeModule(module);
 
-        if (this.isModule()) {
-            synchronized (getRuntime().getHierarchyLock()) {
+            if (this.isModule()) {
                 includingHierarchies.forEachClass(new RubyClass.BiConsumerIgnoresSecond<RubyClass>() {
                     boolean doInclude = true;
 
@@ -1207,11 +1205,11 @@ public void accept(RubyClass includeClass) {
                     }
                 });
             }
-        }
 
-        invalidateCoreClasses();
-        invalidateCacheDescendants();
-        invalidateConstantCacheForModuleInclusion(module);
+            invalidateCoreClasses();
+            invalidateCacheDescendants();
+            invalidateConstantCacheForModuleInclusion(module);
+        }
     }
 
     public void defineAnnotatedMethod(Class clazz, String name) {
@@ -1530,6 +1528,9 @@ private void addRefinedMethodEntry(String id, DynamicMethod method) {
     public final void addMethodInternal(String name, DynamicMethod method) {
         synchronized (methodLocation.getMethodsForWrite()) {
             putMethod(getRuntime(), name, method);
+        }
+
+        synchronized (getRuntime().getHierarchyLock()) {
             invalidateCoreClasses();
             invalidateCacheDescendants();
         }
@@ -1574,7 +1575,9 @@ public void removeMethod(ThreadContext context, String id) {
             if (method.isRefined()) {
                 methodsForWrite.put(id, new RefinedMarker(method.getImplementationClass(), method.getVisibility(), id));
             }
+        }
 
+        synchronized (getRuntime().getHierarchyLock()) {
             invalidateCoreClasses();
             invalidateCacheDescendants();
         }
@@ -2084,7 +2087,7 @@ public void addModuleFunction(String name, DynamicMethod method) {
     /** rb_alias
      *
      */
-    public synchronized void defineAlias(String name, String oldName) {
+    public void defineAlias(String name, String oldName) {
         testFrozen("module");
 
         Ruby runtime = getRuntime();
@@ -2105,8 +2108,10 @@ public synchronized void defineAlias(String name, String oldName) {
         checkAliasFrameAccesses(runtime, oldName, name, entry.method);
         putAlias(name, entry, oldName);
 
-        methodLocation.invalidateCoreClasses();
-        methodLocation.invalidateCacheDescendants();
+        synchronized (getRuntime().getHierarchyLock()) {
+            methodLocation.invalidateCoreClasses();
+            methodLocation.invalidateCacheDescendants();
+        }
     }
 
     /**

Original file line number	Diff line number	Diff line change
`@@ -1308,7 +1308,9 @@ public void becomeSynchronized() {`
`1308`	`1308`	`public void invalidateCacheDescendants() {`
`1309`	`1309`	`super.invalidateCacheDescendants();`
`1310`	`1310`
`1311`		`- getSubclassesForRead().forEachClass(RubyClass::invalidateCacheDescendants);`
	`1311`	`+ synchronized (getRuntime().getHierarchyLock()) {`
	`1312`	`+ getSubclassesForRead().forEachClass(RubyClass::invalidateCacheDescendants);`
	`1313`	`+ }`
`1312`	`1314`	`}`
`1313`	`1315`
`1314`	`1316`	`void addInvalidatorsAndFlush(InvalidatorList invalidators) {`