ICommand.CanExecuteChanged should handle the execution on the correct thread

[trodent83]

NET Core Version: 5.0.201
Windows version: Windows 10 Entrprise 190
Does the bug reproduce also in WPF for .NET Framework 4.8?: Yes
**Problem description:
Currently the registration to the ICommand.CanExecuteChanged event doesn't consider that the triggering of the command state change can happen from a background thread and throws an InvalidOperationException.

Considering that the registration to the command even happens in the control it should ensure the execution of the refresh will happen on the thread of the control.

**Actual behavior: Exception is thrown when the CanExecuteChanged is called from not the Owner thread.

**Expected behavior: The state refresh should be executed without any problems

[miloush]

I think you should provide some repro steps and a proposal how you would like to see this fixed. ICommand is an interface, so you cannot enforce a calling thread for the implementer, it does not come with "owner" thread by default and I am not even sure I would want to enforce it in cases where the consumer does not care about threading.

[trodent83]

@miloush
I think you missunderstood. Not the implementer of the ICommand should make sure that we are on the correct thread but the registration on the event of ICommand. Exactly for the reason that the implementer of the ICommand should not have to care about on which thread we are on.

Make something like this in the command execute body (easiest way to reproduce):
System.Threading.Tasks.Task.Factory.StartNew(() => { this.CanExecuteChanged?.Invoke(this, new EventArgs()); });

[miloush]

Oh right, so if I understand you correctly, you are proposing to change every subscriber that cares about threading to do the job and switch to the thread if called from different one?

The problem is that in .NET, the default behavior is the opposite - everything non-static is thread unsafe. You are breaking the rule by invoking an event on a different thread than it was subscribed to. What you are proposing is against all current .NET practice and guidelines of not providing thread safety for instance members by default, see e.g. https://docs.microsoft.com/en-us/dotnet/standard/threading/managed-threading-best-practices#recommendations-for-class-libraries:

• Avoid the need for synchronization, if possible. This is especially true for heavily used code. For example, an algorithm might be adjusted to tolerate a race condition rather than eliminate it. Unnecessary synchronization decreases performance and creates the possibility of deadlocks and race conditions.
• Make static data (Shared in Visual Basic) thread safe by default.
• Do not make instance data thread safe by default. Adding locks to create thread-safe code decreases performance, increases lock contention, and creates the possibility for deadlocks to occur. In common application models, only one thread at a time executes user code, which minimizes the need for thread safety. For this reason, the .NET class libraries are not thread safe by default.

Unless I misunderstood your suggestion, I am afraid that this cannot and should not be changed.

[trodent83]

@miloush
Look the rules you have writen down do not apply in the current case as the locking mechanism is the only way to ensure that the refresh mechanism work. Avoiding this issue in the framework internally only means that the users of the framework will have to deal with it which is more problematic an their end.
Beside I would say that basicly the Control is the one who is bracking the rule above, as all instance methods are not thread safe, ergo if you are registering on a public event you cannot assume the calling thread, which the control does. Ergo you are assuming that the command does everything in a thread safe manner which according to .Net rules is wrong.

Also I would say that a locking mechanism is not required here, as a FireAndForget mechanism which simply pushes a refresh request in the message pump is more than enough, so no possibility of deadlock would occure.

[miloush]

Control is the one who is bracking the rule above, as all instance methods are not thread safe

The rule is "all instance methods are not thread safe", meaning you cannot call them from multiple threads, so Control is following the rule. You are asking to change thousands of methods and properties, for the sake of a rare scenario, and introducing hard to debug issues. Fire and forget would be changing the behavior from synchronous execution to asynchronous, which would be a breaking change. I don't see how it prevents deadlocks.

If someone introduces a new thread to an application, they should also be the one who deal with the consequences.

[trodent83]

@miloush
The control is injecting it self in the execution chain, and not explicitly being called. Because it is doing the injection it is basicly calling himself ergo he is the responsible party to ensure that the call happens from the correct thread.
Also not threadsafe does not mean that it cannot be called from multiple threads but that it cannot be called at the same time.

Where are the thousend propeties are comming from I have no idea, also I do not understand the bracking change part. If you are in the correct thread you execute the code, if not you enque it? What changes? That it doesn't crash where it used to? Please elaborate a little.

It eliminates deadlock because no locking isn't really needed as during the refresh you are on that thread where all ui triggers are happening and becuase you have a work queue.

If someone introduces a new thread to an application, they should also be the one who deal with the consequences.

Let me ask why does the Binding handle the syncronizations? Why was for the UI syncronization in .Net 4.5 the BindingOperations.EnableCollectionSynchronization introduced?