Consider update and accumulate methods on Property

[kaifox]

In addition to pure set() methods, a property could have update-like methods like an Atomic reference which should then ensure atomic updates of the value.

Proposed signatures:

Property<T>{
  update(UnaryOperator<T> updateOp);
  accumulate(T in, BinaryOperator<T> updateOp);

 // potentially also these variants (or only those)
  updateAndGet(UnaryOperator<T> updateOp);
  accumulateAndGet(T in, BinaryOperator<T> updateOp);
}

In principle, e.g. update would do the following in an atomic way:

T value = get();
set(updateOp.apply(value));

[andreacalia]

Ciao @kaifox ! Then it'll really become a "listenable" AtomicReference 😄

[michi42]

Sounds fine to me :-)

We would probably have to extend DispatchingObservableValue a bit to delegate these operations to the internal AtomicReference it holds (we should not introduce another one in Property I think). But since this is our own internal class - I don't see any problem with that.

Of course, the notification of subscribers will be non atomic, dispatched onto another thread.
The only guarantee will be that a subsequent get() will give the new value.

Edit: Another thing we have to be aware of - all our transformations are also async subscribers. So if you do

Property<T> property = ...;
ObservableValue<T> derived = property.map(...);
property.update(...);
derived.get();

it is not guaranteed that derived.get() will give you the new value.

[kaifox]

Yes indeed, then it is a listenable atomic reference ;-) ... Indeed we should not have a second atomic reference ... Yes and the listener notification and the derived will not be atomic... but I think it still is worth it ...

I use such constructs a lot e.g. when you want to publish a state of e.g. a service and one has to do consistent modifications (transformations) of it. And in principle you want to make sure that you do not mix inputs, but 'chain' them. So at the moment one either has to synchronize or enforce a single thread ... both some effort if you use the pattern more frequently ...

With an 'ObservableAtomicReference' it would be simpler ;-) ...

[michi42]

Yes, I agree, actually I always thought of Property as an "AtomicReference plus". So if you want to give it a try, please do.

[kaifox]

I will try (soon) ;-)

[kaifox]

Gi guys, I finally started trying this ... However, soon found a bit a showstopper:
With all these operations (accumulate, update, ... ) one either can get the old value OR the new value, but it is not possible to get both in a threadsafe consistent way. For the dispatching 'on change' we would need both ....

Currently, I have not idea how to solve this cleanly... Any proposals?

[michi42]

Perhaps we could use getAndUpdate() to fetch the previous value, and within the actual update callback we save the new value before we return?

[kaifox]

@michi42, do you mean more or less something like this?:

public T getAndUpdate(UnaryOperator<T> operator) {
  AtomicReference<T> newValue = new AtomicReference<>();
  T oldValue = lastValue.getAndUpdate(x -> {
     T v = operator.apply(x);
     newValue.set(v);
     return v;
  })
  dispatchChange(oldValue, newValue.get());
  return oldValue;
}

... It all looks a bit clumsy and I am completely unsure if it is bulletproof .... I am not sure, if e.g. the update operations might be retried etc...

[andreacalia]

Hi guys,

I didn't try it unfortunately.. but if void DispatchingObservableValue.dispatchValue(T) becomes something like T DispatchingObservableValue.updateAndDispatchValue(UnaryOperator<T>)? In the line of Michi's comment I think dispatch should happens atomically 🤔

[michi42]

Hi,

From what I see, the function can be applied multiple times, but it is always the last result that will be propagated - so the hack you do should be safe as far as I can see (in the current implementation of AtomicReference that is...)
It is a bit a pity that AtomicReference has no getUpdateAndGet() method, since all the information is there in the implementations ...

[kaifox]

Ok ... I had similar thoughts ... so if you consider it 'ok' I will try to make a proposal based on this...

[kaifox]

@andreacalia , the problem is that - for the update on change notification - one needs both, old and final value ...

[andreacalia]

true indeed ! I see it now the problem.
The solution is a bit convoluted but should work as mentioned by michi 😉 .. anyway I don't see other solutions without introducing many more changes 😇
@kaifox if you have a branch locally rebase first because there's an update that could affect you 🙂

[kaifox]

There is a PR out #62 ... but I am not sure ... did I mess up something with the rebase? damn ...

[andreacalia]

Ciao Kajetan, I think it's ok just rebase on top of master now and it should be perfectly in sync 🙂