Fix frontends getting out of sync.

As described in #3111 it can happen that a frontend can get out of sync
due to not echoing back messages from the frontend.

Widgets can opt out of this behaviour if it is too costly and/or does
not make sense, e.g. the data from the file widget.

Fixes #3111

python/ipywidgets/ipywidgets/widgets/tests/test_set_state.py

@@ -80,7 +80,7 @@ def test_set_state_simple():
         c=[False, True, False],
     ))
 
-    assert w.comm.messages == []
+    assert len(w.comm.messages) == 1
 
 
 def test_set_state_transformer():
@@ -105,7 +105,7 @@ def test_set_state_data():
         a=True,
         d={'data': data},
     ))
-    assert w.comm.messages == []
+    assert len(w.comm.messages) == 1
 
 
 def test_set_state_data_truncate():
@@ -124,7 +124,7 @@ def test_set_state_data_truncate():
         data=dict(
             buffer_paths=[['d', 'data']],
             method='update',
-            state=dict(d={})
+            state=dict(d={}, a=True)
         )))
 
     # Sanity:
@@ -144,8 +144,8 @@ def test_set_state_numbers_int():
         i = 3,
         ci = 4,
     ))
-    # Ensure no update message gets produced
-    assert len(w.comm.messages) == 0
+    # Ensure one update message gets produced
+    assert len(w.comm.messages) == 1
 
 
 def test_set_state_numbers_float():
@@ -156,8 +156,8 @@ def test_set_state_numbers_float():
         cf = 2.0,
         ci = 4.0
     ))
-    # Ensure no update message gets produced
-    assert len(w.comm.messages) == 0
+    # Ensure one update message gets produced
+    assert len(w.comm.messages) == 1
 
 
 def test_set_state_float_to_float():
@@ -167,8 +167,8 @@ def test_set_state_float_to_float():
         f = 1.2,
         cf = 2.6,
     ))
-    # Ensure no update message gets produced
-    assert len(w.comm.messages) == 0
+    # Ensure one message gets produced
+    assert len(w.comm.messages) == 1
 
 
 def test_set_state_cint_to_float():
@@ -235,6 +235,7 @@ def _propagate_value(self, change):
     # this mimics a value coming from the front end
     widget.set_state({'value': 42})
     assert widget.value == 42
+    assert widget.stop is True
 
     # we expect no new state to be sent
     calls = []
@@ -268,3 +269,90 @@ def _propagate_value(self, change):
 
     calls = [call42]
     widget._send.assert_has_calls(calls)
+
+
+
+def test_echo():
+    # we always echo values back to the frontend
+    class ValueWidget(Widget):
+        value = Float().tag(sync=True)
+
+    widget = ValueWidget(value=1)
+    assert widget.value == 1
+
+    widget._send = mock.MagicMock()
+    # this mimics a value coming from the front end
+    widget.set_state({'value': 42})
+    assert widget.value == 42
+
+    # we expect this to be echoed
+    msg = {'method': 'update', 'state': {'value': 42.0}, 'buffer_paths': []}
+    call42 = mock.call(msg, buffers=[])
+
+    calls = [call42]
+    widget._send.assert_has_calls(calls)
+
+
+def test_echo_single():
+    # we always echo multiple changes back in 1 update
+    class ValueWidget(Widget):
+        value = Float().tag(sync=True)
+        square = Float().tag(sync=True)
+        @observe('value')
+        def _square(self, change):
+            self.square = self.value**2
+
+    widget = ValueWidget(value=1)
+    assert widget.value == 1
+
+    widget._send = mock.MagicMock()
+    # this mimics a value coming from the front end
+    widget._handle_msg({
+        'content': {
+            'data': {
+                'method': 'update',
+                'state': {
+                    'value': 8,
+                }
+            }
+        }
+    })
+    assert widget.value == 8
+    assert widget.square == 64
+
+    # we expect this to be echoed
+    msg = {'method': 'update', 'state': {'square': 64, 'value': 8.0}, 'buffer_paths': []}
+    call = mock.call(msg, buffers=[])
+
+    calls = [call]
+    widget._send.assert_has_calls(calls)
+
+
+def test_no_echo():
+    # in cases where values coming fromt the frontend are 'heavy', we might want to opt out
+    class ValueWidget(Widget):
+        value = Float().tag(sync=True, no_echo=True)
+
+    widget = ValueWidget(value=1)
+    assert widget.value == 1
+
+    widget._send = mock.MagicMock()
+    # this mimics a value coming from the front end
+    widget._handle_msg({
+        'content': {
+            'data': {
+                'method': 'update',
+                'state': {
+                    'value': 42,
+                }
+            }
+        }
+    })
+    assert widget.value == 42
+
+    # widget._send.assert_not_called(calls)
+    widget._send.assert_not_called()
+
+    # a regular set should sync to the frontend
+    widget.value = 43
+    widget._send.assert_has_calls([mock.call({'method': 'update', 'state': {'value': 43.0}, 'buffer_paths': []}, buffers=[])])

python/ipywidgets/ipywidgets/widgets/widget.py

@@ -415,8 +415,8 @@ def get_view_spec(self):
     def _default_keys(self):
         return [name for name in self.traits(sync=True)]
 
-    _property_lock = Dict()
     _holding_sync = False
+    _holding_sync_from_frontend_update = False
     _states_to_send = Set()
     _msg_callbacks = Instance(CallbackDispatcher, ())
 
@@ -497,10 +497,6 @@ def send_state(self, key=None):
         """
         state = self.get_state(key=key)
         if len(state) > 0:
-            if self._property_lock:  # we need to keep this dict up to date with the front-end values
-                for name, value in state.items():
-                    if name in self._property_lock:
-                        self._property_lock[name] = value
             state, buffer_paths, buffers = _remove_buffers(state)
             msg = {'method': 'update', 'state': state, 'buffer_paths': buffer_paths}
             self._send(msg, buffers=buffers)
@@ -549,10 +545,8 @@ def _compare(self, a, b):
 
     def set_state(self, sync_data):
         """Called when a state is received from the front-end."""
-        # The order of these context managers is important. Properties must
-        # be locked when the hold_trait_notification context manager is
-        # released and notifications are fired.
-        with self.hold_sync(), self._lock_property(**sync_data), self.hold_trait_notifications():
+        # maybe self.hold_sync()
+        with self._hold_sync_frontend(), self.hold_trait_notifications():
             for name in sync_data:
                 if name in self.keys:
                     from_json = self.trait_metadata(name, 'from_json',
@@ -598,10 +592,14 @@ def notify_change(self, change):
         # Send the state to the frontend before the user-registered callbacks
         # are called.
         name = change['name']
-        if self.comm is not None and self.comm.kernel is not None:
-            # Make sure this isn't information that the front-end just sent us.
-            if name in self.keys and self._should_send_property(name, getattr(self, name)):
-                # Send new state to front-end
+        if self.comm is not None and self.comm.kernel is not None and name in self.keys:
+            if self._holding_sync:
+                # if we're holding a sync, we will only record which trait was changed
+                # but we skip those traits marked no_echo, during an update from the frontend
+                if not (self._holding_sync_from_frontend_update and self.trait_metadata(name, 'no_echo')):
+                    self._states_to_send.add(name)
+            else:
+                # otherwise we send it directly
                 self.send_state(key=name)
         super().notify_change(change)
 
@@ -612,21 +610,6 @@ def __repr__(self):
     # Support methods
     #-------------------------------------------------------------------------
 
-    @contextmanager
-    def _lock_property(self, **properties):
-        """Lock a property-value pair.
-
-        The value should be the JSON state of the property.
-
-        NOTE: This, in addition to the single lock for all state changes, is
-        flawed.  In the future we may want to look into buffering state changes
-        back to the front-end."""
-        self._property_lock = properties
-        try:
-            yield
-        finally:
-            self._property_lock = {}
-
     @contextmanager
     def hold_sync(self):
         """Hold syncing any state until the outermost context manager exits"""
@@ -641,27 +624,19 @@ def hold_sync(self):
                 self.send_state(self._states_to_send)
                 self._states_to_send.clear()
 
-    def _should_send_property(self, key, value):
-        """Check the property lock (property_lock)"""
-        to_json = self.trait_metadata(key, 'to_json', self._trait_to_json)
-        if key in self._property_lock:
-            # model_state, buffer_paths, buffers
-            split_value = _remove_buffers({ key: to_json(value, self)})
-            split_lock = _remove_buffers({ key: self._property_lock[key]})
-            # A roundtrip conversion through json in the comparison takes care of
-            # idiosyncracies of how python data structures map to json, for example
-            # tuples get converted to lists.
-            if (jsonloads(jsondumps(split_value[0])) == split_lock[0]
-                and split_value[1] == split_lock[1]
-                and _buffer_list_equal(split_value[2], split_lock[2])):
-                if self._holding_sync:
-                    self._states_to_send.discard(key)
-                return False
-        if self._holding_sync:
-            self._states_to_send.add(key)
-            return False
+    @contextmanager
+    def _hold_sync_frontend(self):
+        """Same as hold_sync, but will not sync back traits tagged as no_echo"""
+        if self._holding_sync_from_frontend_update is True:
+            with self.hold_sync():
+                yield
         else:
-            return True
+            try:
+                self._holding_sync_from_frontend_update = True
+                with self.hold_sync():
+                    yield
+            finally:
+                self._holding_sync_from_frontend_update = False
 
     # Event handlers
     @_show_traceback
@@ -684,7 +659,8 @@ def _handle_msg(self, msg):
         # Handle a custom msg from the front-end.
         elif method == 'custom':
             if 'content' in data:
-                self._handle_custom_msg(data['content'], msg['buffers'])
+                with self._hold_sync_frontend():
+                    self._handle_custom_msg(data['content'], msg['buffers'])
 
         # Catch remainder.
         else:

python/ipywidgets/ipywidgets/widgets/widget_upload.py

@@ -133,7 +133,7 @@ class FileUpload(DescriptionWidget, ValueWidget, CoreWidget):
     style = InstanceDict(ButtonStyle).tag(sync=True, **widget_serialization)
     error = Unicode(help='Error message').tag(sync=True)
     value = TypedTuple(Dict(), help='The file upload value').tag(
-        sync=True, **_value_serialization)
+        sync=True, no_echo=True, **_value_serialization)
 
     @default('description')
     def _default_description(self):