ConfigBase with file read & write synchronization

[kiminuo]

Related to #7828

Makes sure that reading & writing of config files are mutually exclusive operations. This is a workaround for #7828 (comment).

Config classes are likely still plagued with concurrency issues. That might improve #7851 in the future (i.e. hopefully after the WW2 release).

[yahiheb]

LGTM

[soosr]

First, the MethodNameLocked naming was odd to me as I thought the lock is inside of them, but it is the other way around, they have to be inside of the Lock.

Otherwise, LGTM.

[stale]

This has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions.

[yahiheb]

@kiminuo What's up with this one?

[kiminuo]

@kiminuo What's up with this one?

There were some changes that make it non-trivial to fix the conflict. But the idea is still imho valid so I'll get to it. Hopefully soon.

[stale]

This has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions.

[molnard]

Converted to draft until this is not a priority.

[stale]

This has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions.

[molnard]

LGTM. Can you check if TryEnsureBackwardsCompatibility is still required?

[kiminuo]

LGTM. Can you check if TryEnsureBackwardsCompatibility is still required?

The code was added in #1945 and it was 3 years ago. And as far as I understand that mechanism, it looks to me that to apply the BC changes, you just run WW and it immediately saves correct key-value pairs to your .json file.

So a person would have to not use her wallet for 3 years to observe an issue. The odds are very low for that to happen. Not sure if that old WW version still works at all.

I created #9958 for that.

[molnard]

LGTM

[molnard]

BTW What was the original issue and how is this solving it? How was there a threading issue exactly?

[kiminuo]

BTW What was the original issue

The original issue is #7828 and it simply shows that we can concurrently read & write config file which leads to exceptions.

and how is this solving it?

This PR adds locking around file reading and file writing operations.

How was there a threading issue exactly?

You can be in the process of writing the config file and then somebody attempts to read the config file (CheckFileChange).

Anyway, the sentence - Config classes are likely still plagued with concurrency issues. - from the OP still holds. This PR makes the situation better, not completely fixed though.

[molnard]

You can be in the process of writing the config file and then somebody attempts to read the config file (CheckFileChange).

Can you elaborate a bit more? I don't understand how - it is surrounded by a lock:
https://github.com/molnard/WalletWasabi/blob/master/WalletWasabi/Bases/ConfigBase.cs#L50

[kiminuo]

You can be in the process of writing the config file and then somebody attempts to read the config file (CheckFileChange).

Can you elaborate a bit more? I don't understand how - it is surrounded by a lock: https://github.com/molnard/WalletWasabi/blob/master/WalletWasabi/Bases/ConfigBase.cs#L50

I don't have it fresh in memory. You are right about the file contents, I think. It's been too long since I created the PR. But I guess I wanted to make sure that config data do not get corrupted.

One scenario that seems dangerous is when two threads can execute the following two lines:

• Thread-1: https://github.com/molnard/WalletWasabi/blob/454a677e6340618a34b3abd233459b5350da18b5/WalletWasabi/Bases/ConfigBase.cs#L122
• Thread-2: https://github.com/molnard/WalletWasabi/blob/454a677e6340618a34b3abd233459b5350da18b5/WalletWasabi/Bases/ConfigBase.cs#L94

Basically, we can populate config with data and at the same time we can try to deserialize it.

The approach of this PR is basically to be defensive and allow less parallel executions. That way you have a bigger chance that nothing gets corrupted or that your write is missed or you read older data (some previous config setting).

[molnard]

Basically, we can populate config with data and at the same time we can try to deserialize it.

And why that generates The process cannot access the file exception?

The approach of this PR is basically to be defensive and allow less parallel executions.

I understand but it would be good to know how exactly the original issue happened. Anyway if we are not able to figure it out precisely, I am OK with this PR. From my side I have no idea how that happened, the original code should not do that either.

[kiminuo]

And why that generates The process cannot access the file exception?

I think that exception message was fixed by introducing locks (a previous PR). If not, somebody will report a new issue hopefully. This PR builds on top of that to make it a little bit more better (defensive).

[molnard]

ACK