Feature Commit (or Close) PR #1889

[fronobulax]

For reference see https://github.com/kolmafia/kolmafia/pull/1889

#1889 was a solution to preference file corruption. It appeared to address the issue so it was committed. But real world experience indicated a lot of deadlocks started occurring including one that occurred fairly consistently when using the Deck of Every Card. So it was reverted but kept open. As a side effect of being open several issues with testing were discovered and resolved. #2808 was created and committed to also address preference file issues. A passing test from #1889 was used with a branch that included #2808 and failed there. It is assumed that #2808 did not fix the original problem, although it is possible that the #1889 test is not a "good" test. A test was written (and committed) that used the Deck of Every Cards. It has been merged into #1889 but it has not been observed to fail. It is not certain whether the Deck induced deadlock has been fixed or whether other changes have reduced the frequency of occurrence.

I have built and run #1889 for the past week or so and had no deadlock problems.

I have not heard a lot of complains about preference file corruption in quite some time but I also know that there are many problems that I do not hear about because they are not reported or discussed on KoLmafia.us.

Should #1889 be Closed or Committed?

The case for taking some action now is the amount it time it has been open with no resolution. The alternative to Closing or Committing would be for someone to step up and agree to resolve the Deck deadlock or study the code enough to make a reliable claim that the cause of the deadlock has been eliminated. After a year and a half it is pretty clear that I personally lack something that would make that happen because I have tried and not yet succeeded.

If it is Closed then we may have to deal with preference file corruption again in the near future assuming the test in #1889 is valid. But that does not seem to be happening enough to be a current concern.

If it is Committed then we may have to deal with deadlock if Real World usage identifies additional instances. It should be noted that KoLmafia now has some deadlock detection built in and will produce stack traces when deadlock is discovered which might allow for a fix rather than a revert in #1889 is committed.

 

[heeheehee]

I have built and run #1889 for the past week or so and had no deadlock problems.

There is some bifurcation of logic between whether you're running this in headless mode or not. Preferences.java#L183-198:

      if (SwinglessUIUtils.isSwingAvailable()) {
        try {
          SwingUtilities.invokeLater(
              () -> {
                AdventureFrame.updateFromPreferences();
                MoodManager.updateFromPreferences();
                PreferenceListenerRegistry.fireAllPreferencesChanged();
              });
        } catch (Exception e) {
          // We don't really care about these exceptions.
        }
      } else {
        AdventureFrame.updateFromPreferences();
        MoodManager.updateFromPreferences();
        PreferenceListenerRegistry.fireAllPreferencesChanged();
      }

IIRC, this represents a large component of the fix-forward attempt. I would not feel confident that the deadlock has been fixed unless we've tested extensively in both modes. (In particular, I don't think any of the relevant unit tests do anything with Swing.)

 

[fronobulax]

I never ran headless but once upon a time saw the deadlock a lot running with the GUI. Since my confidence level is related to tests passing/failing it is worth looking at the Deck test to see whether headless/Swing could be a factor. April 1 was an arbitrary deadline anyway. Thanks.

 

[fronobulax]

Well...

I decided to run the coverage reports locally since that was a quick way to find out whether the lines referenced above were executed or not. The Deck test has failed locally twice now. Go figure and stand by.