From f4d07117d097840a112ffc2a66fc480dfc7fe136 Mon Sep 17 00:00:00 2001 From: Miha_x64 Date: Sun, 12 Aug 2018 01:00:11 +0300 Subject: [PATCH] re-binding sequentially, potentially fixes #40 --- .../aquadc/properties/internal/-Listeners.kt | 42 +++++++-- .../properties/internal/ConcListeners.kt | 22 +++-- .../properties/internal/ConcMutable-.kt | 87 +++++++++---------- .../net/aquadc/properties/internal/utils.kt | 2 +- 4 files changed, 92 insertions(+), 61 deletions(-) diff --git a/properties/src/main/kotlin/net/aquadc/properties/internal/-Listeners.kt b/properties/src/main/kotlin/net/aquadc/properties/internal/-Listeners.kt index faf1e6ca..c59a51e7 100644 --- a/properties/src/main/kotlin/net/aquadc/properties/internal/-Listeners.kt +++ b/properties/src/main/kotlin/net/aquadc/properties/internal/-Listeners.kt @@ -399,16 +399,25 @@ abstract class `-Listeners` : AtomicReferen } private fun concChangeObservedStateTo(obsState: Boolean) { - val firsState = concState().getUndUpdate { prev -> + var prev: ConcListeners + var next: ConcListeners + do { + prev = concState().get() + while (prev.transitionLocked) { + // if we can't transition for external reasons (e. g. in ConcMutableProperty), just wait until this ends + Thread.yield() + prev = concState().get() + } + if (prev.nextObservedState == obsState) { // do nothing if we're either transitioning to current state or already there return } - prev.startTransition() - } + next = prev.startTransition() + } while (!concState().compareAndSet(prev, next)) - if (firsState.transitioningObservedState) { + if (prev.transitioningObservedState) { // do nothing if this method is already on the stack somewhere return } @@ -433,7 +442,30 @@ abstract class `-Listeners` : AtomicReferen } } - /*...not overridden in [ConcMutableDiffProperty], because it is not mapped and cannot be bound. */ + /* intentionally does not contain try..catch */ + internal inline fun withLockedTransition(block: () -> Unit) { + while (!tryLockTransition()) Thread.yield() + block() + unlockTransition() + } + + internal fun tryLockTransition(): Boolean { + val prev = concState().get() + if (prev.transitionLocked || prev.transitioningObservedState) { + return false + } + + return concState().compareAndSet(prev, prev.flippedTransitionLock()) + } + + internal fun unlockTransition() { + val old = concState().getAndUpdate(ConcListeners::flippedTransitionLock) + check(old.transitionLocked) + } + + /*...not overridden in [ConcMutableDiffProperty], because it is not mapped and cannot be bound. + * This callback can be called only once at a time (i. e. under mutex) for a single property, + * and with transitionLocked=false */ internal open fun observedStateChanged(observed: Boolean) {} @Suppress("NOTHING_TO_INLINE", "UNCHECKED_CAST") diff --git a/properties/src/main/kotlin/net/aquadc/properties/internal/ConcListeners.kt b/properties/src/main/kotlin/net/aquadc/properties/internal/ConcListeners.kt index 58eb0d27..4b7e826e 100644 --- a/properties/src/main/kotlin/net/aquadc/properties/internal/ConcListeners.kt +++ b/properties/src/main/kotlin/net/aquadc/properties/internal/ConcListeners.kt @@ -12,11 +12,12 @@ internal class ConcListeners( @JvmField val listeners: Array, @JvmField val pendingValues: Array, @JvmField val transitioningObservedState: Boolean, - @JvmField val nextObservedState: Boolean + @JvmField val nextObservedState: Boolean, + @JvmField val transitionLocked: Boolean ) { fun withListener(newListener: @UnsafeVariance L): ConcListeners = - ConcListeners(listeners.with(newListener) as Array, pendingValues, transitioningObservedState, nextObservedState) + ConcListeners(listeners.with(newListener) as Array, pendingValues, transitioningObservedState, nextObservedState, transitionLocked) fun withoutListenerAt(idx: Int): ConcListeners { val newListeners = when { @@ -36,11 +37,11 @@ internal class ConcListeners( return if (pendingValues.isEmpty() && newListeners.isEmpty() && !transitioningObservedState && !nextObservedState) NoListeners else - ConcListeners(newListeners, pendingValues, transitioningObservedState, nextObservedState) + ConcListeners(newListeners, pendingValues, transitioningObservedState, nextObservedState, transitionLocked) } fun withNextValue(newValue: @UnsafeVariance T): ConcListeners = - ConcListeners(listeners, pendingValues.with(newValue) as Array, transitioningObservedState, nextObservedState) + ConcListeners(listeners, pendingValues.with(newValue) as Array, transitioningObservedState, nextObservedState, transitionLocked) fun next(): ConcListeners { val listeners = if (this.pendingValues.size == 1) { @@ -51,17 +52,20 @@ internal class ConcListeners( } // remove value at 0, that listeners were just notified about - return ConcListeners(listeners, pendingValues.copyOfWithout(0, EmptyArray) as Array, transitioningObservedState, nextObservedState) + return ConcListeners(listeners, pendingValues.copyOfWithout(0, EmptyArray) as Array, + transitioningObservedState, nextObservedState, transitionLocked) } - fun startTransition(): ConcListeners { - return ConcListeners(listeners, pendingValues, true, !nextObservedState) - } + fun startTransition(): ConcListeners = + ConcListeners(listeners, pendingValues, true, !nextObservedState, transitionLocked) fun continueTransition(appliedState: Boolean): ConcListeners { check(transitioningObservedState) val done = appliedState == nextObservedState - return ConcListeners(listeners, pendingValues, !done, nextObservedState) + return ConcListeners(listeners, pendingValues, !done, nextObservedState, transitionLocked) } + fun flippedTransitionLock(): ConcListeners = + ConcListeners(listeners, pendingValues, transitioningObservedState, nextObservedState, !transitionLocked) + } diff --git a/properties/src/main/kotlin/net/aquadc/properties/internal/ConcMutable-.kt b/properties/src/main/kotlin/net/aquadc/properties/internal/ConcMutable-.kt index 434a0378..188bb041 100644 --- a/properties/src/main/kotlin/net/aquadc/properties/internal/ConcMutable-.kt +++ b/properties/src/main/kotlin/net/aquadc/properties/internal/ConcMutable-.kt @@ -16,47 +16,45 @@ internal class `ConcMutable-`( true, value ), MutableProperty, ChangeListener { - override var value: T // field: T | Binding | rebinding() + override var value: T // field: T | Binding get() { - var value: Any? - do { value = valueUpdater().get(this) } while (value === rebinding()) - return if (value is Binding<*>) (value as Binding).original.value else value as T + val value = valueUpdater().get(this) + return if (value is Binding<*>) { + value as Binding + val retValue: T + if (!isBeingObserved()) { + // we're not observed, so stale value may be remembered — update it + retValue = value.original.value + (value as Binding).ourValue = retValue + retValue + } else { + value.ourValue as T + } + } else { + value as T + } } set(newValue) { while (!casValue(value, newValue)) Thread.yield() } override fun bindTo(sample: Property) { - val newValue: Any? - val newSample: Property? - if (sample.mayChange) { - newValue = Binding(sample) - newSample = sample - } else { - newValue = sample.value - newSample = null - } + val newValOrBinding: Any? = if (sample.mayChange) Binding(sample, sample.value) else sample.value + val oldValOrBinding = valueUpdater().get(this) - var oldValue: Any? - while (true) { - oldValue = valueUpdater().getAndSet(this, rebinding()) - if (oldValue === rebinding()) Thread.yield() // other thread rebinding this property, wait - else break - } - // under mutex + val prevValue = if (oldValOrBinding is Binding<*>) (oldValOrBinding as Binding).original.value else oldValOrBinding as T + val newValue = if (newValOrBinding is Binding<*>) (newValOrBinding as Binding).original.value else newValOrBinding as T - // fixme: potential concurrent bug, #40 - if ((oldValue !is Binding<*> || oldValue.original !== newSample) && isBeingObserved()) { - (oldValue as? Binding)?.original?.removeChangeListener(this) - newSample?.addUnconfinedChangeListener(this) + withLockedTransition { // 'observed' state should not be changed concurrently + if (isBeingObserved()) { + (oldValOrBinding as? Binding)?.original?.removeChangeListener(this) + (newValOrBinding as? Binding)?.original?.addUnconfinedChangeListener(this) + } + (newValOrBinding as? Binding)?.ourValue = newValue + valueUpdater().set(this, newValOrBinding) } - // end mutex - check(valueUpdater().getAndSet(this, newValue) === rebinding()) - val prevValue = if (oldValue is Binding<*>) (oldValue as Binding).original.value else oldValue as T - val newValueValue = if (newValue is Binding<*>) (newValue as Binding).original.value else newValue as T - - valueChanged(prevValue, newValueValue, null) + valueChanged(prevValue, newValue, null) } override fun observedStateChanged(observed: Boolean) { @@ -70,48 +68,45 @@ internal class `ConcMutable-`( override fun casValue(expect: T, update: T): Boolean { val prevValOrBind = valueUpdater().get(this) - if (prevValOrBind === rebinding()) return false val prevValue: T val realExpect: Any? if (prevValOrBind is Binding<*>) { prevValOrBind as Binding - if (!valueUpdater().compareAndSet(this, prevValOrBind, rebinding())) return false + if (!tryLockTransition()) return false // under mutex - prevValue = prevValOrBind.original.value + prevValue = prevValOrBind.ourValue as T // hmm, where's my smart-cast? if (prevValue !== expect) { // under mutex (no update from Sample allowed) we understand that sample's value !== expected - check(valueUpdater().compareAndSet(this, rebinding(), prevValOrBind)) - // so just revert and report failure + unlockTransition() + // so just report failure return false } - realExpect = rebinding() + prevValOrBind.original.removeChangeListener(this) + unlockTransition() + realExpect = prevValOrBind } else { prevValue = prevValOrBind as T realExpect = expect } - val success = valueUpdater().compareAndSet(this, realExpect, update) // end mutex + val success = valueUpdater().compareAndSet(this, realExpect, update) if (success) { valueChanged(prevValue, update, null) - } else if (realExpect === rebinding()) { - // if we're set a mutex, we must release it; if it is not set, there's a program error - throw AssertionError() } return success } override fun invoke(old: T, new: T) { - while (value === rebinding()) Thread.yield() - valueChanged(old, new, null) + withLockedTransition { + (valueUpdater().get(this) as? Binding)?.ourValue = new + valueChanged(old, new, null) + } } - private class Binding(val original: Property) - - @Suppress("NOTHING_TO_INLINE") - private inline fun rebinding() = Unset + private class Binding(val original: Property, @Volatile var ourValue: T) @Suppress("NOTHING_TO_INLINE", "UNCHECKED_CAST") private inline fun valueUpdater() = diff --git a/properties/src/main/kotlin/net/aquadc/properties/internal/utils.kt b/properties/src/main/kotlin/net/aquadc/properties/internal/utils.kt index 5cb03b7c..4d049e7e 100644 --- a/properties/src/main/kotlin/net/aquadc/properties/internal/utils.kt +++ b/properties/src/main/kotlin/net/aquadc/properties/internal/utils.kt @@ -74,7 +74,7 @@ val Unset: Any = Any() emptyArray() @[JvmField JvmSynthetic] internal val NoListeners = - ConcListeners(EmptyArray, EmptyArray, false, false) as ConcListeners + ConcListeners(EmptyArray, EmptyArray, false, false, false) as ConcListeners @[JvmField JvmSynthetic PublishedApi] internal val TRUE = `Immutable-`(true) @[JvmField JvmSynthetic PublishedApi] internal val FALSE = `Immutable-`(false)