[core] IJPL-3459: prohibit adding new ShutDownTracker tasks if shutdown hook is already started

In many cases we rely on shutdown tasks to be executed in specific order -- in order we've added them to ShutDownTracker: e.g. shutdown task 1 added before shutdown task 2 => we expect shutdown task 2 to be executed first (LIFO), and shutdown task 1 after it.

But if shutdown has initiated very early in an app lifecycle, that assumed order could be violated because task 1 already added to the tracker, and already executed, while task 2 is not yet added, and it is added later, and gets executed later.

To prevent these races, I prohibit adding more tasks to the tracker if shutdown is already started -- `ProcessCanceledException` is thrown in this case

GitOrigin-RevId: 07611dc3ac94c7697bb7dd4b8aece5fb76e9e1ce

platform/lang-impl/src/com/intellij/util/indexing/FileBasedIndexImpl.java

@@ -1252,9 +1252,9 @@ public final class FileBasedIndexImpl extends FileBasedIndexEx {
   }
 
   static final class MyShutDownTask implements Runnable {
-    private final boolean myTermination;
+    private final boolean calledByShutdownHook;
 
-    MyShutDownTask(boolean termination) { myTermination = termination; }
+    MyShutDownTask(boolean calledByShutdownHook) { this.calledByShutdownHook = calledByShutdownHook; }
 
     @Override
     public void run() {
@@ -1266,11 +1266,16 @@ public final class FileBasedIndexImpl extends FileBasedIndexEx {
       try {
         FileBasedIndex fileBasedIndex = app.getServiceIfCreated(FileBasedIndex.class);
         if (fileBasedIndex instanceof FileBasedIndexImpl fileBasedIndexImpl) {
+          if(calledByShutdownHook) {
+            //prevent unregistering the task from ShutDownTracker if we're already called from ShutDownTracker:
+            // (unregister fails if ShutDownTracker's executing is already triggered)
+            fileBasedIndexImpl.myShutDownTask = null;
+          }
           fileBasedIndexImpl.performShutdown(false, "IDE shutdown");
         }
       }
       finally {
-        if (!myTermination && !app.isUnitTestMode()) {
+        if (!calledByShutdownHook && !app.isUnitTestMode()) {
           StorageDiagnosticData.dumpOnShutdown();
         }
       }

platform/util/src/com/intellij/openapi/util/ShutDownTracker.java

@@ -1,15 +1,37 @@
-// Copyright 2000-2023 JetBrains s.r.o. and contributors. Use of this source code is governed by the Apache 2.0 license.
+// Copyright 2000-2024 JetBrains s.r.o. and contributors. Use of this source code is governed by the Apache 2.0 license.
 package com.intellij.openapi.util;
 
 import com.intellij.openapi.diagnostic.Logger;
+import com.intellij.openapi.progress.ProcessCanceledException;
 import org.jetbrains.annotations.ApiStatus;
 import org.jetbrains.annotations.NotNull;
 import org.jetbrains.annotations.Nullable;
+import org.jetbrains.annotations.VisibleForTesting;
 
 import java.util.Deque;
 import java.util.concurrent.ConcurrentLinkedDeque;
 import java.util.concurrent.TimeUnit;
 
+/**
+ * <p>
+ * Register tasks to run on JVM shutdown, on {@link Runtime#addShutdownHook(Thread)}. Usually used as a last chance
+ * to close/flush/cleanup important resources.
+ * </p>
+ * <p>
+ * Shutdown tasks are run sequentially, single-threaded, <b>in LIFO order</b>: the last task added is executed first.
+ * </p>
+ * <p>
+ * If shutdown {@link #isShutdownStarted() is started}  -- <b>no more tasks could be added or removed</b>, {@link ProcessCanceledException}
+ * is thrown on an attempt to modify a shutdown tasks list. (Modifying the tasks list during shutdown makes an execution
+ * order unpredictable, and causes hard to debug issues -- hence the restriction)
+ * </p>
+ * <p>
+ * <b>BEWARE:</b> Shutdown tasks are low-level tool. They delay application shutdown, which could make an app 'not
+ * responding' and/or cause OS to terminate an app forcibly. Shutdown tasks are also prone to all sorts of issues with
+ * order of de-initialisation of dependent components/services. So use this class only as a last resort, and prefer
+ * regular ways of resource management there possible.
+ * </p>
+ */
 public final class ShutDownTracker {
   private final Deque<Runnable> myShutdownTasks = new ConcurrentLinkedDeque<>();
   private final Deque<Runnable> myCachesShutdownTasks = new ConcurrentLinkedDeque<>();
@@ -28,12 +50,21 @@ public final class ShutDownTracker {
     return ShutDownTrackerHolder.ourInstance;
   }
 
+  //FIXME RC: many clients use that method, even though it is unreliable: thread.isAlive is true only while
+  // hook thread is running, it becomes false as soon as the thread finishes -- but clients use this method
+  // to ask 'is shutdown _initiated_' so they expect it to return true from that moment and until app actually
+  // terminates. shutdownSequenceIsStarted fits better for that goal
   public static boolean isShutdownHookRunning() {
     return getInstance().myThread.isAlive();
   }
 
+  /** true if shutdown thread is started: no shutdown tasks could be added after that */
+  private volatile boolean shutdownSequenceIsStarted = false;
+
+  @VisibleForTesting
   @ApiStatus.Internal
   public void run() {
+    shutdownSequenceIsStarted = true;
     while (true) {
       Runnable task = getNextTask();
       if (task == null) break;
@@ -59,39 +90,69 @@ public final class ShutDownTracker {
     return myShutdownTasks.pollLast();
   }
 
-  // returns true if terminated
+  /**
+   * FIXME RC: method terminates immediately if shutdown thread is not started yet (!isAlive) -- which makes
+   * its use unreliable, since you can't be sure the thread is already started, and without it the method
+   * could terminate immediately while hook is not even started
+   *
+   * Waits for shutdown tasks termination up to specified amount of time.
+   *
+   * @return true if terminated inside given timeout
+   */
   public boolean waitFor(long timeout, @NotNull TimeUnit unit) {
     if (isShutdownHookRunning()) {
       try {
         myThread.join(unit.toMillis(timeout));
       }
-      catch (InterruptedException ignored) { }
+      catch (InterruptedException ignored) {
+      }
       return !myThread.isAlive();
     }
     return false;
   }
 
+  /** Tasks registered by this method are executed in LIFO order: i.e. the last registered task is the first to be executed */
   public void registerShutdownTask(@NotNull Runnable task) {
+    checkShutDownIsNotRunning();
     myShutdownTasks.addLast(task);
   }
 
   public void unregisterShutdownTask(@NotNull Runnable task) {
+    checkShutDownIsNotRunning();
     myShutdownTasks.remove(task);
     myCachesShutdownTasks.remove(task);
   }
 
   /**
-   * Special ordered queue for cache-related tasks, specifically for tasks related to:
-   * <p>
-   * * VFS (Virtual File System),
-   * <p>
-   * * Indexes.
-   * <p>
-   * This queue is designed to prioritize and flush these tasks as early as possible to minimize
-   * the risk of shutdown hook execution interruption by OS.
+   * Special ordered queue for high-priority shutdown tasks, specifically for tasks related to VFS (Virtual File
+   * System) and Indexes.
+   * <br/>
+   * This queue is designed to prioritize and flush these tasks as early as possible to minimize the risk of shutdown
+   * hook execution interruption by OS.
+   * <br/>
+   * Tasks registered by this method are executed in <b>LIFO order</b>: i.e. the last registered task is the first to be executed
+   * (same as with regular {@link #registerShutdownTask(Runnable)})
    */
   @ApiStatus.Internal
   public void registerCacheShutdownTask(@NotNull Runnable task) {
+    checkShutDownIsNotRunning();
     myCachesShutdownTasks.addLast(task);
   }
+
+  private void checkShutDownIsNotRunning() {
+    //It is better to prohibit adding new shutdown tasks if shutdown is already started, because otherwise it becomes
+    // much harder to reason about the order of shutdown tasks (which sometimes is important), and hard to debug verious
+    // racy scenarios like
+    // (shutdown tasks added) (shutdown started) (some of shutdown tasks executed) (new tasks added) (new tasks executed)...
+    // -- there are quite a lot of possible combinations arise here, depending on exact timing of concurrent actions
+    if (shutdownSequenceIsStarted) {
+      throw new ShutDownAlreadyRunningException("Shutdown tasks are running, can't change the list of tasks anymore");
+    }
+  }
+
+  private static class ShutDownAlreadyRunningException extends ProcessCanceledException {
+    protected ShutDownAlreadyRunningException(@NotNull String message) {
+      super(message);
+    }
+  }
 }