[wpilib] Support disabling Preferences support for legacy dashboards

[kcooney]

To support legacy dashboards, the Java 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 persistent.

Note that the listener was only installed in Java, so the original work-
around only worked if the custom dashboard and/or the robot was written in
Java. Custom dashboard used by teams might break today if teams switch from
Java to C++ or Python for their robot code.

[calcmogul]

Which dashboards does "legacy dashboard" refer to? Can the dashboards be fixed directly if they're still being maintained and that's where the root cause is?

[kcooney]

Which dashboards does "legacy dashboard" refer to?

@calcmogul I'm not sure. The original commit adding this listener (87fc49c) did not provide those details.

Can the dashboards be fixed directly if they're still being maintained and that's where the root cause is?

The root cause of the SIGSEGV is a race condition in the WPILib C++ code that would be challenging to fix.

Dashboards that write to NetworkTables using keys under /Preferences directly (vs via the Preferences) can ensure that data is persistent by updating the topic to be persistent. If there are some dashboards that do not do this teams can use the enableLegacyDashboardSupport() call to ensure the data is persisted.

It's hard to say what dashboards do not do this, as the workaround added ten years ago would hide those bugs. The only way to find them is to turn off the behavior by default at some point.

Note the SIGSEGV would not happen in a competition because the default NetworkTableInstance cannot be closed.

[ThadHouse]

If we're going to do something like this, we'd probably prefer it to be opt out, rather then opt in. If you opt out before switching the instance, that should work. Otherwise, this risks breaking dashboards.

[kcooney]

If we're going to do something like this, we'd probably prefer it to be opt out, rather then opt in.

@ThadHouse thanks for the feedback.

I considered making it opt-out (by adding a boolean parameter to enableLegacyDashboardSupport()) but the code to do that would be slightly more complex, and I wanted to wait for feedback from the maintainers.

I didn't consider making it opt-out for the default instance but opt-in otherwise. My initial thinking on that idea is that it would be confusing to users. IMHO the behavior of a NetworkTableInstance shouldn't depend on how it was created.

Let's see what the WPILib maintainers say about opt-in vs opt-out.

If it is opt-out initially, I would personally prefer that it be opt-in for the following year. The APIs to fix the dashboards have existed for a long time, and the SIGSEGV caused a lot of failed GitHub workflows for our team.

[ThadHouse]

Having it be opt in would be much more confusing for users, and would be a breaking change. A majority of users are not changing the instance.

Ideally we'd prefer to fix the actual issue, but pending that having an opt out for users that due run into issues would be enough in our eyes.

[kcooney]

Having it be opt in would be much more confusing for users, and would be a breaking change. A majority of users are not changing the instance.

Thanks for the feedback, but I'm not sure if the WPILib team will accept something like this at all, so I won't be making changes on this branch until I get feedback from a maintainer.

Ideally we'd prefer to fix the actual issue

If you have ideas on how that might be done, I suggest commenting on the bug.

I would personally prefer we fix the underlying issue and have a path forward to eventually removing this work-around.

[calcmogul]

Thanks for the feedback, but I'm not sure if the WPILib team will accept something like this at all, so I won't be making changes on this branch until I get feedback from a maintainer.

@ThadHouse is a WPILib maintainer.

[kcooney]

Thanks for the feedback, but I'm not sure if the WPILib team will accept something like this at all, so I won't be making changes on this branch until I get feedback from a maintainer.

@ThadHouse is a WPILib maintainer.

Ah! Wasn't aware of that. GitHub shows the roles of people making comments on issues, but apparently not in PRs. @ThadHouse sorry for the misunderstanding.

I can make changes sometime next week (tracking down and working around this issue in our code ate up a lot time, and I need to focus on other things)

@ThadHouse would the WPILib team accept something like this with the listener being enabled by default?

[sciencewhiz]

Which dashboards does "legacy dashboard" refer to? Can the dashboards be fixed directly if they're still being maintained and that's where the root cause is?

In 2015, I think the legacy dashboards would have been SmartDashboard, LabVIEW dashboard and SFX (which was dropped long ago). But I think that's less important since any dashboards made after that point could also be depending on that behavior. It does appear that SmartDashboard was updated to set persistent on new keys created in the dashboard. It looks like while Shuffleboard, Elastic, and FRC Web Components have preferences widgets, none of them appear to let users add keys, so wouldn't be affected. However, there are a number of team maintained dashboards that this may break, and this would be a silent breakage, which is bad.

One thought would be that it it was opt-out, the listener could also be modified to output a warning if a new key is not set persistent and the listener has to do it, which is a way to warn people about a future change.

[kcooney]

One thought would be that it it was opt-out, the listener could also be modified to output a warning if a new key is not set persistent and the listener has to do it, which is a way to warn people about a future change.

@sciencewhiz I considered that, but since the listeners are triggered from a different thread, it is in theory possible that it would see topics created by the Preferences code after they were created but before they were marked as persistent.

[kcooney]

@ThadHouse updated the support for legacy dashboards in this PR to be opt-out. PTAL.