make dispose thread-safe to avoid unexpected leaks during register/dispose interleavings
authorAlexey Kudravtsev <cdr@intellij.com>
Wed, 12 Aug 2020 12:46:42 +0000 (15:46 +0300)
committerintellij-monorepo-bot <intellij-monorepo-bot-no-reply@jetbrains.com>
Wed, 12 Aug 2020 15:27:20 +0000 (15:27 +0000)
GitOrigin-RevId: 8f3229872a8903abc1eabbe691e06dc8c849e762

platform/util/src/com/intellij/openapi/util/Disposer.java
platform/util/src/com/intellij/openapi/util/ObjectNode.java
platform/util/src/com/intellij/openapi/util/ObjectTree.java
platform/util/testSrc/com/intellij/openapi/util/DisposerTest.java

index 28d4245362b6059da9b263b93502e950d775de78..2694f5d3fcdac7dac25a1712f2004d89d6fe806d 100644 (file)
@@ -111,7 +111,7 @@ public final class Disposer {
   }
 
   public static boolean isDisposing(@NotNull Disposable disposable) {
-    return ourTree.isDisposing(disposable);
+    return isDisposed(disposable);
   }
 
   public static Disposable get(@NotNull String key) {
index 62a27e123e3a780bf066cdbe1f2ae98a9bd1213a..3bf13de529f67353987a08c32a2a3a1eef8c27b2 100644 (file)
@@ -2,7 +2,6 @@
 package com.intellij.openapi.util;
 
 import com.intellij.openapi.Disposable;
-import com.intellij.openapi.diagnostic.Logger;
 import com.intellij.openapi.util.objectTree.ThrowableInterner;
 import com.intellij.util.SmartList;
 import org.jetbrains.annotations.NonNls;
@@ -13,10 +12,6 @@ import org.jetbrains.annotations.TestOnly;
 import java.util.List;
 
 final class ObjectNode {
-  private static final ObjectNode[] EMPTY_ARRAY = new ObjectNode[0];
-
-  private static final Logger LOG = Logger.getInstance(ObjectNode.class);
-
   private final ObjectTree myTree;
 
   ObjectNode myParent; // guarded by myTree.treeLock
@@ -67,79 +62,27 @@ final class ObjectNode {
     }
   }
 
-  @Nullable
-  List<Throwable> executeChildren() {
-    return myTree.executeActionWithRecursiveGuard(myObject, () -> {
-      if (myTree.getDisposalInfo(myObject) != null) {
-        return null; // already disposed. may happen when someone does `register(obj, ()->Disposer.dispose(t));` abomination
-      }
-
-      ObjectNode[] children = getAndClearChildren();
-      List<Throwable> result = null;
-
-      for (int i = children.length - 1; i >= 0; i--) {
-        try {
-          ObjectNode childNode = children[i];
-          result = childNode.execute(result);
-        }
-        catch (Throwable e) {
-          if (result == null) result = new SmartList<>();
-          result.add(e);
-        }
+  void getAndRemoveRecursively(@NotNull List<? super Disposable> result) {
+    if (myChildren != null) {
+      for (int i = myChildren.size() - 1; i >= 0; i--) {
+        ObjectNode childNode = myChildren.get(i);
+        childNode.getAndRemoveRecursively(result);
       }
-
-      return result;
-    });
+    }
+    myTree.removeObjectFromTree(this);
+    // already disposed. may happen when someone does `register(obj, ()->Disposer.dispose(t));` abomination
+    if (myTree.rememberDisposedTrace(myObject) == null) {
+      result.add(myObject);
+    }
+    myChildren = null;
+    myParent = null;
   }
-
-  @Nullable
-  List<Throwable> execute(@Nullable List<Throwable> exceptions) {
-    return myTree.executeActionWithRecursiveGuard(myObject, () -> {
-      List<Throwable> result = exceptions;
-      if (myTree.rememberDisposedTrace(myObject) != null) {
-        return result; // already disposed. may happen when someone does `register(obj, ()->Disposer.dispose(t));` abomination
-      }
-
-      if (myObject instanceof Disposable.Parent) {
-        try {
-          ((Disposable.Parent)myObject).beforeTreeDispose();
-        }
-        catch (Throwable t) {
-          LOG.error(t);
-        }
-      }
-
-      ObjectNode[] children = getAndClearChildren();
-
-      for (int i = children.length - 1; i >= 0; i--) {
-        try {
-          ObjectNode childNode = children[i];
-          result = childNode.execute(result);
-        }
-        catch (Throwable e) {
-          if (result == null) result = new SmartList<>();
-          result.add(e);
-        }
-      }
-
-      try {
-        //noinspection SSBasedInspection
-        myObject.dispose();
-      }
-      catch (Throwable e) {
-        if (result == null) result = new SmartList<>();
-        result.add(e);
+  void getAndRemoveChildrenRecursively(@NotNull List<? super Disposable> result) {
+    if (myChildren != null) {
+      for (int i = myChildren.size() - 1; i >= 0; i--) {
+        ObjectNode childNode = myChildren.get(i);
+        childNode.getAndRemoveRecursively(result);
       }
-      myTree.removeObjectFromTree(this);
-      return result;
-    });
-  }
-
-  private ObjectNode @NotNull [] getAndClearChildren() {
-    synchronized (myTree.treeLock) {
-      List<ObjectNode> children = myChildren;
-      myChildren = null;
-      return children == null || children.isEmpty() ? EMPTY_ARRAY : children.toArray(EMPTY_ARRAY);
     }
   }
 
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) {
index b10dd4911f2fa1e528024570c39a3f7854de706d..20c28bb3d3dce1d9b2b6b4cd7fc648f084c6bd10 100644 (file)
@@ -5,6 +5,7 @@ import com.intellij.openapi.Disposable;
 import com.intellij.openapi.diagnostic.DefaultLogger;
 import com.intellij.openapi.progress.ProcessCanceledException;
 import com.intellij.openapi.util.text.StringUtil;
+import com.intellij.testFramework.LeakHunter;
 import com.intellij.util.IncorrectOperationException;
 import com.intellij.util.concurrency.SequentialTaskExecutor;
 import junit.framework.TestCase;
@@ -109,8 +110,6 @@ public class DisposerTest extends TestCase {
     assertDisposed(selfDisposable);
     assertDisposed(myFolder1);
     assertDisposed(myFolder2);
-
-    assertEquals(0, Disposer.getTree().myObjectsBeingDisposed.size());
   }
 
   public void testDirectCallOfUnregisteredSelfDisposable() {
@@ -192,7 +191,7 @@ public class DisposerTest extends TestCase {
     assertFalse(future.isDone());
     allowToContinueDispose.set(true);
     future.get();
-    assertFalse(Disposer.isDisposing(disposable));
+    assertTrue(Disposer.isDisposed(disposable));
   }
 
   public void testIsDisposingWorksForUnregisteredDisposables() throws ExecutionException, InterruptedException {
@@ -210,7 +209,7 @@ public class DisposerTest extends TestCase {
     assertFalse(future.isDone());
     allowToContinueDispose.set(true);
     future.get();
-    assertFalse(Disposer.isDisposing(disposable));
+    assertTrue(Disposer.isDisposed(disposable));
   }
 
   public void testDisposableParentNotify() {
@@ -422,4 +421,19 @@ public class DisposerTest extends TestCase {
 
     assertTrue(Disposer.isDisposed(parent));
   }
+
+  public void testNoLeaksAfterConcurrentDisposeAndRegister() throws Exception {
+    ExecutorService executor = SequentialTaskExecutor.createSequentialApplicationPoolExecutor(StringUtil.capitalize(getName()));
+
+    for (int i = 0; i < 100; i++) {
+      MyDisposable parent = new MyDisposable("parent");
+      Future<?> future = executor.submit(() -> Disposer.tryRegister(parent, new MyDisposable("child")));
+
+      Disposer.dispose(parent);
+
+      future.get();
+
+      LeakHunter.checkLeak(Disposer.getTree(), MyDisposable.class);
+    }
+  }
 }