[java] Support disabling Preferences support for legacy dashboards

To support legacy dashboards, the Preferences code installed a listener that
marked all new topics as persisted. Unfortunately, having a listener that
updates new topics in the background can result in a closed
NetworkTablesInstance being accessed (resulting in a SIGSEGV) when
Preferences.setNetworkTableInstance() is called soon after values have been
published for new topics (see #8215).

It's possible this race condition is why PreferencesTest.removeAllTest() was
flaky.

The code to set new topics discovered under /Preferences as persistent was
added back in 2015 in 87fc49c. Dashboards that want to want data for new keys
to be saved should be explicitly marking the topics as persistant.

Author: kcooney

wpilibj/src/main/java/edu/wpi/first/wpilibj/Preferences.java

@@ -45,6 +45,7 @@ public final class Preferences {
   private static StringPublisher m_typePublisher;
   private static MultiSubscriber m_tableSubscriber;
   private static NetworkTableListener m_listener;
+  private static boolean m_supportLegacyDashboards = true;
 
   /** Creates a preference class. */
   private Preferences() {}
@@ -60,7 +61,12 @@ private Preferences() {}
    * @param inst NetworkTable instance
    */
   public static synchronized void setNetworkTableInstance(NetworkTableInstance inst) {
-    m_table = inst.getTable(kTableName);
+    NetworkTable table = inst.getTable(kTableName);
+    if (table.equals(m_table)) {
+      return;
+    }
+    m_table = table;
+
     if (m_typePublisher != null) {
       m_typePublisher.close();
     }
@@ -78,23 +84,57 @@ public static synchronized void setNetworkTableInstance(NetworkTableInstance ins
     }
     m_tableSubscriber = new MultiSubscriber(inst, new String[] {m_table.getPath() + "/"});
 
-    // Listener to set all Preferences values to persistent
-    // (for backwards compatibility with old dashboards).
     if (m_listener != null) {
       m_listener.close();
     }
-    m_listener =
-        NetworkTableListener.createListener(
-            m_tableSubscriber,
-            EnumSet.of(NetworkTableEvent.Kind.kImmediate, NetworkTableEvent.Kind.kPublish),
-            event -> {
-              if (event.topicInfo != null) {
-                Topic topic = event.topicInfo.getTopic();
-                if (!topic.equals(m_typePublisher.getTopic())) {
-                  event.topicInfo.getTopic().setPersistent(true);
-                }
-              }
-            });
+    if (m_supportLegacyDashboards) {
+      m_listener = createTopicPersistingListener();
+    } else {
+      m_listener = null;
+    }
+  }
+
+  /**
+   * Enables or disables support for dashboards that create Preferences topics without marking them
+   * as persistent.
+   *
+   * <p>Legacy dashboard support is enabled by default. In future releases of WPILIb, it might
+   * change to be disabled by default.
+   *
+   * @param enable Whether legacy dashboard support should be enabled
+   * @return Whether legacy dashboard support was enabled when this method was called
+   */
+  public static synchronized boolean enableLegacyDashboardSupport(boolean enable) {
+    boolean wasEnabled = m_supportLegacyDashboards;
+    if (wasEnabled != enable) {
+      if (enable) {
+        m_listener = createTopicPersistingListener();
+      } else if (m_listener != null) {
+        m_listener.close();
+        m_listener = null;
+      }
+      m_supportLegacyDashboards = enable;
+    }
+    return wasEnabled;
+  }
+
+  /**
+   * Creates a listener that update all Preference topics to be persistent.
+   *
+   * <p>This is done for backwards compatibility with old dashboards.
+   */
+  private static NetworkTableListener createTopicPersistingListener() {
+    return NetworkTableListener.createListener(
+        m_tableSubscriber,
+        EnumSet.of(NetworkTableEvent.Kind.kImmediate, NetworkTableEvent.Kind.kPublish),
+        event -> {
+          if (event.topicInfo != null) {
+            Topic topic = event.topicInfo.getTopic();
+            if (!topic.equals(m_typePublisher.getTopic())) {
+              event.topicInfo.getTopic().setPersistent(true);
+            }
+          }
+        });
   }
 
   /**

wpilibj/src/test/java/edu/wpi/first/wpilibj/PreferencesTest.java

@@ -12,15 +12,21 @@
 import static org.junit.jupiter.api.parallel.ExecutionMode.SAME_THREAD;
 
 import edu.wpi.first.networktables.NetworkTable;
+import edu.wpi.first.networktables.NetworkTableEntry;
+import edu.wpi.first.networktables.NetworkTableEvent;
 import edu.wpi.first.networktables.NetworkTableInstance;
+import edu.wpi.first.networktables.NetworkTableListener;
 import edu.wpi.first.networktables.Topic;
 import java.io.IOException;
 import java.io.InputStream;
 import java.nio.file.Files;
 import java.nio.file.Path;
 import java.util.ArrayList;
 import java.util.Arrays;
+import java.util.EnumSet;
 import java.util.List;
+import java.util.concurrent.Semaphore;
+import java.util.concurrent.TimeUnit;
 import java.util.stream.Stream;
 import org.junit.jupiter.api.AfterEach;
 import org.junit.jupiter.api.BeforeEach;
@@ -31,6 +37,7 @@
 import org.junit.jupiter.api.parallel.Execution;
 import org.junit.jupiter.params.ParameterizedTest;
 import org.junit.jupiter.params.provider.MethodSource;
+import org.junit.jupiter.params.provider.ValueSource;
 
 @Execution(SAME_THREAD)
 class PreferencesTest {
@@ -69,6 +76,7 @@ void setup(@TempDir Path tempDir) {
 
   @AfterEach
   void cleanup() {
+    m_inst.waitForListenerQueue(0.1);
     m_inst.close();
   }
 
@@ -121,6 +129,40 @@ void defaultValueTest() {
         () -> assertFalse(Preferences.getBoolean("checkedValueBoolean", true)));
   }
 
+  @ParameterizedTest
+  @ValueSource(booleans = {true, false})
+  void enableLegacyDashboardSupportTest(boolean enable) throws InterruptedException {
+    // Publish a value, wait until we are sure the listener would have fired, and verify that the
+    // topic is persistant only if legacy dashboard support is enabled.
+    boolean wasEnabled = Preferences.enableLegacyDashboardSupport(enable);
+    Semaphore semaphore = new Semaphore(0);
+    NetworkTableEntry entry = m_table.getEntry("legacyDashboardValueLong");
+    try (NetworkTableListener listener =
+        NetworkTableListener.createListener(
+            entry,
+            EnumSet.of(NetworkTableEvent.Kind.kImmediate, NetworkTableEvent.Kind.kValueAll),
+            event -> {
+              if (event.valueData != null) {
+                semaphore.release();
+              }
+            })) {
+      // Publish a value, wait for listeners to fire, and do that again. This ensures any listener
+      // installed by Preferences would have been called at least once.
+      for (int value = 1; value < 3; value++) {
+        entry.setInteger(value);
+        m_inst.waitForListenerQueue(0.5);
+        if (!semaphore.tryAcquire(100, TimeUnit.MILLISECONDS)) {
+          fail("timed out waiting for event listener");
+        }
+      }
+      assertEquals(enable, entry.isPersistent());
+    } finally {
+      if (wasEnabled != enable) {
+        Preferences.enableLegacyDashboardSupport(wasEnabled);
+      }
+    }
+  }
+
   @Nested
   class PutGetTests {
     @Test