Toggle repository potential synchronization issue

[maorelias]

Maybe I'm missing something, but it seems like the periodic update of the feature toggle repository is not guaranteed to synchronize cross thread. Here the toggle collection is reassigned in the periodic update routine, but it's not volatile and the update is not performed in the context of a lock (as far as I could see). Since other threads (of the app) may perform read operations later, they don't seem to be guaranteed to read the latest copy of the toggles? (e.g. if the local cache of the reading thread is not invalidated).

Thanks in advance!

[ivarconr]

Hi there,

I do not see how this can be a problem? You should only instantiate a single instance of the SDK in your application. The toggle collection itself is never exposed out, and only the value or a copy if it is returned back to the calling function. The periodic fetching happens in the background on a single thread.

If you put any cache between the SDK and the function calling the SDK you will be required to invalidate that cache yourself.

[maorelias]

Hi, In the case I mentioned - the reading thread will be the app thread trying to check if some toggle is enabled (via the API) and the writing thread is the single thread executing the periodic refresh. As far as I can see, there is no synchronization between them and so the reading thread might always be reading old copies of the toggle collection. Am I missing something?

[ivarconr]

I really do not understand how this would be a problem?

The private member field "toggleCollection" is a shared resource between the "reading thread" and the periodic background checker. If updates are detected the pointer is swapped with the new "toggleCollection".

Synchronization in java is used to control access to "shared resources". Because swapping the pointer is a single operation, and we do not care if someone else is reading while swapping (they might get an old value in the transition), but because the pointer is changed, all reading threads will get the updated values, after the pointer is swapped.

Are you able to provide example code that demonstrates your concern and how this is not working as you would expect?

[maorelias]

but because the pointer is changed, all reading threads will get the updated values, after the pointer is swapped.

Why are the reading threads guaranteed to get the updated values? This depends on whether their (= the threads') local cache is invalidated or not, which can very between different processor architectures and applications? I have a strong feeling that the collection needs to be declared as volatile (or some other equivalent) as to guarantee that the reading thread will always read the most recent copy?

[checketts]

I believe @maorelias Is correct. @ivarconr when different threads in java are accessing a variable that could change, there is chance that a thread could read a stale (cached) value. (By 'cached' that refers to a CPU processor cache)

I don't think there is a very large risk here, but adding volatile or using an AtomicReference would make the code more correct.

Here is a good summary on volatile and its impact on cpu/memory. (https://www.baeldung.com/java-volatile)

[ivarconr]

Thanks @checketts, I have read up on Java Threading and I think there is a potential risk here, but I assume in most cases it will correct itself fast enough in practice, and that is why this has never been brought up before.

I agree that adding volatile could be a good way to increase the read guarantees.

Anyone up for a PR?

Thanks for putting this to our attention @maorelias 👍🏼

[checketts]

Feel free to assign the issue to me. I have another PR I'll be working on soon. (Unless @maorelias wants to tackle this)

[maorelias]

@checketts go ahead :) thanks!