2

Closed

Default implementation ActionMessage.PrepareContext and thread affinity

description

The default implementation of the ActionMessage.PrepareContext does not enforce thread affinity whenever an action guard changes:
PropertyChangedEventHandler handler = null;
                handler = (s, e) =>
                {
                    if (string.IsNullOrEmpty(e.PropertyName) || e.PropertyName == guardName)
                    {
                        if (context.Message == null)
                        {
                            inpc.PropertyChanged -= handler;
                            return;
                        }
                        context.Message.UpdateAvailability();
                    }
                };
If a guard changes as a consequence of an asynchronous operation, this line
context.Message.UpdateAvailability();
will try to manipulate a DependencyObject, throwing an exception.

Wrapping such call into an Execute.OnUIThread seems to me the best approach, considering that property change notification are often silently marshalled on the UI thread (consider the Binding mechanism).

Note that such modification can be implemented as a customization, nevertheless I think that the current approach bears no benefits, while changing the default implementation causes no harm.
Closed Mar 10, 2013 at 3:01 AM by EisenbergEffect
Fixed

comments

tibel wrote Feb 20, 2013 at 6:35 PM

I thould PropertyChangedBase.NotifyOfPropertyChange() handles the switch to the UI thread already. Maybe you can attach a sample project that shows what is going wrong?

BladeWise wrote Feb 21, 2013 at 9:24 AM

I am using a class implementing just INotifyPropertyChanged as a view-model, due to some legacy code I am dealing with, so I am not using the CM implementation of NotifyPropertyChanged (and I have not access to the code itself).

That said, dispatching property changes on the UI thread is an overkill, since thread-affinity is ensured by the binding engine when a Binding is used (I am certain this is the case in WPF/Silverlight, I am not sure, but pretty confident, in WP7/8). The current implementation of NotifyPropertyChange will marshall all handlers on the UI, which is pretty much extreme and can lead to UI slow-downs in case a property changes as an effect of an asynchronous operation. Moreover, such behavior is not intuitive: if I change a property in an async task, I expect handlers being called on such thread, and it is my responsability to deal with concurrency issues (this is a standard behavior). I will probably create a separate issue about this, so we can continue this discussion in deep (maybe there are reason I am not aware of).

Back to the topic, since the ActionMessage does not deal exclusively with PropertyChangedBase objects, just with plain INotifyPropertyChanged objects, it is up to the class itself to ensure thread-affinity. In general, it is my opinion that whatever class deals directly with UI objects, has to deal with thread affinity directly, without relying on the fact that the caller has already dispatched the call onthe UI thread.

tibel wrote Feb 26, 2013 at 6:04 PM

Do you have a small sample that shows the issue or maybe already the fix?

BladeWise wrote Feb 26, 2013 at 7:00 PM

The fix is just to wrap context.Message.UpdateAvailability() in an Execute.OnUIThread. That ensures that a generic View-model can ne used.

BladeWise wrote Mar 8, 2013 at 5:15 PM

@Tibel: I noticed one problem today, with the fix I proposed. There is a concurrency issue with the current code, whenever the handler is executed on a thread different from the UI: message.Context can be not null at the time it is evaluated, but then it could be when dispatched on the UI thread. To ensure consistency, it is better to wrap the entire handler in an Execute.OnUIThread call, and keep a local reference to the context. Below, the proposed code:
handler = (s, e) => {
                     Execute.OnUIThread(() => { 
                        if (string.IsNullOrEmpty(e.PropertyName) || e.PropertyName == guardName) {
                                                var message = context.Message;
                                                if (message == null) {
                                                    ((INotifyPropertyChanged)s).PropertyChanged -= handler;
                                                    return;
                                                }
                       message.UpdateAvailability(); });
                    }
Sorry for the late fix, unfortunally I just noticed the problem stress-testing the new version from sources.

tibel wrote Mar 9, 2013 at 6:27 AM

Changed with 5f4e82b41769.

It is a little different than your proposed fix, as I don't want to transition to the UI thread when the event is not raised for the guard property.
May you stress-test this version again, please.

BladeWise wrote Mar 9, 2013 at 9:26 AM

Definitely better than my proposed code. My tests did not show any problem. Is it possible that this fix solves the NRE exception reported by this work item?