ObservableValueHolder>>value: lock instVar can get stuck to true

[ericwinger]

Bug description Occasionally, I see a Spec2 list seem to get "disconnected" and the rest of the gui wouldn't update after a selection change.

I chased the problem down and eventually I observed that SpMultipleSelectionMode>>selectIndexes: wasn't honoring the subscription blocks in the selectedIndexes ObservableSlot. Turns out that the lock inst var in the ObservableValueHolder would get stuck to true. If lock gets stuck to true, the gui won't update anymore. The culprit may be that there is a tiny window where if a process in ObservableValueHolder>>#value: is terminated after lock is set to true, but before the ensure block is invoked, lock will forever be true. See code below.

value: anObject
    "Handle circular references as explained in the class comment"
    lock ifTrue: [ ^ self ].

    lock := true.   <<<<<<<< If the process is terminated after lock is set to true, but before the block is invoked, lock will forever be true. 

    [ | oldValue |
    oldValue := value.
    value := anObject.
    self valueChanged: oldValue ]
        ensure: [ lock := false ]

Some potential fixes:

1. Move "lock := true" code inside the ensure block.
2. Wrap the "lock := true" in another ensure block which runs the original ensure block.
3. Possibly use a #valueUninterruptably (but that could have unintended side effects)

I'm experimenting with #1 and #2 solutions to see if they eliminate the problem.

To Reproduce Hard to reproduce, but you can try killing processes that are changing slot values.

Expected behavior Don't let lock get stuck in true so subscription blocks will always run

Screenshots None

Version information: Reproduced in Pharo 11. Code is identical in Pharo 12. Pharo 12.0.0 Build information: Pharo-12.0.0+SNAPSHOT.build.1516.sha.25ecf718d4a363b275862b4a093b1a240ec7e8d2 (64 Bit)

Expected development cost Shouldn't be too hard to fix if my analysis is correct.

Additional context Very hard to reproduce this problem but was impacting user severely.

[Ducasse]

THANKS a lot for your findings. I would explain why alain mentions problems with observeSlot. @plantec @tesonep @MarcusDenker

[Ducasse]

I do not understand what the assignment is not part of the ensure.