Skip to content

Commit fb6fc0a

Browse files
committed
[java] Make Preferences support for legacy dashboards opt-in
This removes one of the triggers of #8215 (but not the underlying cause). 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. 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 new topics are added. It's possible this race condition is why PreferencesTest.removeAllTest() was flaky. Changes: - Add enableLegacyDashboardSupport() - Only create a listener in setNetworkTableInstance() if enableLegacyDashboardSupport() was called
1 parent 08f1148 commit fb6fc0a

File tree

2 files changed

+79
-2
lines changed

2 files changed

+79
-2
lines changed

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

Lines changed: 32 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -45,6 +45,7 @@ public final class Preferences {
4545
private static StringPublisher m_typePublisher;
4646
private static MultiSubscriber m_tableSubscriber;
4747
private static NetworkTableListener m_listener;
48+
private static boolean m_supportLegacyDashboards;
4849

4950
/** Creates a preference class. */
5051
private Preferences() {}
@@ -78,11 +79,40 @@ public static synchronized void setNetworkTableInstance(NetworkTableInstance ins
7879
}
7980
m_tableSubscriber = new MultiSubscriber(inst, new String[] {m_table.getPath() + "/"});
8081

81-
// Listener to set all Preferences values to persistent
82-
// (for backwards compatibility with old dashboards).
8382
if (m_listener != null) {
8483
m_listener.close();
84+
m_listener = null;
8585
}
86+
if (m_supportLegacyDashboards) {
87+
createTopicPersistingListener();
88+
}
89+
}
90+
91+
/**
92+
* Enables support for dashboards that create Preferences topics without marking them as
93+
* persistent.
94+
*/
95+
public static synchronized void enableLegacyDashboardSupport() {
96+
if (!m_supportLegacyDashboards) {
97+
m_supportLegacyDashboards = true;
98+
createTopicPersistingListener();
99+
}
100+
}
101+
102+
static synchronized void disableLegacyDashboardSupport() {
103+
if (m_supportLegacyDashboards) {
104+
m_supportLegacyDashboards = false;
105+
m_listener.close();
106+
m_listener = null;
107+
}
108+
}
109+
110+
/**
111+
* Creates a listener that update all Preference topics to be persistent.
112+
*
113+
* <p>This is done for backwards compatibility with old dashboards.
114+
*/
115+
private static synchronized void createTopicPersistingListener() {
86116
m_listener =
87117
NetworkTableListener.createListener(
88118
m_tableSubscriber,

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

Lines changed: 47 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -12,15 +12,21 @@
1212
import static org.junit.jupiter.api.parallel.ExecutionMode.SAME_THREAD;
1313

1414
import edu.wpi.first.networktables.NetworkTable;
15+
import edu.wpi.first.networktables.NetworkTableEntry;
16+
import edu.wpi.first.networktables.NetworkTableEvent;
1517
import edu.wpi.first.networktables.NetworkTableInstance;
18+
import edu.wpi.first.networktables.NetworkTableListener;
1619
import edu.wpi.first.networktables.Topic;
1720
import java.io.IOException;
1821
import java.io.InputStream;
1922
import java.nio.file.Files;
2023
import java.nio.file.Path;
2124
import java.util.ArrayList;
2225
import java.util.Arrays;
26+
import java.util.EnumSet;
2327
import java.util.List;
28+
import java.util.concurrent.Semaphore;
29+
import java.util.concurrent.TimeUnit;
2430
import java.util.stream.Stream;
2531
import org.junit.jupiter.api.AfterEach;
2632
import org.junit.jupiter.api.BeforeEach;
@@ -69,6 +75,8 @@ void setup(@TempDir Path tempDir) {
6975

7076
@AfterEach
7177
void cleanup() {
78+
Preferences.disableLegacyDashboardSupport();
79+
m_inst.waitForListenerQueue(0.1);
7280
m_inst.close();
7381
}
7482

@@ -121,6 +129,45 @@ void defaultValueTest() {
121129
() -> assertFalse(Preferences.getBoolean("checkedValueBoolean", true)));
122130
}
123131

132+
@Test
133+
void enableLegacyDashboardSupportTest() throws InterruptedException {
134+
// Publish a value, wait until we are sure the listener would have fired, and verify that the
135+
// topic is not marked as persistant.
136+
Semaphore semaphore = new Semaphore(0);
137+
NetworkTableEntry entry = m_table.getEntry("legacyDashboardValueLong");
138+
try (NetworkTableListener listener =
139+
NetworkTableListener.createListener(
140+
entry,
141+
EnumSet.of(NetworkTableEvent.Kind.kImmediate, NetworkTableEvent.Kind.kValueAll),
142+
event -> {
143+
if (event.valueData != null) {
144+
semaphore.release();
145+
}
146+
})) {
147+
// Set a value, wait for listeners to fire, and do that again. This ensures any listener for
148+
// thus entry would have been called at least once.
149+
for (int value = 1; value < 3; value++) {
150+
entry.setInteger(value);
151+
if (!semaphore.tryAcquire(100, TimeUnit.MILLISECONDS)) {
152+
fail("timed out waiting for event listener");
153+
}
154+
}
155+
156+
assertFalse(entry.isPersistent());
157+
158+
// When legacy dashboard support is enabled, the topic should be marked as persistant.
159+
Preferences.enableLegacyDashboardSupport();
160+
for (int value = 3; value < 5; value++) {
161+
entry.setInteger(value);
162+
if (!semaphore.tryAcquire(100, TimeUnit.MILLISECONDS)) {
163+
fail("timed out waiting for event listener");
164+
}
165+
}
166+
167+
assertTrue(entry.isPersistent());
168+
}
169+
}
170+
124171
@Nested
125172
class PutGetTests {
126173
@Test

0 commit comments

Comments
 (0)