Optionally UI Thread property change marshalling

[RafaelSalguero]

When combining property change notification with reactive observables there are situations where is desirable that the property change event is leaved as-is, on its original thread.

Since WPF already does UI Thread marshaling on bindings, would be useful to be able to turn off this behavior.

[superware]

Thanks @RafaelSalguero.

Hi @nigel-sampson, can you please consider merging?

We've discussed this a few months ago, but I'm still having issues, mainly with deadlocking on dispose:

public class MyViewModel : PropertyChangedBase, IDisposable
{
	private readonly object _sync = new object();
	private readonly IMyService _myService;
	private IMyEntity _myEntity;
	private string _myProperty;
	private string _myProperty2;

	public MyViewModel(IMyService myService)
	{
		_myService = myService;
		_myService.EntityAdded += MyService_EntityAdded;
	}

	private void MyService_EntityAdded(object sender, EntityAddedEventArgs e)
	{
		lock (_sync)
		{
			if (_myEntity != null)
			{
				_myEntity.SomethingChanged -= MyEntity_SomethingChanged;
			}

			_myEntity = e.MyEntity;

			if (_myEntity != null)
			{
				_myEntity.SomethingChanged += MyEntity_SomethingChanged;
			}
		}
	}

	private void MyEntity_SomethingChanged(object sender, EntitySomethingChangedEventArgs e)
	{
		lock (_sync)
		{
			MyProperty = e.MyProperty;
			MyProperty2 = e.MyProperty2; // <-- a working thread is stuck here, waiting for the UI thread which is now disposing
		}
	}

	public string MyProperty
	{
		get { return _myProperty; }
		private set { _myProperty = value; NotifyOfPropertyChange(); }
	}

	public string MyProperty2
	{
		get { return _myProperty2; }
		private set { _myProperty2 = value; NotifyOfPropertyChange(); }
	}

	public void Dispose()
	{
		lock (_sync) // <-- the UI thread is stuck here, waiting for Execute.OnUIThread
		{
			if (_myEntity != null)
			{
				_myEntity.SomethingChanged -= MyEntity_SomethingChanged;
			}

			_myService.EntityAdded -= MyService_EntityAdded;
		}
	}
}

Thank you in advance.

[nigel-sampson]

Sorry this one slipped through my radar, I've no objection to adding this sort of functionality to the framework.

One thing though is that I'm not sure adding that static to PropertyChangedBase is the best place. I'm always cautious when adding new members to the commonly used class in the framework.

I would consider adding it to IPlatformProvider since it's static and then be configured solution wide.

I would then add the same behaviour to BindableCollection for consistency.

[superware]

You simply rock! thanks.

[superware]

@RafaelSalguero Can you please revise this PR per @nigel-sampson recommendations? Thanks!

[superware]

Hi @nigel-sampson,

Can you please verify the issue for the code snippet I posted earlier? Am I missing something?

Thanks!

[nigel-sampson]

Which thread is the Dispose being called on?

[nigel-sampson]

Going to close this off with the discussion on #247 on how we'll proceed.

[superware]

Hi @nigel-sampson,

Dispose is being called on the UI thread (application shutdown), see comments in the code.

Thanks!

[nigel-sampson]

Ah, I'd be really hesitant to use locks on the UI thread for fear of holding stuff up.

[superware]

Once the application is being terminated, the IoC container is disposing all classes that implement IDisposable automatically... What would you put inside the VM's Dispose in my code example?

[superware]

I'm still having difficulties with PropertyChangedBase (and all inheriting classes) marshalling PropertyChanged to the UI thread.

For example, the application is being disposed on shutdown, if one of the service classes has a thread Join being called while that thread is changing a VM property, then Join will never complete and the application will never exit.

Why not simply add IsPropertyChangedUIMarshallingDisabled (default is false) in IPlatformProvider? isn't this MUCH better than implementing an IPlatformProvider where OnUIThread isn't actually executing something on the UI thread?

I wish there'll be a global way to disable this marshalling behaviour, which is completely unnecessary in WPF.

[nigel-sampson]

I'll have another look into this, thanks.

[superware]

Hi, did you get a chance to look into this?

[nigel-sampson]

It's on the list for 4.0.0 and will most likely be included, but not sure when I'll do the work.

[superware]

That's great news, I'll gladly test the PR.

[superware]

Hey @nigel-sampson, I hope it's not too rude, but may I ask whether 4.0.0 (2nd alpha?) is going to be released any time soon?

[nigel-sampson]

Not too rude at all, to be honest it's taking longer than expected mostly because I haven't had a lot of time spare to work on Caliburn.Micro in the last few months beyond triaging issues.

I'm not sure when that will change. Hopefully soon but I can't make any promises.

[superware]

@nigel-sampson Hey, as far as I understand this can be a simple non-breaking option, how do you see this? can I help with the implementation? Btw, couldn't find it on the 4.0.0 list :)

[nigel-sampson]

I've re-opened this as a reminder.

[superware]

Hey @nigel-sampson, any news regarding this? :|

How are you planning to implement this? Via IPlatformProvider? I'm eager to test it..

[nigel-sampson]

This has now been included on master and available through the pre-release feed.

[superware]

Hi @nigel-sampson, can you please release a new alpha?

[nigel-sampson]

Will see what I can do, any reason you can't use the MyGet feed?

[superware]

I prefer to keep the NuGet chain of reference. Anyway, is there a time table for the next alpha/beta/release? Thanks.

[nigel-sampson]

4.0.105-alpha has just been published.

[superware]

Hi @nigel-sampson, how are you?

It seems PropertyChangeNotificationsOnUIThread isn't being checked on ActionMessage's PrepareContext "Guard Names" mechanism.

The only workaround I found is

ActionMessage.BuildPossibleGuardNames = m => { return Array.Empty<string>(); };

On bootstrapper ctor, but it's a hack in order to disable that feature.

[nigel-sampson]

Thanks, will look into this.

[nigel-sampson]

Can you expand on what problem you're seeing here @superware ?

[superware]

ActionMessage is subscribing to PropertyChanged, and the handler is calling Execute.OnUIThread without first checking the platform provider's PropertyChangeNotificationsOnUIThread. If it's false it should update the availability immediately. In my case I'm getting a deadlock, which PropertyChangeNotificationsOnUIThread was designed to prevent on WPF.

[nigel-sampson]

Ok, now I get you, it feels weird to be checking PropertyChangeNotificationsOnUIThread inside ActionMessage as it's not what the property is really representing. Given that the property handler there is updating the UI thread it makes sense that it should use Execute.OnUIThread.

The whole thing smells like a problem with the approach as we shouldn't need to guard every call on Execute.OnUIThread against this property.

However Execute.OnUIThread for WPF should be checking if its on the UI thread and not doing any dispatching if it is already. Not sure how that would deadlock.

[KasperSK]

@nigel-sampson I am closing this as it seems that the issue it represents have been solved at least partly. If you feel that there is any outstanding work in this PR please let us know.