Protect AppSettings saving using global mutex

[mstv]

Fixes #10016

Proposed changes

• Protect AppSettings.SaveSettings using a global mutex common to all GE instances

Screenshots

N/A

Test methodology

• manual

Test environment(s)

• Git Extensions 33.33.33
• Build 403856d
• Git 2.35.2.windows.1 (recommended: 2.37.1 or later)
• Microsoft Windows NT 10.0.19044.0
• .NET 6.0.9
• DPI 96dpi (no scaling)

Merge strategy

I agree that the maintainer squash merge this PR (if the commit message is clear).

---

✒️ I contribute this code under The Developer Certificate of Origin.

[gerhardol]

should this be shared?
https://docs.microsoft.com/en-us/dotnet/api/system.threading.mutex.openexisting?view=net-6.0#system-threading-mutex-openexisting(system-string)

[mstv]

Yes, that's the trick. GE instances must write the settings one after the other. (The last one wins. For this I leave #8006 open.)

[gerhardol]

Why not use OpenExisting in this PR?
(This PR is probably an improvement anyway.)

[mstv]

The use of OpenExisting needs a lot of additional code for little benefit (protection by access rights).
From https://docs.microsoft.com/en-us/dotnet/api/system.threading.mutex.-ctor?view=net-6.0#system-threading-mutex-ctor(system-boolean-system-string):

If a name is provided and a synchronization object of the requested type already exists in the namespace, the existing synchronization object is used.

[mstv]

A named mutex is always shared between processes - at least in the same session.

If a synchronization object of a different type already exists in the namespace, a WaitHandleCannotBeOpenedException is thrown.

[gerhardol]

https://docs.microsoft.com/en-us/dotnet/api/system.threading.mutex.-ctor?view=net-6.0#system-threading-mutex-ctor(system-boolean-system-string)

The name may be prefixed with Global\ or Local\ to specify a namespace. When the Global namespace is specified, the synchronization object may be shared with any processes on the system. When the Local namespace is specified, which is also the default when no namespace is specified, the synchronization object may be shared with processes in the same session. On Windows, a session is a login session, and services typically run in a different non-interactive session. On Unix-like operating systems, each shell has its own session. 

So it seems to be working in Windows, append Global to prepare for other platforms.

Have you tested multiple closing (by adding a delay or so).

[mstv]

OK, I admit I stopped reading after "On Windows, a session is a login session".
Adding "Global\".

Have you tested multiple closing (by adding a delay or so).

Yes, it takes 30s to close three instances at the same each waiting 10s - both with and without "Global\"

[RussKie]

Reading the docs, this looks sensible 👍

[gerhardol]

I would like a confirmation it actually is tested though...

[pmiossec]

Fixes #10016

From what I see of the code, this PR is more a fix for #8006 (error messages because multiple processes fails to acces the same file) than #10016 (the last process which write the settings overwrite some changes made in the settings in a process that exited earlier).

And for #10016, IMHO, there is no perfect (or easy) solution. Maybe preventing the instances that has no setting change to write the settings when exiting...

[mstv]

Fixes #10016

From what I see of the code, this PR is more a fix for #8006 (error messages because multiple processes fails to acces the same file) than #10016 (the last process which write the settings overwrite some changes made in the settings in a process that exited earlier).

And for #10016, IMHO, there is no perfect (or easy) solution. Maybe preventing the instances that has no setting change to write the settings when exiting...

Igor proposed a solution in #8006 (comment).

I think #10016 has the same root cause: multiple instances are trying to write the settings file.

As the user stated "Then when I opened GitExtensions, at least one setting (probably Show artificial commits or not) was reset to default.", the setting were broken - not just the last GE instance won. So the compiled-in defaults were used on next start.

[RussKie]

The settings is a pandora box, isn't it? :)

When I think about this area, I have the following problems in mind:

1. Whenever the Settings dialog is opened it should read the latest settings available, so that when the dialog is closed with [OK] button the settings are up-to-date.
   This operation is the "best we can do", because if there is another instance of the app modifies the settings whilst the Settings dialog is opened in the current instance, the changes from another instance will be overwritten. This is a pure user-error scenario, and I don't believe we have to try to address it.
2. Changes in the Settings dialog must be cancellable, i.e., if the dialog is dismissed (i.e., neither [OK] or [Apply] buttons clicked) then no changes should be written to the file.
   I believe this is the case now for all pages except for ChecklistSettingsPage:
   

   gitextensions/GitUI/CommandsDialogs/SettingsDialog/Pages/ChecklistSettingsPage.cs

   Line 153 in ad6671d

   public override bool IsInstantSavePage => true;

3. Any other settings configurable outside of the Settings dialog (e.g., Show Current Branch) must be persisted immediately.
   I believe this is how the app is working now (though I haven't formally verified it).
   

   gitextensions/GitCommands/Settings/SettingsCache.cs

   Lines 119 to 133 in ad6671d

   	public void SetValue(string name, string? value)
   	{
   	LockedAction(() =>
   	{
   	// will refresh EncodedNameMap if needed
   	string? inMemValue = GetValue(name);
   	if (string.Equals(inMemValue, value))
   	{
   	return;
   	}
   	SetValueImpl(name, value);
   	SettingsChanged();

4. The config needs to be reloaded whenever the settings changed (as follows from #3).
   I believe this is how the app is working now too, as least for some settings - if I apply "Show Current Branch" filter in one instance, and refresh another instance the filter is applied there too.

I don't think we must persist settings on the app exit, and this IMO is the ultimate fix for #10016 and other related issues.

gitextensions/GitUI/CommandsDialogs/FormBrowse.cs

Lines 1435 to 1438 in ad6671d

	private static void SaveApplicationSettings()
	{
	AppSettings.SaveSettings();
	}

[mstv]

Writing the settings must be an atomic operation. That's why this PR is necessary and should be taken as is.
Since this PR does not suffice to avoid losing settings, I have proposed to continue the discussion in #8006.

[gerhardol]

This is not all settings, see FormSettings Save(), but what may be changed outside FormSettings it seems.
So open Settings could still try modify the settings.

This is an improvement and should be merged and included in 4.0

[mstv]

This is not all settings, see FormSettings Save(), but what may be changed outside FormSettings it seems.

Yes, there are other settings. Though they are neither written on SessionEnding nor on Exit. So they fall into point 1. of Igor's #10193 (comment).
Yes, readers must be locked out while one instance is updating the file.

So open Settings could still try modify the settings.

This should be idempotent, i.e. not change on reopen FormSettings right after it was closed.

[mstv]

Any concerns yet merging this?

[gerhardol]

Any concerns yet merging this?

No, it should be limiting some issues.