Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Engine: protect calls to events delegate to avoid concurrency issues #202

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

knocte
Copy link
Contributor

@knocte knocte commented Jan 28, 2015

@knocte
Copy link
Contributor Author

knocte commented Feb 3, 2015

@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
Copy link
Owner

meebey commented Feb 3, 2015

", 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
Copy link
Owner

meebey commented Feb 3, 2015

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

@knocte
Copy link
Contributor Author

knocte commented Feb 3, 2015

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
Copy link
Owner

meebey commented Feb 3, 2015

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
Copy link
Owner

meebey commented Feb 3, 2015

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
Copy link
Contributor Author

knocte commented May 4, 2015

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants