Data race when saving and loading a FileConfig at the same time?

[TheElectronWill]

See neoforged/NeoForge#32 and MinecraftForge/MinecraftForge#9122.

TODO:

• [x] fix WriteSyncFileConfig, the easiest way would be to synchronize the whole body of load() (and close() too btw)
• [x] double-check WriteAsyncFileConfig
• [x] write a test that attempts to reproduce the issue in a multithreaded environment (hard)
• [x] debounce FileWatcher's events
• [x] when autoreloading a configuration, ensure that the config's content is replaced atomically, otherwise the config can be in an inconsistent aka "corrupted" state for a short period of time
• [x] make it possible to only accept the config's reloading result if it passes ConfigSpec's checking

[TheElectronWill]

wip :)

[TheElectronWill]

All FileConfigs are now thread-safe, and provide methods to get a consistent view of their content, preventing any interference from other concurrent operations. If you need such consistency, use the bulkRead, bulkCommentedRead, bulkUpdate and bulkCommentedUpdate methods.

Example

Suppose that we have a FileConfig fileConfig with initial content {a = "old A", b = "old B"}. We would like to change the configuration to {a = "new A", b = "new B"}, in a multi-threaded environment with two threads : thread 1 and thread 2.

A naive (bad!) way of doing this is to use get and set like this:

// thread 1
Object a = fileConfig.get("a");
Object b = fileConfig.get("b");

// thread 2
fileConfig.set("a", "new A");
fileConfig.set("b", "new B");

:warning: Problem: even if the config is backed by a ConcurrentHashMap, consistency is not guaranteed, that is, the operations could be performed in this order:

fileConfig.get("a"); // "old A"
fileConfig.set("a", "new A");
fileConfig.set("b", "new B");
fileConfig.get("b"); // "new B" - inconsistent with a!

In that case, you would see {a = "old A", b = "new B"}, which is NOT a valid state of your config! Worse: if your FileConfig is autosaved, this invalid state may be written to a file, and processed by other programs. Even worse: the same problem occurs when load()-ing a FileConfig, which means that you can observe incomplete loads.

By rewriting the implementation of FileConfig to use actual thread-safe configurations (#152), I've provided a solution to this problem.

:arrow_right: You can now write (good):

// thread A
fileConfig.bulkRead(view -> {
  Object a = view.get("a");
  Object b = view.get("b");
  // do something with a and b
});

// thread B
fileConfig.bulkUpdate(view -> {
  view.set("a", "new A");
  view.set("b", "new B");
});

Here, bulkRead ensures that the code using the view sees a consistent version of the config. The view cannot be modified by someone else during the read. Combined with bulkUpdate, this allows to perform consistent read and write to the configuration. The configuration can never be observed in an invalid state :slightly_smiling_face: Furthermore save() and load() use these new methods (and more shiny stuff) in order to ensure that the configuration stays consistent during all saving and loading operations.

:information_source: It is possible to return a result from the bulk methods:

String myCoherentString = fileConfig.bulkRead(view -> {
    String a = view.get("a");
    String b = view.get("b");
    return a + "," + b;
});
assertTrue(myCoherentString.equals("old A,old B") || myCoherentString.equals("new A,new B"));