Re-fix #2449

[nullst]

Description:

The issue #2449 is still not fixed by the pull request #2450 for me since concurrent writes are not the only problem. There is another problem:

• preferences.saveToFile() sets ignoreChanges to true for 100 ms (using resetIgnore()).
• A default change listener added in newPreferences() just ignores all changes made during that time window.
• If no changes in preferences occur after the 100ms window, the ignored changes are not saved to disk since the change listener responsible for that will never be invoked.

Suggested solution implemented in this pull request:

• Add a counter numIgnoredChanges int to preferences. The access is guarded by the same .prefLock as .ignoreChanges.
• Let the default change listener count how many times it was invoked while ignoreChanges is true (guarded by p.prefLock.Lock() instead of p.prefLock.RLock() since we modify the contents).
• Change resetIgnore() to reset p.numIgnoredChanges as well, and if the number was not zero, call p.InMemoryPreferences.fireChange(). This is not possible as fireChange() is not an exported method of the internal module, so it was made public.

Fixes #2449

Checklist:

• Tests included.
• Lint and formatter run with no errors.
• Tests all pass.

[Jacalz]

Thanks for opening this PR. I just want to give a heads up that the tests seem to be failing.

[nullst]

I've fixed the formatting issue in the commit above.

Note that I have absolutely no idea what caused the test testQueueLazyInit from data/binding/queue_test.go to fail for the original pull request, and exclusively during ubuntu platform tests (not other platforms). My changes are not related to this code. Should we just ignore this as a fluke?

[Jacalz]

I don’t know. Will see what the latest rounds of tests here give us. Something seems to be wrong with your two latest commits. They give me 404 when trying to look at them.

EDIT: All of them do now. Is GitHub down?

[nullst]

That's weird, the links from this pull request give 404 for me as well. Not sure what happened and how to fix it. The links in https://github.com/nullst/fyne/commits/nullst-patch-1 work fine. Part of that is due to a mismatch between e-mail address I used for github and e-mail address configured with git locally, but it should be fixed now, sorry. If nothing else works, maybe I should just make a new pull request with the same content?

[andydotxyz]

Thanks for working on this, I agree there is still a problem.
However I'm not sure that this is the right fix. The ignoreChange is meant to avoid a changedfile->load->fire->write loop, so the additional work here to un-ignore internal changes should not be needed.
I think the root of the problem is the following code, around line 115 of app/preferences.go

		p.prefLock.RLock()
		shouldIgnoreChange := p.ignoreChange
		p.prefLock.RUnlock()
		if shouldIgnoreChange { // callback after loading, no need to save
			return
		}

Here you see that we have a problem on load as well - we read the file, it sees the change and can then force a write, we don't want this.
I wondered if a more correct change might be to stop the change from triggering a save during load, then this code would not have an impact in the rest of the app, which I think would resolve the issue.

What do you think @nullst?

[andydotxyz]

As described on the conversation, I think that this is not the simplest fix - it works around a mistaken use of the ignoreChange instead of resolving the root problem.

[nullst]

I have implemented the strategy suggested by @andydotxyz above: since loading the preferences file is conceptually a different operation than just an application updating multiple preferences very quickly, I added a new flag, "loadingInProgress" to the preferences structure. The existing .ignoreChanges flag was renamed to '.suspendChanges' and repurposed to avoid rewriting the preferences file too fast.

[andydotxyz]

Iim not sure about numSuspendedChanges... This counts how many times we write the file in quick succession.
But after writing the file we should never have to re-read it, that is what ignoreChanges was about in the first place.
Is there some reason you think we need to read the file shortly after we have finished writing it?

[nullst]

Your comment helped me understand the mechanism better, thank you. Mostly, my original changes were not relevant to loading the preferences file at all, I'm concerned with the situation where the application just updates many different preferences in a row. This is kind of similar to what happens during load(), but it should still result in a file save after the last change, and this wasn't working properly due to the measures implemented to avoid triggering save() after load().

In general, as I see it now, there are three situations we want to avoid:

1. "Load causes save": loading preferences from file changes in-memory preferences, and normally those changes should be immediately written back to disk, but in this case this behavior is undesirable.
2. "Save causes load": when an application updates preferences, the preferences file will be modified on disk. But we are also watching the file for changes (in app/preferences_other.go), to reload preferences when modified. The "file changed" callback is invoked at some unknown time slightly after saving is finished. So we want to neuter the callback for some time after saving. (And if a file is modified by an external program during that small time period, too bad for the external program -- I don't think there are easy workarounds.)
3. "Save during saving": if another save is triggered (by the application updating preferences again) during the time period where the "file changed" callback is disabled because of a previous change, as implemented currently, the timer responsible for flipping ignoreChanges flag that started for the first save could disable the flag very shortly after the second save since they do not coordinate, causing situation (2). To be honest, this is not a big deal: at worst, one spurious full reload of the preferences file will happen if you are unlucky. But we may still want to prevent this scenario.

Based on this description, I've renamed the "ignoreChanges" variable to "savedRecently" since it better reflects the role. I've also removed the comments, as you suggested. Originally the same variable was used to protect against both (1) and (2), and I think it does not lead to the correct logic.

[andydotxyz]

Thanks, that is a great summary @nullst.
I agree with your points, but I'm still not sure that the counter is needed. It is read only here:

		if changes > 0 {
			p.InMemoryPreferences.FireChange()
		}

inside the callback that resets the ignore status, which is called after we save. So my question is - why do we need to ensure that we re-load the file after we have written it out a number of times?

[nullst]

Sorry, I'm not sure I understand the question. FireChange() does not reload the file, it saves it to disk. This part is mostly about point (3) of the summary above: suppose an application calls Preferences().SetString() two times in a row, let's say with 50ms passed between them:

1. The first time, savedRecently (previously called ignoreChanges) flag is set, the preferences file is updated on disk, and we run a timer to disable savedRecently after 100ms.
2. The second time an update happens while savedRecently flag is still on. There are now two approaches. Approach A (not implemented): we immediately rewrite the file on disk again and then we want to keep savedRecently flag on for another 100ms. To do this correctly, we would need to communicate with the timer started during the first save to extend the time-out. Approach B (implemented here): we delay the re-writing of the file on disk until the flag savedRecently is flipped by the timer started during the first save, and we remember to call .save() after that. This is what numSuspendedChanges is for: it is nonzero if we registered that save() should be called when the timer expires. It does not have to be a counter, it can just be bool if you prefer.

For approach A a more complicated synchronization pattern would be required than just firing up a goroutine with a timer in ignoreChanges. Approach B seems simpler and more robust. Moreover, for free we get an IO-limiting mechanism: even if the application updates 5000 preferences per second, we only overwrite the file 10 times per second, each time storing the current state of the preferences. This limiting mechanism is not particularly valuable, but getting it naturally is probably a nice bonus.

UPD: I should perhaps also note that numSuspendedChanges is not modified during the loading progress, it is only used when savedRecently is true, not when loadingInProgress is true.

[andydotxyz]

Ah, great explanation thanks. Should the callback perhaps call save instead of feeding it back through the FireChanges loop again (I think this is what confused me).
Perhaps a bool may make it a little cleaner too, as we don’t actually care what number it is after 1

[nullst]

This makes sense, it's actually easier. Now resetSavedRecently calls .save() directly and bool is used instead of a counter. I also made FireChange() an internal function again (i.e., renamed to fireChange()).

[andydotxyz]

Thanks, much clearer now I think - just a small casing issue in the doc

[andydotxyz]

I think we need @Jacalz to OK the changes too