More synchronization needed to avoid rare race conditions

[jan-niestadt]

There's several places in the code where it is assumed that threads will see each other's mutations directly and in the order they were made. However, without synchronization, this is apparently not the case.

To clarify, if thread A does this:

dogName = "Fido";
isDogNameSet = true;

Then we have no guarantee when thread B will see these changes.

Worse, thread B may see the change to isDogNameSet but not the change to dogName! This is because of out-of-order processing on modern CPUs. Without synchronization, Java will generate code that for thread A will behave as if everything is being done in-order, but actually things may be done out of order without thread A being able to detect it. But the compiler obviously cannot take thread B into account, so it sees things in the order they actually happen. This is why we need synchronization, even though at first it seems unnecessary.

The most important instance of this is probably the Hits classes, which are designed to be "eventually immutable" - that is, the view from outside the class should be immutable, but internally, not all hits will have been read yet. We probably need more synchronization in those classes.

Specifically, while debugging, I have seen HitsFromQueryParallel.allSourceSpansFullyRead have the value false after a call to size(), which should not be possible (as size() has to fetch all hits before it can report how many there are).

Apparently this rarely results in issues in practice, but we should clearly rethink our approach. Apart from synchronized, maybe volatile could help in certain cases as well, as it guarantees other threads will get to see the new value right away.

References:

• https://www.logicbig.com/tutorials/core-java-tutorial/java-multi-threading/thread-reordering.html
• https://stackoverflow.com/a/2441402
• https://docs.oracle.com/javase/specs/jls/se7/html/jls-17.html#jls-17.4.4 (17.4.4 and 17.4.5 seem relevant)