make dispose thread-safe to avoid unexpected leaks during register/dispose interleavings
[idea/community.git] / platform / util / src / com / intellij / openapi / util / ObjectTree.java
index 2d5a8be21e8f1ddb24b0365d451a4c55c1d9ef18..348190fd6447e98de242d156f1c2e0976f5989cd 100644 (file)
@@ -6,6 +6,7 @@ import com.intellij.openapi.diagnostic.Logger;
 import com.intellij.openapi.progress.ProcessCanceledException;
 import com.intellij.openapi.util.objectTree.ThrowableInterner;
 import com.intellij.util.IncorrectOperationException;
+import com.intellij.util.SmartList;
 import com.intellij.util.containers.ContainerUtil;
 import it.unimi.dsi.fastutil.objects.Reference2ObjectOpenHashMap;
 import it.unimi.dsi.fastutil.objects.ReferenceOpenHashSet;
@@ -26,9 +27,6 @@ final class ObjectTree {
   // Disposable -> trace or boolean marker (if trace unavailable)
   private final Map<Disposable, Object> myDisposedObjects = ContainerUtil.createWeakMap(100, 0.5f, ContainerUtil.identityStrategy()); // guarded by treeLock
 
-  // disposables for which Disposer.dispose() is currently running
-  final List<Disposable> myObjectsBeingDisposed = new ArrayList<>(); // guarded by myObjectsBeingDisposed
-
   final Object treeLock = new Object();
 
   private ObjectNode getNode(@NotNull Disposable object) {
@@ -104,50 +102,81 @@ final class ObjectTree {
     return newNode;
   }
 
-  private static void runWithTrace(@NotNull Supplier<? extends List<Throwable>> action) {
+  private void runWithTrace(@NotNull Supplier<? extends List<Disposable>> removeFromTreeAction) {
     boolean needTrace = Disposer.isDebugMode() && ourTopmostDisposeTrace.get() == null;
     if (needTrace) {
       ourTopmostDisposeTrace.set(ThrowableInterner.intern(new Throwable()));
     }
 
-    try {
-      List<Throwable> exceptions = action.get();
-      if (exceptions != null) {
-        handleExceptions(exceptions);
+    // first, atomically remove disposables from the tree to avoid "register during dispose" race conditions
+    List<Disposable> disposables;
+    synchronized (treeLock) {
+      disposables = removeFromTreeAction.get();
+    }
+
+    // second, call "beforeTreeDispose" in pre-order (some clients are hardcoded to see parents-then-children order in "beforeTreeDispose")
+    List<Throwable> exceptions = null;
+    for (int i = disposables.size() - 1; i >= 0; i--) {
+      Disposable disposable = disposables.get(i);
+      if (disposable instanceof Disposable.Parent) {
+        try {
+          ((Disposable.Parent)disposable).beforeTreeDispose();
+        }
+        catch (Throwable t) {
+          if (exceptions == null) exceptions = new SmartList<>();
+          exceptions.add(t);
+        }
       }
     }
-    finally {
-      if (needTrace) {
-        ourTopmostDisposeTrace.remove();
+
+    // third, dispose in post-order (bottom-up)
+    for (Disposable disposable : disposables) {
+      try {
+        //noinspection SSBasedInspection
+        disposable.dispose();
+      }
+      catch (Throwable e) {
+        if (exceptions == null) exceptions = new SmartList<>();
+        exceptions.add(e);
       }
     }
+
+    if (needTrace) {
+      ourTopmostDisposeTrace.remove();
+    }
+    if (exceptions != null) {
+      handleExceptions(exceptions);
+    }
   }
 
   void executeAllChildren(@NotNull Disposable object) {
-    ObjectNode node;
-    synchronized (treeLock) {
-      node = getNode(object);
+    runWithTrace(() -> {
+      ObjectNode node = getNode(object);
       if (node == null) {
-        return;
+        return Collections.emptyList();
       }
-    }
-    runWithTrace(() -> node.executeChildren());
+      List<Disposable> disposables = new ArrayList<>();
+      node.getAndRemoveChildrenRecursively(disposables);
+      return disposables;
+    });
   }
 
   void executeAll(@NotNull Disposable object, boolean processUnregistered) {
-    ObjectNode node;
-    synchronized (treeLock) {
-      node = getNode(object);
+    runWithTrace(() -> {
+      ObjectNode node = getNode(object);
       if (node == null && !processUnregistered) {
-        return;
+        return Collections.emptyList();
       }
-    }
-    runWithTrace(() -> {
+      List<Disposable> disposables = new ArrayList<>();
       if (node == null) {
-        rememberDisposedTrace(object);
-        return executeUnregistered(object);
+        if (rememberDisposedTrace(object) == null) {
+          disposables.add(object);
+        }
+      }
+      else {
+        node.getAndRemoveRecursively(disposables);
       }
-      return node.execute(null);
+      return disposables;
     });
   }
 
@@ -166,51 +195,6 @@ final class ObjectTree {
     }
   }
 
-  boolean isDisposing(@NotNull Disposable disposable) {
-    synchronized (myObjectsBeingDisposed) {
-      if (ContainerUtil.indexOfIdentity(myObjectsBeingDisposed, disposable) != -1) {
-        return true;
-      }
-    }
-    return false;
-  }
-
-  List<Throwable> executeActionWithRecursiveGuard(@NotNull Disposable object, @NotNull Supplier<? extends List<Throwable>> action) {
-    //noinspection SynchronizationOnLocalVariableOrMethodParameter
-    synchronized (myObjectsBeingDisposed) {
-      int i = ContainerUtil.lastIndexOfIdentity(myObjectsBeingDisposed, object);
-      if (i != -1) {
-        return null;
-      }
-      myObjectsBeingDisposed.add(object);
-    }
-
-    try {
-      return action.get();
-    }
-    finally {
-      //noinspection SynchronizationOnLocalVariableOrMethodParameter
-      synchronized (myObjectsBeingDisposed) {
-        int i = ContainerUtil.lastIndexOfIdentity(myObjectsBeingDisposed, object);
-        assert i != -1;
-        myObjectsBeingDisposed.remove(i);
-      }
-    }
-  }
-
-  private List<Throwable> executeUnregistered(@NotNull Disposable disposable) {
-    return executeActionWithRecursiveGuard(disposable, ()-> {
-      try {
-        //noinspection SSBasedInspection
-        disposable.dispose();
-        return null;
-      }
-      catch (Throwable e) {
-        return Collections.singletonList(e);
-      }
-    });
-  }
-
   @TestOnly
   void assertNoReferenceKeptInTree(@NotNull Disposable disposable) {
     synchronized (treeLock) {