Bug in set_state/_lock_property

[maartenbreddels]

There seems to be a bug in the set_state/_lock_property mechanism that is very subtle, it goes like this:

• For the AnnoyingWidget below, an update comes from the frontend setting value to 42, triggering set_state (mimicked in the unittest).
• Widget.set_state uses the _lock_property context manager, which sets `_property_lock = {'value': 42}, causing the trait 'value' to be set to 42, not to propagate to the frontend (makes sense).
• Now AnnoyingWidget. _propagate_value sets .value, to 2, which will be propagated to the frontend.
• Directly after, the widget sets its value to 42 again, which does not get propagated to the frontend (because the _property_lock dict is not up to date).

Now the frontend and backend are out of sync, which is something that can and does happen with the ToggleButtons.

The fix is to update the _property_lock dict.

[jasongrout]

Should the property lock be updated in send state instead? It represents the concept of the value in the frontend.

[maartenbreddels]

Yes, makes more sense, good idea.

[jasongrout]

Thanks. I'd probably put it after the message is actually sent, so the update only happens if the send was successful and didn't throw an error.

[vidartf]

My only concern about this is that a sync hold might interfere with it? Other than that, this feels like another band-aid on the logic.

As far as I understand it, the logic when the data is set from the kernel side, goes like this:

1. The traitlet validators are run (might modify the value). If no notification hold exists, cross validators are also run.
2. The traitlet values are set.
3. Zero or more trait notification attempt are made. If no notification hold is in place it happens immediately. Otherwise, new values might be collapsed before it is released. Once released:
   1. Cross validators are run once for each changed trait.
   2. The traitlet validators are run again.
   3. The traitlet values are set again.
   4. Each changed traitlet generates a trait notification
4. For each trait notification:
   1. The widget sends an update message to the front-end (unless a sync hold is applied).
   2. The observers of the traitlets are notified.
5. If a sync hold was in effect, a single update message is sent to the front-end once released.

When a state update is received from the front-end, the ideal logic should follow this pattern:

1. Set the traits to the received values.
2. After setting the traits, changes resulting from validation or observers during the previous step should be detected.
3. Any such changes detected should be synced back.

Given these two patterns, I'm considering whether this issue might not simply be solved by holding the sync while setting the state from the front-end, and then when the lock is released, remove any parts of the state update that is identical to what was received.

Edit: I commented this before I was fully done looking into it, so consider this a WIP

[vidartf]

I tested this out a bit, and whether or not my proposed fix is acceptable or not depends on whether one expects such a case as the test case in this PR to trigger two updates to the front-end, or none. As such, it would be nice if there was a more realistic use case to consider. @maartenbreddels

[vidartf]

Or put another way: For the ideal logic, should step 2 differentiate between observers and validators? I.e. should the actions of observers always happen after the state has been sent (current behavior)?

[maartenbreddels]

band-aid on the logic.

I totally agree, it does not feel like this PR (except from the unittest) will simplify the code, it took me a while to understand it (not sure I fully do now), so any simplification we can make would be great for the future and readablility.

As such, it would be nice if there was a more realistic use case to consider

it is a realistic use case, this is what is happening with the ToggleButtons actually. Although I want to change that behaviour, I'd first like to resolve the issue so it doesn't pop up again.

I believe you are more comfortable at @vidartf , so if you'd like to push to this branch (don't force push so we can can back) feel free.

[vidartf]

I think we can merge this for now, and consider using hold_sync during set_state in a later issue.

[SylvainCorlay]

Note: we had an in-person discussion about this with vidaar and maarten and decided to include this fix.

A change to the anyi-bounce logic should be coming for the next major version.