Engine: protect calls to events delegate to avoid concurrency issues

[knocte]

This good practice is explained in Gendarme's rule documentation: http://www.mono-project.com/docs/tools+libraries/tools/gendarme/rules/concurrency/#protectcalltoeventdelegatesrule

[knocte]

@meebey: from our convo in IRC I think I got that you thought that the assignment to a handler would serve only the purpose of avoiding a race condition if the event gets assigned to null again, which doesn't happen in smuxi code. However it's not only to prevent that situation, it would also prevent that a handler that has desubscribed already, gets the call anyway.

(Taken from http://codeblog.jonskeet.uk/2015/01/30/clean-event-handlers-invocation-with-c-6/ , which BTW shows that C#6.0 will help the code be more convoluted when you're ready to accept this version of the language in smuxi; but in the meantime, I think this patch is desirable.)

[meebey]

", it would also prevent that a handler that has desubscribed already, gets the call anyway." I dont agree with that statement, the local copy does not solve that issue. http://stackoverflow.com/a/23737078 I had a different article that explains this unfixable race, but this seems to cover it also. You only move the race window, you don't fix the race.

[meebey]

This could be a compromise of readable code vs no NRE race: http://stackoverflow.com/a/786473

[knocte]

I dont agree with that statement, the local copy does not solve that issue

Oh you're right, my bad.

You only move the race window, you don't fix the race.

True, but I find the non-NRE race to be less likely to happen.

This could be a compromise of readable code vs no NRE race:

Yes but you would need that extension method for each delegate type (this is also discussed in JonSkeet's blogpost).

[meebey]

http://blogs.msdn.com/b/ericlippert/archive/2009/04/29/events-and-races.aspx is also a very good read about this topic and the issues of the different fixes and non-fixes

[meebey]

Quote from http://blogs.msdn.com/b/ericlippert/archive/2009/04/29/events-and-races.aspx:

Essentially there are two problems here that are being conflated. The two problems are:

1) The event handler delegate can be null at any time.
2) A “stale” handler can be invoked even after it has been removed."

I agree that we fix 1) but not at the cost of making the code ugly. The default empty event handler seems to be the only pattern in C# that fixes 1) but does not need to refactor any event raiser code. Do you agree with that @knocte ?

[knocte]

Well, in the end, the best way to fix this will be adopting C#6.0's '?' operator for events:

OnChanged?.Invoke(this, args);

More info here: http://channel9.msdn.com/Series/ConnectOn-Demand/211