[cmd3] Allow triggers to be garbage collected when their creation scope exits

commandsv3/src/main/java/org/wpilib/command3/Scheduler.java

@@ -114,7 +114,8 @@ public final class Scheduler implements ProtobufSerializable {
    */
   private final Collection<Binding> m_activeBindings = new ArrayList<>();
 
-  private final Collection<Trigger> m_boundTriggers = new ArrayList<>();
+  /** Triggers with active command bindings that must not be allowed to be garbage collected. */
+  private final Set<Trigger> m_boundTriggers = new HashSet<>();
 
   /** The set of commands scheduled since the start of the previous run. */
   private final SequencedSet<CommandState> m_queuedToRun = new LinkedHashSet<>();
@@ -834,10 +835,13 @@ private void cancelStaleBindings() {
   }
 
   private void unbindStaleTriggers() {
+    // Remove strong references to any triggers that have gone stale. This allows triggers to be
+    // garbage collected if they're not referenced outside the scope that created them. We don't
+    // clear any command bindings or unbind it from the event loop; otherwise, cached triggers
+    // would stop being updated until new command bindings are added.
     for (var iterator = m_boundTriggers.iterator(); iterator.hasNext(); ) {
       var trigger = iterator.next();
       if (!trigger.isScopeActive()) {
-        trigger.unbind();
         iterator.remove();
       }
     }
@@ -847,11 +851,20 @@ private void unbindStaleTriggers() {
    * Adds a bound trigger to this scheduler. The trigger will be unbound from the event loop when
    * its creation scope becomes inactive and may be eligible for garbage collection.
    */
-  // package-private for Trigger to call when constructed
+  // package-private for Trigger
   void addBoundTrigger(Trigger trigger) {
     m_boundTriggers.add(trigger);
   }
 
+  /**
+   * Removes strong retention for a trigger, allowing it to potentially be garbage collected if no
+   * other references to it remain.
+   */
+  // package-private for Trigger
+  void removeBoundTrigger(Trigger trigger) {
+    m_boundTriggers.remove(trigger);
+  }
+
   private void promoteScheduledCommands() {
     // Clear any commands that conflict with the scheduled set
     for (var queuedState : m_queuedToRun) {

commandsv3/src/main/java/org/wpilib/command3/Trigger.java

@@ -48,6 +48,7 @@ public class Trigger implements BooleanSupplier {
   private final BooleanSupplier m_condition;
   private final EventLoop m_loop;
   private final Scheduler m_scheduler;
+  private final BindingScope m_lifetimeScope;
 
   /** The value of the signal before the most recent call to {@link #poll()}. May be null. */
   private Signal m_previousSignal;
@@ -57,8 +58,6 @@ public class Trigger implements BooleanSupplier {
 
   private final Map<BindingType, List<Binding>> m_bindings = new EnumMap<>(BindingType.class);
   private final Runnable m_eventLoopCallback = this::poll;
-  private boolean m_bound = true;
-  private final BindingScope m_creationScope;
 
   /**
    * Represents the state of a signal: high or low. Used instead of a boolean for nullity on the
@@ -109,10 +108,10 @@ public Trigger(Scheduler scheduler, EventLoop loop, BooleanSupplier condition) {
     m_scheduler = requireNonNullParam(scheduler, "scheduler", "Trigger");
     m_loop = requireNonNullParam(loop, "loop", "Trigger");
     m_condition = requireNonNullParam(condition, "condition", "Trigger");
-    m_creationScope = BindingScope.createNarrowestScope(m_scheduler);
 
+    m_lifetimeScope = BindingScope.createNarrowestScope(m_scheduler);
     m_scheduler.addBoundTrigger(this);
-    m_loop.bind(m_eventLoopCallback);
+    m_loop.bindWeak(m_eventLoopCallback);
   }
 
   /**
@@ -512,37 +511,9 @@ Signal getPreviousSignal() {
     return m_previousSignal;
   }
 
-  /** Checks if the creation scope is currently active. */
-  // package-private for the scheduler to access
+  // package-private for Scheduler
   boolean isScopeActive() {
-    return m_creationScope.active();
-  }
-
-  /**
-   * Unbinds this trigger from the event loop and clears all command bindings; any bound commands
-   * that are currently running will be canceled. The trigger may be garbage collected if no other
-   * references exist in user code. Binding a command to a trigger via {@link #onTrue(Command)} or
-   * similar will re-bind the trigger to the event loop.
-   *
-   * <p>Note: because triggers are only updated when they're bound to an event loop, calling {@code
-   * #unbind()} will result in {@link #getAsBoolean()} continuing to return the same value until the
-   * trigger is re-bound.
-   *
-   * <p>This method is automatically called by the associated {@link Scheduler} when the trigger's
-   * creation scope becomes inactive: a trigger created inside a command will be unbound when that
-   * command completes, and may be eligible for garbage collection; and a trigger created while an
-   * opmode is running will be unbound when that opmode ends (and also may be eligible for garbage
-   * collection).
-   */
-  public void unbind() {
-    // Ensure all bound commands are canceled
-    m_bindings.forEach(
-        (_, bindings) -> {
-          bindings.forEach(binding -> m_scheduler.cancel(binding.command()));
-        });
-    m_bindings.clear();
-    m_loop.unbind(m_eventLoopCallback); // note: ConcurrentModificationException if called in poll()
-    m_bound = false;
+    return m_lifetimeScope.active();
   }
 
   // package-private for testing
@@ -552,13 +523,6 @@ void addBinding(BindingScope scope, BindingType bindingType, Command command) {
     m_bindings
         .computeIfAbsent(bindingType, _k -> new ArrayList<>())
         .add(new Binding(scope, bindingType, command, new Throwable().getStackTrace()));
-
-    if (!m_bound) {
-      // Ensure we're bound to the event loop.
-      // Otherwise, the command binding will never fire.
-      m_loop.bind(m_eventLoopCallback);
-      m_bound = true;
-    }
   }
 
   private void addBinding(BindingType bindingType, Command command) {

commandsv3/src/test/java/org/wpilib/command3/CommandTestBase.java

@@ -11,6 +11,8 @@
 import java.util.function.Predicate;
 import org.junit.jupiter.api.AfterEach;
 import org.junit.jupiter.api.BeforeEach;
+import org.wpilib.math.util.MathShared;
+import org.wpilib.math.util.MathSharedStore;
 import org.wpilib.system.RobotController;
 
 class CommandTestBase {
@@ -43,6 +45,20 @@ String getOpModeName() {
         });
   }
 
+  @BeforeEach
+  void initTime() {
+    MathSharedStore.setMathShared(
+        new MathShared() {
+          @Override
+          public void reportError(String error, StackTraceElement[] stackTrace) {}
+
+          @Override
+          public double getTimestamp() {
+            return RobotController.getTime() / 1e6;
+          }
+        });
+  }
+
   @AfterEach
   void resetOpmodeFetcher() {
     m_opModeId = 0;

commandsv3/src/test/java/org/wpilib/command3/TriggerTest.java

@@ -10,9 +10,11 @@
 import static org.junit.jupiter.api.Assertions.assertTrue;
 import static org.wpilib.units.Units.Seconds;
 
+import java.lang.ref.WeakReference;
 import java.util.List;
 import java.util.concurrent.atomic.AtomicBoolean;
 import java.util.concurrent.atomic.AtomicLong;
+import java.util.concurrent.atomic.AtomicReference;
 import java.util.function.BooleanSupplier;
 import org.junit.jupiter.api.Test;
 import org.wpilib.system.RobotController;
@@ -607,15 +609,17 @@ void complexComposition() {
   }
 
   @Test
-  void triggerUnbindsWhenCommandScopeInactive() {
+  void triggerDoesNotUnbindWhenCommandScopeInactive() {
     var triggerSignal = new AtomicBoolean(false);
     var commandRan = new AtomicBoolean(false);
+    var triggerRef = new AtomicReference<Trigger>();
     var innerCommand = Command.noRequirements(_ -> commandRan.set(true)).named("Inner");
 
     var outerCommand =
         Command.noRequirements(
                 co -> {
                   var trigger = new Trigger(m_scheduler, triggerSignal::get);
+                  triggerRef.set(trigger);
                   trigger.onTrue(innerCommand);
                   co.park();
                 })
@@ -634,7 +638,59 @@ void triggerUnbindsWhenCommandScopeInactive() {
     m_scheduler.run();
     assertFalse(m_scheduler.isRunning(outerCommand));
 
-    // The trigger should have unbound itself during the last run() call.
+    // Trigger should still update even though command bindings were removed
+    assertTrue(triggerRef.get().getAsBoolean());
+    triggerSignal.set(false);
+    assertTrue(triggerRef.get().getAsBoolean());
+
+    m_scheduler.run();
+    assertFalse(triggerRef.get().getAsBoolean());
+  }
+
+  @Test
+  void oneLineWhileTrueBindingRetainsTrigger() {
+    var signal = new AtomicBoolean(false);
+    var command = Command.noRequirements(Coroutine::park).named("Command");
+
+    var triggerRef = new WeakReference<>(new Trigger(m_scheduler, signal::get).whileTrue(command));
+
+    assertFalse(
+        waitForCollection(triggerRef),
+        "Trigger with active command bindings should be strongly retained");
+
+    signal.set(true);
+    m_scheduler.run();
+    assertTrue(
+        m_scheduler.isRunning(command),
+        "Retained trigger should continue scheduling command bindings");
+  }
+
+  @Test
+  void unscopedTriggerCanBeGarbageCollected() {
+    // Makes the trigger scoped to an opmode
+    m_opModeId = 1;
+    m_opModeName = "opmode";
+
+    var signal = new AtomicBoolean(false);
+
+    var command = Command.noRequirements(Coroutine::park).named("Command");
+    var triggerRef = new WeakReference<>(new Trigger(m_scheduler, signal::get).onTrue(command));
+
+    m_scheduler.run();
+    assertFalse(
+        waitForCollection(triggerRef),
+        "Trigger should not be garbage collected while still in scope");
+
+    // Exit the opmode scope
+    m_opModeId = 0;
+    m_opModeName = "";
+
+    signal.set(true);
+    m_scheduler.run(); // internally removes a strong reference to the trigger
+    assertEquals(List.of(), m_events, "The trigger should not have fired");
+    assertTrue(
+        waitForCollection(triggerRef),
+        "Trigger should be garbage collected after going out of scope");
   }
 
   @Test
@@ -839,4 +895,28 @@ private BooleanSupplier flickering(AtomicBoolean signal) {
       return val;
     };
   }
+
+  @SuppressWarnings("PMD.DoNotCallGarbageCollectionExplicitly")
+  private static boolean waitForCollection(WeakReference<?> reference) {
+    for (int i = 0; i < 200; i++) {
+      if (reference.get() == null) {
+        return true;
+      }
+
+      System.gc();
+      byte[] pressure = new byte[1024 * 1024];
+      pressure[0] = 1;
+
+      try {
+        Thread.sleep(5);
+      } catch (InterruptedException ex) {
+        Thread.currentThread().interrupt();
+        break;
+      }
+
+      assertEquals(1, pressure[0]);
+    }
+
+    return reference.get() == null;
+  }
 }

styleguide/spotbugs-exclude.xml

@@ -37,6 +37,11 @@
     <Bug pattern="DM_EXIT" />
     <Class name="org.wpilib.framework.OpModeRobot" />
   </Match>
+  <Match>
+    <!-- We explicitly call System.gc() in tests to test behavior when weak references are garbage collected. -->
+    <Bug pattern="DM_GC" />
+    <Class name="~org\.wpilib\..*Test" />
+  </Match>
   <Match>
     <Bug pattern="DMI_HARDCODED_ABSOLUTE_FILENAME" />
     <Or>

wpilibj/src/main/java/org/wpilib/event/EventLoop.java

@@ -4,22 +4,27 @@
 
 package org.wpilib.event;
 
-import java.util.Collection;
 import java.util.ConcurrentModificationException;
 import java.util.LinkedHashSet;
+import org.wpilib.util.container.WeakLinkedHashSet;
 
 /**
  * A declarative way to bind a set of actions to a loop and execute them when the loop is polled.
  */
 public final class EventLoop {
-  private final Collection<Runnable> m_bindings = new LinkedHashSet<>();
+  // Keep all bindings in one insertion-ordered collection for polling.
+  private final WeakLinkedHashSet<Runnable> m_bindings = new WeakLinkedHashSet<>();
+  // Keep strong references for strongly bound actions to prevent garbage collection.
+  private final LinkedHashSet<Runnable> m_strongBindings = new LinkedHashSet<>();
   private boolean m_running;
 
   /** Default constructor. */
   public EventLoop() {}
 
   /**
-   * Bind a new action to run when the loop is polled.
+   * Bind a new action to run when the loop is polled. The event loop keeps a reference to the
+   * action, which will prevent it from being garbage collected. If memory leaks are a concern,
+   * consider {@link #bindWeak(Runnable)}.
    *
    * @param action the action to run.
    */
@@ -28,6 +33,30 @@ public void bind(Runnable action) {
       throw new ConcurrentModificationException("Cannot bind EventLoop while it is running");
     }
     m_bindings.add(action);
+    m_strongBindings.add(action);
+  }
+
+  /**
+   * Weakly binds a new action to run when the loop is polled. Unlike {@link #bind(Runnable)}, a
+   * weakly bound action will not be prevented from being garbage collected.
+   *
+   * @param action the action to run
+   */
+  public void bindWeak(Runnable action) {
+    if (m_running) {
+      throw new ConcurrentModificationException("Cannot bind EventLoop while it is running");
+    }
+    m_bindings.add(action);
+  }
+
+  /**
+   * Checks if an action is bound to the event loop.
+   *
+   * @param action the action to check
+   * @return true if the action is bound, false if not
+   */
+  public boolean isBound(Runnable action) {
+    return m_bindings.contains(action);
   }
 
   /**
@@ -41,6 +70,7 @@ public void unbind(Runnable action) {
       throw new ConcurrentModificationException("Cannot unbind EventLoop while it is running");
     }
     m_bindings.remove(action);
+    m_strongBindings.remove(action);
   }
 
   /** Poll all bindings. */
@@ -60,5 +90,6 @@ public void clear() {
       throw new ConcurrentModificationException("Cannot clear EventLoop while it is running");
     }
     m_bindings.clear();
+    m_strongBindings.clear();
   }
 }

wpilibj/src/test/java/org/wpilib/event/EventLoopTest.java

@@ -61,6 +61,21 @@ void testClear() {
     assertEquals(1, counter.get());
   }
 
+  @Test
+  void testMixedWeakAndStrongBindingsRunInInsertionOrder() {
+    var order = new StringBuilder();
+    var loop = new EventLoop();
+
+    loop.bindWeak(() -> order.append('a'));
+    loop.bind(() -> order.append('b'));
+    loop.bindWeak(() -> order.append('c'));
+    loop.bind(() -> order.append('d'));
+
+    loop.poll();
+
+    assertEquals("abcd", order.toString());
+  }
+
   @Test
   void testConcurrentModification() {
     var loop = new EventLoop();