Possible races

[sunmisc]

What is it?

• Codebase improvement (dev facing)

Description of the changes in your PR

Let's start in order:
First, if you write under a lock, it doesn't mean that reading a non-thread-safe collection is thread-safe, you break HB's transitivity:
There are two ways out in this situation:

• COW imposes additional costs on writing (copies), but at the same time reading from COW is thread-safe and without blocking
• Synchronize non-thread-safe list (which is better in this case)

But, there are still problems, the fact is that the class is non-thread-safe by design:

• You provide a list to the user, which means you provide a non-thread-safe iterator - the solution is to copy the list and give the user a snapshot

P.S eeven though the race may be out of scope for your implementation, if you are already making the methods thread-safe, you need to make those method binders thread-safe

Unfortunately, this is the first class in which I encountered a race, such a problem may be in other classes

Fixes the following issue(s)

Possible races in perspective

[Stypox]

I don't know much about multithreading unfortunately. And sorry for reviewing so late.

• What does "HB's transitivity" mean? At least I could find out that CoW means "copy-on-write".
• I also don't understand what volatile does exactly: I have read this article but I don't understand how it applies here and why you need it. Do you use it to make sure all class instance fields are up-to-date whenever you read any volatile field from them?
• What is the difference between having synchronized in a method signature vs synchronized(this) just inside?
• Is the queue actually ever accessed from multiple threads? The player service and the UI both run on the main thread afaik. If I am wrong on this then, well, be sure to tell me ;-)
• Did you test these changes thoroughly?

[sunmisc]

no, unfortunately, I did not test what I wrote, but I can say one thing for sure:
If we have thread competition, this can happen if we add elements to the collection from some other thread (in asynchronous loading for example)
We must also read under a lock, because we can get an intermediate result (not atomic), which can lead to unpredictable results, even if we read in one thread, if there is a writer from another, we must coordinate the memory and their behavior
For higher read concurrency, you can use ReadWriteLock
about volatile, jdk 9 java memory model update added a new api - VarHandle and there are many exotic accesses, in this case we are interested in acquire / release

The point is that the state between threads is dragged through the release/acqure state; volatile is stronger than this semantics (provides an order guarantee)
this is not the point, if you just have a producer-consumer, you can use (for performance) release / acqure, but no one forbids stronger semantics - volatile read / write

If you look into the implementation of the Atomic classes, you will see that reading and writing (get/set) happens to a volatile field or VarHandle.setVolatile/getVolatile

There is another situation when you are guaranteed to write and read only in one thread, then you don’t need any synchronization / atomic (volatile) at all

you can read more details here:
https://gee.cs.oswego.edu/dl/jmm/cookbook.html
but if you are not very familiar with concurrent, this can only confuse you more, in fact, when you do not need a lot of performance or concurrency is enough to coordinate read and write, this can be blocking or volatile (a.k.a synchronization, but for fields - (much faster OS lock))

[Stypox]

Thanks for the explanation, now I've understood a little bit more!

Checkstyle is failing because the code is not following our style.
At the moment I am not able to test whether the queue works correctly because it is bugged on dev...

[TobiGr]

• rebased
• fixed checkstyle
• applied review

[TobiGr]

I don't understand why the PlayQueueTest.SetIndexTests.addToHistory() test fails. It passed successfully locally.