nit: probably should be oldState to be consistent with naming convention used in newState.

Or maybe for the clarity we can just inline FlagsClientState.NotReady where needed, because stateOld is val anyway and cannot be changed.

getCurrentStateAttempted is not actually needed I think, the necessary wait for the getCurrentStateThread completion is already implemented by the getCurrentStateThread.join() call below.

synchronized calls can be removed if type here is CopyOnWriteArrayList

Why do we want to block getCurrentState when addListener is called, if addListener is not changing the state?

🤔 We may have had the right lock here in the first place...

If addListener holds the read lock, then, ostensibly, nothing can write to the state (current state or the listeners). While addListener is technically mutating the protected state by adding a listener, the underlying mechanism itself is theadsafe/synchronized via CopyOnWriteArrayList so it's fine if parallel calls to addListener attempt to mutate the set of listeners. The order of listener subscription is not important to maintain here.

We should just be able to rely on the underlying thread-safety of CopyOnWriteArrayList to avoid clobbering of the listener list, right?

Maybe we shouldn't wrap adding listener with lock then?

We need to block writing to the current state until the first notification and addListener are complete. We don't get that atomicity with DDCoreSubscription, only that the calls underlying calls to add the listener to the list are thread safe and synchronized so no listener will be lost when parallel calls are made.

In effect, I believe we can drop this change altogether and rely on the read lock in conjunction with the underlying lock in CopyOnWriteArrayList

I think yes, we should. My thinking is the following:

• getCurrentState covers only FlagsStateManager.currentState. Listeners is not a part of FlagsStateManager.currentState, so they are not related to the getCurrentState call.
• FlagsStateManager.addListener call doesn't modify FlagsStateManager.currentState, it only reads. So that us why we added a read lock. If something is in the process of modifying FlagsStateManager.currentState, then there will be a wait to acquire read lock.
• FlagsStateManager.addListener mutates underlying listeners collection, but anyway this is atomic and thread-safe, currentState value passed down is guarded by the read lock and if there is iteration already happening in updateState -> subscription.notifyListeners, then this new listener won't be a part of ongoing iteration (due to the usage of CopyOnWriteArrayList in DDSubscription).

So I'm curious why should we block getCurrentState call during FlagsStateManager.addListener invocation.

The race condition could otherwise result in a listener missing a state change

Is it about FlagsStateManager.currentState property here? From the code I see, listener shouldn't miss any state change, since read lock cannot be acquired if write lock is active, and the opposite.

Well, having read it more carefully, I agree with @nikita. The only thing that still makes me curious is the fact that we adding listener with a lock, but removing without. Yes, underlying CopyOnWriteArrayList will protect us from the ConcurrentModificationException, however if updateState and removeListener will be executed at the same time, removed listener could get an undesired update (from subscription.notifyListeners ) and could lead to undefined behavior. Not sure if this is a critical issue, but I'd prefer to maintain consistency here.

Thanks @satween. Agreed that there's an apparent lack of synchronicity on removing a listener.

We are not just "Adding a listener", however.
We need to

• get the current state
• notify the listener
• add the listener to the list
  All without the currentState changing, so we block changes with a read lock.

When we remove the listener, have only that operation to complete so there isn't a risk of missing data, only, as you noted, receiving extra data. We would need to add an extra synchronization shared between the removeListener method and the updateState method. I think you're correct here though - a long-running listener callback could cause a removed listener to be notified after the call to removeListener completes

The scale of adding/removing listeners will be very low (0, or 1 or maybe a few) so there is a very small risk here of a superfluous event.

I'll come back to this with an updated lock after a couple of other tasks

Updated to use a read lock on both add and remove listener to ensure that state changes are not sent to removed listeners

I don't quite understand why do we need synchronized in the assertions block. Assertions are supposed to be executed only when everything is completed and updateStateThread.join() should guarantee safe read without this block anyway (and it is not needed if CopyOnWriteArrayList is used as well).

A bit more fair will be to use CopyOnWriteArrayList for notificationsAfterRemove instead of using synchronized here.

nit: having newState and stateNew is a bit confusing. Maybe we should rename stateNew to readyState?

nit: the line above already imposes that initialStateTime is less than updatedStateTime. If it is not the case, this line won't be even executed.