Race Conditions in C# Eventing?
Karsten Schramm
in VB.Net I fire an event very simply:
RaiseEvent(MyEvent) 'Fire&Forget
The same in C# looks like
if(MyEvent != null) //check, if someone's listening
{
MyEvent(); //Fire
}
What happens in a multithreaded environment? The moment after I checked
MyEvent the last listener could retreat and I would have a
NullReferenceException nonetheless, wouldn't I?
Best regards,
Karsten Schramm
Mehdi
> The same in C# looks like
>
> if(MyEvent != null) //check, if someone's listening
> {
> MyEvent(); //Fire
> }
>
> What happens in a multithreaded environment? The moment after I checked
> MyEvent the last listener could retreat and I would have a
> NullReferenceException nonetheless, wouldn't I?
Yes. See Jon Skeet's article about thread safe events here:
<http://www.yoda.arachsys.com/csharp/threads/lockchoice.shtml>
Michael Nemtsev
RaiseEvent in VB.net is not thread-safe, you can see this in MSIL
thus use locking like
SyncLock Me
RaiseEvent(MyEvent)
End SyncLock
But, registration and unregistration is *always* thread-safe.
Now about C#.
The problem is that calls to register and unregister events are thread-safe
if they are called from outside the class with the event
KS> in VB.Net I fire an event very simply:
KS>
KS> RaiseEvent(MyEvent) 'Fire&Forget
KS>
KS> The same in C# looks like
KS>
KS> if(MyEvent != null) //check, if someone's listening
KS> {
KS> MyEvent(); //Fire
KS> }
KS> What happens in a multithreaded environment? The moment after I
KS> checked MyEvent the last listener could retreat and I would have a
KS> NullReferenceException nonetheless, wouldn't I?
KS>
KS> Best regards,
KS> Karsten Schramm
---
WBR,
Michael Nemtsev :: blog: http://spaces.msn.com/laflour
"At times one remains faithful to a cause only because its opponents do not
cease to be insipid." (c) Friedrich Nietzsche
![Jon Skeet [C# MVP]'s profile photo](http://lh3.googleusercontent.com/a-/ALV-UjUE-UkOE6oV07-Al6msToqDuW9IN9k9G_wNeBLWDr7OkQdq7K1C5zHX3w=s40-c)
Jon Skeet [C# MVP]
> RaiseEvent in VB.net is not thread-safe, you can see this in MSIL
> thus use locking like
>
> SyncLock Me
> RaiseEvent(MyEvent)
> End SyncLock
That's a really bad idea IMO - holding a lock while you raise an event
could be catastrophic, as the event handler could be doing something
like calling Control.Invoke, which could involve the UI thread waiting
to acquire the same lock.
> But, registration and unregistration is *always* thread-safe.
Well, it depends on exactly what you mean. It will always be atomic,
but you won't necessarily see the change from other threads.
--
Jon Skeet - <sk...@pobox.com>
http://www.pobox.com/~skeet Blog: http://www.msmvps.com/jon.skeet
If replying to the group, please do not mail me too
Nicholas Paldino [.NET/C# MVP]
In C#, the code that you have is actually a bad idea. The current
suggestion out of MS is to do something like this:
// Assign to a local variable.
EventHandler myEvent = MyEvent;
// Check for null.
if (myEvent != null)
{
// Fire.
myEvent();
}
This will handle the case of the delegate list being modified on another
thread (having it nulled out).
However, the CLR can actually optimize the local variable away (through
inlining), and directly access MyEvent still.
Because of this, to be TRULY safe, you want to pass the event through to
a method that is either virtual, or has the MethodImpl attribute attached to
it, passing the MethodImplOptions.NoInlining option. This way, the function
will not be inlined, which results in your parameter being preserved,
instead of being inlined to the original variable.
Hope this helps.
--
- Nicholas Paldino [.NET/C# MVP]
- m...@spam.guard.caspershouse.com
"Karsten Schramm" <NewsGroups-e...@yajirobi.de> wrote in message
news:1152284571.7...@75g2000cwc.googlegroups.com...
Michael Nemtsev
J> Michael Nemtsev <nem...@msn.com> wrote:
>> RaiseEvent in VB.net is not thread-safe, you can see this in MSIL
>> thus use locking like
>> SyncLock Me
>> RaiseEvent(MyEvent)
>> End SyncLock
J> That's a really bad idea IMO - holding a lock while you raise an
J> event could be catastrophic, as the event handler could be doing
J> something like calling Control.Invoke, which could involve the UI
J> thread waiting to acquire the same lock.
Yep, but we need to realize what are we doing.
Jon, I see the problem in RaiseEvent that MSIL doesn't guarantee that it
wont be interupted somewhere in the middle before Invoke call and in that
case, for example we unreg our event, return to Invoke and ta-da got NullException
>> But, registration and unregistration is *always* thread-safe.
J> Well, it depends on exactly what you mean. It will always be atomic,
J> but you won't necessarily see the change from other threads.
I meant that for VB MSIL always mark reg/unreg as syncronized (translating
call to add_xxx remove_xxx methods)
But for C# it works in some cases - only when these calls are in separate
class
Jon Skeet [C# MVP]
> In C#, the code that you have is actually a bad idea. The current
> suggestion out of MS is to do something like this:
>
> // Assign to a local variable.
> EventHandler myEvent = MyEvent;
>
> // Check for null.
> if (myEvent != null)
> {
> // Fire.
> myEvent();
> }
>
> This will handle the case of the delegate list being modified on another
> thread (having it nulled out).
It won't make sure that any changes made by a different thread are used
in the first place though.
> However, the CLR can actually optimize the local variable away (through
> inlining), and directly access MyEvent still.
I'm sure I've read something about this, but that it turned out not to
be true. I've a sneaking suspicion it was on some blog or other. I
found this:
http://blogs.msdn.com/grantri/archive/2004/09/07/226355.aspx
but I'm not sure it's the same thing.
> Because of this, to be TRULY safe, you want to pass the event through to
> a method that is either virtual, or has the MethodImpl attribute attached to
> it, passing the MethodImplOptions.NoInlining option. This way, the function
> will not be inlined, which results in your parameter being preserved,
> instead of being inlined to the original variable.
A better solution is to make it thread-safe, IMO.
See http://www.pobox.com/~skeet/csharp/threads/lockchoice.shtml for my
preferred way of doing things.
Jon Skeet [C# MVP]
> J> That's a really bad idea IMO - holding a lock while you raise an
> J> event could be catastrophic, as the event handler could be doing
> J> something like calling Control.Invoke, which could involve the UI
> J> thread waiting to acquire the same lock.
>
> Yep, but we need to realize what are we doing.
>
> Jon, I see the problem in RaiseEvent that MSIL doesn't guarantee that it
> wont be interupted somewhere in the middle before Invoke call and in that
> case, for example we unreg our event, return to Invoke and ta-da got NullException
So do it the C# instead of using RaiseEvent in the first place. It does
seem that RaiseEvent is pretty broken.
> >> But, registration and unregistration is *always* thread-safe.
> J> Well, it depends on exactly what you mean. It will always be atomic,
> J> but you won't necessarily see the change from other threads.
>
> I meant that for VB MSIL always mark reg/unreg as syncronized (translating
> call to add_xxx remove_xxx methods)
> But for C# it works in some cases - only when these calls are in separate
> class
The autogenerated add/remove calls are indeed synchronized, but unless
you *also* lock when accessing the delegate, you don't get the
equivalent of volatility. The changes will be effectively volatile, but
the read may not be - you could read a value which is "stale",
basically.
(There's also the problem that the autogenerated methods lock on
"this", which is generally a bad idea in the first place...)
Michael Nemtsev
J> Michael Nemtsev <nem...@msn.com> wrote:
>> J> That's a really bad idea IMO - holding a lock while you raise an
>> J> event could be catastrophic, as the event handler could be doing
>> J> something like calling Control.Invoke, which could involve the UI
>> J> thread waiting to acquire the same lock.
>>
>> Yep, but we need to realize what are we doing.
>>
>> Jon, I see the problem in RaiseEvent that MSIL doesn't guarantee that
>> it wont be interupted somewhere in the middle before Invoke call and
>> in that case, for example we unreg our event, return to Invoke and
>> ta-da got NullException
>>
J> So do it the C# instead of using RaiseEvent in the first place. It
J> does seem that RaiseEvent is pretty broken.
Exactly. It's what I'm talking about
>>> But, registration and unregistration is *always* thread-safe.
>>>>
>> J> Well, it depends on exactly what you mean. It will always be
>> atomic, J> but you won't necessarily see the change from other
>> threads.
>>
>> I meant that for VB MSIL always mark reg/unreg as syncronized
>> (translating call to add_xxx remove_xxx methods) But for C# it works
>> in some cases - only when these calls are in separate class
>>
J> The autogenerated add/remove calls are indeed synchronized, but
J> unless you *also* lock when accessing the delegate, you don't get the
J> equivalent of volatility. The changes will be effectively volatile,
J> but the read may not be - you could read a value which is "stale",
J> basically.
Don't quite understand about equivalent of volatility and why read may be
dirty in that case?
Could you explait in a bit?
It's more important to remember that we must *always" lock reg/unreg within
the class with event.
J> (There's also the problem that the autogenerated methods lock on
J> "this", which is generally a bad idea in the first place...)
J>
Nicholas Paldino [.NET/C# MVP]
> in the first place though.
Yes you are right, the code only will handle the case if the event is
null or not. It doesn't handle preserving the list itself. If you access
the event directly, there is a chance that in between the check for null,
and the raising of the event, you could get a null reference exception
because another thread has nulled out the list (removed the last event
listener).
>
>> However, the CLR can actually optimize the local variable away
>> (through
>> inlining), and directly access MyEvent still.
>
> I'm sure I've read something about this, but that it turned out not to
> be true. I've a sneaking suspicion it was on some blog or other. I
> found this:
> http://blogs.msdn.com/grantri/archive/2004/09/07/226355.aspx
> but I'm not sure it's the same thing.
Yes, this is a similar example. However, you can't place the volatile
keyword on an event, so the solution posted there won't work.
>> Because of this, to be TRULY safe, you want to pass the event through
>> to
>> a method that is either virtual, or has the MethodImpl attribute attached
>> to
>> it, passing the MethodImplOptions.NoInlining option. This way, the
>> function
>> will not be inlined, which results in your parameter being preserved,
>> instead of being inlined to the original variable.
>
> A better solution is to make it thread-safe, IMO.
>
> See http://www.pobox.com/~skeet/csharp/threads/lockchoice.shtml for my
> preferred way of doing things.
I don't think that this is a good way to do this. You have to end up
placing this code at the call site and the declaration of every single event
that you have. This quickly becomes a maintinence issue.
In using a method that will handle the firing of all of your events for
you (or at least the ones that follow the suggested pattern of two
parameters, object o and EventArgs e), and declaring that the method can't
be inlined, you can easily provide nearly the same level of protection
(neither your solution nor mine actually preserves the delegate chain, only
preventing it from being set to null in a multi-threaded environment), and
only have to modify the call site for actually raising the event.
With your way of doing it, though, you gain the advantage of
encapsulating the lock object that is used on the event. In delcaring the
event normally, you end up synchronizing on this, or the Type of the object.
I would say that it is a judgement call whether or not you want to maintain
that much code for that benefit.
--
- Nicholas Paldino [.NET/C# MVP]
- m...@spam.guard.caspershouse.com
>
Jon Skeet [C# MVP]
> > in the first place though.
>
> Yes you are right, the code only will handle the case if the event is
> null or not. It doesn't handle preserving the list itself. If you access
> the event directly, there is a chance that in between the check for null,
> and the raising of the event, you could get a null reference exception
> because another thread has nulled out the list (removed the last event
> listener).
No, that's not an issue because delegates are immutable. Nothing can
actually remove the event listener from the list - they would only
change the variable to hold a reference to a different delegate.
However, you could find that event handlers have been subscribed (or
removed) from other threads but that you never see them.
> > A better solution is to make it thread-safe, IMO.
> >
> > See http://www.pobox.com/~skeet/csharp/threads/lockchoice.shtml for my
> > preferred way of doing things.
>
> I don't think that this is a good way to do this. You have to end up
> placing this code at the call site and the declaration of every single event
> that you have. This quickly becomes a maintinence issue.
It's a lot of work, but that's what you get when you want to make a
class thread-safe. Of course, if you don't need to make it thread-safe
in the first place (if you can demand that all subscriptions and
unsubscriptions take place on the same thread that the event is raised
on) then there's less of an issue. Admittedly in a strict mood I'd
still want to wire the add/remove myself to avoid using an implicit
lock, but that's a different matter.
> In using a method that will handle the firing of all of your events for
> you (or at least the ones that follow the suggested pattern of two
> parameters, object o and EventArgs e), and declaring that the method can't
> be inlined, you can easily provide nearly the same level of protection
> (neither your solution nor mine actually preserves the delegate chain, only
> preventing it from being set to null in a multi-threaded environment), and
> only have to modify the call site for actually raising the event.
Mine is safe and correct by the spec rather than just by the current
implementation. Inlining and thread safety are only related by virtue
of the current implementation, IMO.
> With your way of doing it, though, you gain the advantage of
> encapsulating the lock object that is used on the event. In delcaring the
> event normally, you end up synchronizing on this, or the Type of the object.
> I would say that it is a judgement call whether or not you want to maintain
> that much code for that benefit.
Well, I'm kinda anal about thread safety...
Barry Kelly
wrote:
> In C#, the code that you have is actually a bad idea. The current
> suggestion out of MS is to do something like this:
>
> // Assign to a local variable.
> EventHandler myEvent = MyEvent;
>
> // Check for null.
> if (myEvent != null)
> {
> // Fire.
> myEvent();
> }
>
> This will handle the case of the delegate list being modified on another
> thread (having it nulled out).
I think this pattern brings back the same old Java "synchronized"
problem of designing *every* class as if it was OK to be used
simultaneously on lots of different threads. It was wrong for Java (it
wasn't included in C#) and I think it's wrong to use *all* the time for
raising events too. In carefully selected classes, it may be justified.
What's more, it will break if the compiler implements copy propagation.
To get it to work reliably, you either need to use a backing variable
(i.e. "add" and "remove" which manipulate the backing variable) which is
marked volatile, or lock on some object while reading the event:
---8<---
EventHandler myEvent;
lock (_someObject)
myEvent = MyEvent;
// ...
--->8---
The compiler can't and won't use copy propagation here, since it's aware
of threads and locks. (It's also one of the reasons that the C++
standard's agnosticism on threads is wrong-headed, but that's another
different story...)
As Jon links to, Grant (working on the x64 compiler) ran into trouble
implementing copy propagation, since it broke BCL source code
assumptions.
-- Barry
Barry Kelly
wrote:
> > See http://www.pobox.com/~skeet/csharp/threads/lockchoice.shtml for my
> > preferred way of doing things.
>
> I don't think that this is a good way to do this. You have to end up
> placing this code at the call site and the declaration of every single event
> that you have. This quickly becomes a maintinence issue.
You don't have a lot of choice *if* you want to make the class
thread-safe.
A third way (other than volatile or locking) is to use a memory read
barrier between the read and the subsequent uses:
---8<---
EventHandler myEvent = MyEvent;
Thread.MemoryBarrier();
if (myEvent != null)
myEvent();
--->8---
Nicholas Paldino [.NET/C# MVP]
See inline:
> Mine is safe and correct by the spec rather than just by the current
> implementation. Inlining and thread safety are only related by virtue
> of the current implementation, IMO.
Using the MethodImpl attribute is spec-safe as well. In Partition II,
Section 15.4.3, the definition for the flags for ImplAttr (which is mapped
from the MethodImplOptions enumeration passed to MethodImplAttribute)
defines noinlining as:
The runtime shall not expand the method inline.
Furthermore, in the same partition, Section 15.4.3.3 ("Implementation
information"), it goes on to define the "noinlining" flag:
noinlining specifies that the body of this method should not be included
into the code of any caller methods, by a CIL-to-native-code compiler; it
shall be kept as a separate routine.
Continuing in the "synchronized" section:
noinlining specifies that the runtime shall not inline this method. Inlining
refers to the process of replacing the call instruction with the body of the
called method. This can be done by the runtime for optimization purposes.
Basically, the CLI spec indicates that a valid implementation of the CLR
will honor the attribute, and it is not an implementation detail. That
being said, it will honor the copy of the parameter to a method that is
inlined, and produce the same effect with less code.
>> With your way of doing it, though, you gain the advantage of
>> encapsulating the lock object that is used on the event. In delcaring
>> the
>> event normally, you end up synchronizing on this, or the Type of the
>> object.
>> I would say that it is a judgement call whether or not you want to
>> maintain
>> that much code for that benefit.
>
> Well, I'm kinda anal about thread safety...
Yeah, well, no need to remind me =P
--
- Nicholas Paldino [.NET/C# MVP]
- m...@spam.guard.caspershouse.com
>
Jon Skeet [C# MVP]
> > implementation. Inlining and thread safety are only related by virtue
> > of the current implementation, IMO.
>
> Using the MethodImpl attribute is spec-safe as well. In Partition II,
> Section 15.4.3, the definition for the flags for ImplAttr (which is mapped
> from the MethodImplOptions enumeration passed to MethodImplAttribute)
> defines noinlining as:
>
> The runtime shall not expand the method inline.
<snip>
> Basically, the CLI spec indicates that a valid implementation of the CLR
> will honor the attribute, and it is not an implementation detail. That
> being said, it will honor the copy of the parameter to a method that is
> inlined, and produce the same effect with less code.
It will honour the method not being inlined, but that doesn't
necessarily mean that there will be memory barriers. There's nothing in
the memory model which specifies that there must be a memory barrier at
method call boundaries, as far as I'm aware.