Is this a bug with events and add/remove?

[David Levine]

I ran into a problem this morning with event accessor methods that appears
to be a bug in C# and I am wondering if this a known issue.

public event SomeDelegateSignature fooEvent;
public event SomeDelegateSignature foo2Event;
{
add
{ // some code here }
remove
{ // some code here }
}

public void DoSomething ()
{
fooEvent.GetInvocationList(); // this line compiles
foo2Event.GetInvocationList(); // this line does not compile
}

The fooEvent compiles just fine but fooEvent2 generates compiler errors
error CS0079: The event 'foo2Event' can only appear on the left hand side of
+= or -=

The only difference between the two events is that with foo2 I manually
implement the add/remove accessors. I believe this used to work v1.1. Is
this no longer supported under 2.0?

 

[Chris Jobson]

I ran into a problem this morning with event accessor methods that appears
to be a bug in C# and I am wondering if this a known issue.

public event SomeDelegateSignature fooEvent;
public event SomeDelegateSignature foo2Event;

I assume the ";" at the end of the previous line is just a typo?

{
add
{ // some code here }
remove
{ // some code here }
}

public void DoSomething ()
{
fooEvent.GetInvocationList(); // this line compiles
foo2Event.GetInvocationList(); // this line does not compile
}

The fooEvent compiles just fine but fooEvent2 generates compiler errors
error CS0079: The event 'foo2Event' can only appear on the left hand side
of += or -=

The only difference between the two events is that with foo2 I manually
implement the add/remove accessors. I believe this used to work v1.1. Is
this no longer supported under 2.0?

I don't know about v1.1, but surely the point about the add/remove accessors
is that you are managing the invocation list yourself and therefore there's
no way that GetInvocationList could actually return anything useful.
Therefore it seems sensible not to allow it to be called.

Chris Jobson

 

[Jon Skeet [C# MVP]]

I ran into a problem this morning with event accessor methods that appears
to be a bug in C# and I am wondering if this a known issue.

It's not a bug in C#. It's a fault in your understanding of event
declaration.

public event SomeDelegateSignature fooEvent;

This is a "field-like" event declaration. It effectively declares both
a public event and a private delegate field to back the event.

public event SomeDelegateSignature foo2Event;
{
add
{ // some code here }
remove
{ // some code here }
}

This *only* declares the event.

public void DoSomething ()
{
fooEvent.GetInvocationList(); // this line compiles

That accesses the delegate - because GetInvocationList() is a delegate
method.

foo2Event.GetInvocationList(); // this line does not compile

Indeed, because you can't call GetInvocationList() on an actual event.
You can only call add/remove, basically (from C# at least).

}

The fooEvent compiles just fine but fooEvent2 generates compiler errors
error CS0079: The event 'foo2Event' can only appear on the left hand side of
+= or -=

The only difference between the two events is that with foo2 I manually
implement the add/remove accessors. I believe this used to work v1.1. Is
this no longer supported under 2.0?

No, it wouldn't have worked in 1.1 either.

 

[David Levine]

It's not a bug in C#. It's a fault in your understanding of event
declaration.

That's now all too obvious ...and that's why I put a question mark in the
subject line. I never really took a close look at events.before so long as
it did everything I wanted it to do.

This is a "field-like" event declaration. It effectively declares both
a public event and a private delegate field to back the event.



This *only* declares the event.

Is there a syntax for doing both? Both declare the field delegate for the
event and the public event with add/remove accessors? Or am I force to do
something like save the accessor method's value into an explicitly declared
private delegate field, like this...

private SomeDelegateSignature _theDelegate;
public event SomeDelegateSignature foo2Event;
{
add
{ // other stuff here
_theDelegate = value;
}
}

TIA

 

[David Levine]

public void DoSomething ()

I don't know about v1.1, but surely the point about the add/remove
accessors is that you are managing the invocation list yourself and
therefore there's no way that GetInvocationList could actually return
anything useful. Therefore it seems sensible not to allow it to be called.

That's not true at all. .

 

[Jon Skeet [C# MVP]]

That's now all too obvious ...and that's why I put a question mark in the
subject line. I never really took a close look at events.before so long as
it did everything I wanted it to do.

Sure. I meant to apologise for being so brusque, too - I didn't mean to
be rude. I suspect averaging about 3 hours sleep a night for the last
month has lowered my politeness level

Is there a syntax for doing both? Both declare the field delegate for the
event and the public event with add/remove accessors? Or am I force to do
something like save the accessor method's value into an explicitly declared
private delegate field, like this...

private SomeDelegateSignature _theDelegate;
public event SomeDelegateSignature foo2Event;
{
add
{ // other stuff here
_theDelegate = value;
}
}

The whole point of *having* the add/remove accessors is to customise
the behaviour, so you don't (and can't) get any "for free". Think of it
as being like the difference between a public field and a property.
(It's not quite the same, but the *feeling* of it is.)

Note that you don't want _theDelegate = value; as your add though - you
want _theDelegate += value;

You may also want to think about thread safety (depending on your
situation). See
http://www.pobox.com/~skeet/csharp/threads/lockchoice.shtml for my
preferred pattern.

 

[Jon Skeet [C# MVP]]

That's not true at all. .

I think it is. An event doesn't logically have an invocation list at
all - it only has a pair (or trio in theory) of methods. It's like
asking a property what variable is backing it - just because it will
*often* be backed by a variable doesn't mean it *has* to be.

 

[David Levine]

This *only* declares the event.

The whole point of *having* the add/remove accessors is to customise
the behaviour, so you don't (and can't) get any "for free". Think of it
as being like the difference between a public field and a property.

It is a bit like that but there are a lot of differences too. It would have
been "nice" had they provided a way of exposing the functionality so that
you could do preprocessing within the add/remove accessor and then
explicitly invoke the default behavior from within it - this would make it
unncessary to provide your own backing field for the delegate, and all other
behaviors of the event would remain the same.

(It's not quite the same, but the *feeling* of it is.)

Note that you don't want _theDelegate = value; as your add though - you
want _theDelegate += value;

I wrote that off rather quickly. I'd use either the +=/-= or explicitly call
Delegate.Combine/Remove.

You may also want to think about thread safety (depending on your
situation). See
http://www.pobox.com/~skeet/csharp/threads/lockchoice.shtml for my
preferred pattern.

Thianks, I'm aware of the thread safety issues. I'm overriding the
add/remove accessors to be able to capture the syncrhonization context of
the suibscriber so that the event can be delivered within the same context.
I may expand that to include the execution context as well.

 

[David Levine]

I think it is. An event doesn't logically have an invocation list at
all - it only has a pair (or trio in theory) of methods. It's like
asking a property what variable is backing it - just because it will
*often* be backed by a variable doesn't mean it *has* to be.

--

I was thinking of the delegate that supplies the backing for the event. The
invocation list will still likely be stored within the delegate, not in some
other list. I suppose one could use a private field declared as
List<Delegate> and manually manage it but it seems like that would be more
work since the behavior is already built into the Delegate class.

 

[Jon Skeet [C# MVP]]

I was thinking of the delegate that supplies the backing for the event.

Who says there is such a thing? An event is just a pair (or trio) of
methods - there's nothing to say it *has* to be stored in a delegate at
all, or even that delegates are involved in the first place, other than
as the parameters for the add/remove.

The invocation list will still likely be stored within the delegate, not in some
other list. I suppose one could use a private field declared as
List<Delegate> and manually manage it but it seems like that would be more
work since the behavior is already built into the Delegate class.

And that's the key - it's part of the Delegate class, and delegates are
somewhat independent of events, conceptually.

 

[Jon Skeet [C# MVP]]

It is a bit like that but there are a lot of differences too. It would have
been "nice" had they provided a way of exposing the functionality so that
you could do preprocessing within the add/remove accessor and then
explicitly invoke the default behavior from within it - this would make it
unncessary to provide your own backing field for the delegate, and all other
behaviors of the event would remain the same.

There's very little involved in providing the backing delegate though,
just as there isn't much involved in providing the backing variable for
a property.

I wrote that off rather quickly. I'd use either the +=/-= or explicitly call
Delegate.Combine/Remove.

In that case it sounds like you don't want an event at all - you're
certainly breaking the expected semantics of it. If I do:

foo.SomeEvent += someDelegate;
foo.SomeEvent += someOtherDelegate;

I don't expect the second line to make the first effectively ignored.

The point is that you don't need to make the delegate variable public,
which you would have to if you wanted to expose combination/removal -
unless you had your own methods AddDelegate and RemoveDelegate, which
is effectively what events are anyway.

Thianks, I'm aware of the thread safety issues. I'm overriding the
add/remove accessors to be able to capture the syncrhonization context of
the suibscriber so that the event can be delivered within the same context.
I may expand that to include the execution context as well.

It sounds like you don't really want events at all, certainly not if
you're planning on simply *replacing* a delegate each time += is being
used.

 

[David Levine]

Who says there is such a thing? An event is just a pair (or trio) of
methods - there's nothing to say it *has* to be stored in a delegate at
all, or even that delegates are involved in the first place, other than
as the parameters for the add/remove.

Sure, but my st impulse was to use a delegate to provide the backing. Now
that I've had a day to think about it and try different techniques I've
found much better ways to accomplish my purpose.

And that's the key - it's part of the Delegate class, and delegates are
somewhat independent of events, conceptually.

Ater playing with it I've decide to store the delegates in a Dictionary and
map a hash code to the delegate and its associated context; as you said, the
event itself is independent of the delegate. I never had to delve into it
before so I never had to make the explicit distinction between the event and
a delegate. It's clear now, thanks.

 

[David Levine]

There's very little involved in providing the backing delegate though,
just as there isn't much involved in providing the backing variable for
a property.
agreed.


In that case it sounds like you don't want an event at all - you're
certainly breaking the expected semantics of it. If I do:

foo.SomeEvent += someDelegate;
foo.SomeEvent += someOtherDelegate;

I don't expect the second line to make the first effectively ignored.

Hmm, I guess I don't see how I would break the semantics since the form I
suggested would provide the same external behavior; both subscribers to the
event would get called.

After working on the prototype for a bit I've decided against using a
delegate as the backing field anyway; a Dictionary provides better
functionality for my purposes.

The point is that you don't need to make the delegate variable public,
which you would have to if you wanted to expose combination/removal -
unless you had your own methods AddDelegate and RemoveDelegate, which
is effectively what events are anyway.

I did not mean to imply that the delegate (or backing field) would be
public, only the event. The backing field is an implementation detail that I
would not want exposed.

It sounds like you don't really want events at all, certainly not if
you're planning on simply *replacing* a delegate each time += is being
used.

Events provide the semantics I want. I want existing subscribers to be able
to get this functionality without needing to be recompiled or changed, nd I
want to suport multiple subscribers, and I want the semantics to be familiar
and easy for developers to use. My apologees for not describing it clearly.

My goal is to capture the synchronization context of each subscriber so that
each delegate is invoked in the correct sync context for that subscriber.
For example, given a number of different subscribers of the event, each
doing something like this...

SomeMethod()
{ // running on thread 1, perhaps from within a WinForm
foo.SomeEvent += someDelegate;
}
SomeOtherMethod()
{ // running on thread 2, perhaps from within a different WinForm
foo.SomeEvent += someDelegate;
foo.SomeEvent += someOtherDelegate;
}

Regardless of the thread these methods are running on their synchronization
context should be captured at the instant of subscription, and that context
should be used to invoke the delegate when the event is fired. It also needs
to handle the case where the same delegate instance subscribes to the same
event multiple times but in different sync contexts.

I haven't looked at capturing the execution context at this time (this
encapsulates things like security, localization, call, transaction, as well
as synchronization) and using that for each subscriber, mainly because I
don't have a need for that at this time, but that could also be done.

The end result is to eliminate the need for the client to need to
reestablish its context when the event is fired. For example, this
eliminates the need for the callback code to do something like this...

void Callback(object state)
{
if ( someButton.InvokeRequired ) { someButton.Invoke( // yada yada yada,
marshal to correct thread ) }
}

The marshalling is already done for that callback, and for all callbacks
regardless of the sync context. This moves the burden of invoking callbacks
on the correct thread from the client (who may forget, not know this is
needed, or not know how) to the implementor of the library that exposes the
event. For example, I have a network interface layer running on a background
thread that delivers network event notifications to winforms using this
technique.

I've got a working prototype which appears to provide this. It's not in a
form I consider final and it currently is limited to a single delegate
signature. If I have the time I want to write it using DynamicMethods to
spin up a class that will work with any delegate signature, but that's for
another day.

On the other hand, perhaps you are aware of an implementation that already
provides this functionality?

Thanks
Dave

 

[Jon Skeet [C# MVP]]

Hmm, I guess I don't see how I would break the semantics since the form I
suggested would provide the same external behavior; both subscribers to the
event would get called.

Not with the code you showed. Here's an example:

using System;

class Foo
{
public delegate void Bar();

Bar _theDelegate;

public event Bar MyEvent
{
add
{
_theDelegate = value;
}
remove
{
// You didn't show the code you would
// use here
}
}

public void RaiseEvent()
{
_theDelegate();
}
}

class Test
{
static void Main()
{
Foo f = new Foo();
f.MyEvent += delegate { Console.WriteLine ("First"); };
f.MyEvent += delegate { Console.WriteLine ("Second"); };

f.RaiseEvent();
}
}

The result is just "Second".

After working on the prototype for a bit I've decided against using a
delegate as the backing field anyway; a Dictionary provides better
functionality for my purposes.

I suspect a List would actually give closer semantics to the "normal"
event semantics in terms of what happens if you add the same delegate
twice and remove it.

I did not mean to imply that the delegate (or backing field) would be
public, only the event. The backing field is an implementation detail that I
would not want exposed.

Good - in that case I hope you see why being able to call
GetInvocationList() on an event doesn't make sense (and how it might be
a pain to implement in your case anyway).

Events provide the semantics I want. I want existing subscribers to be able
to get this functionality without needing to be recompiled or changed, nd I
want to suport multiple subscribers, and I want the semantics to be familiar
and easy for developers to use. My apologees for not describing it clearly.

Right. That all makes sense - but the straight assignment you used in
the sample code doesn't help you, and indeed it wouldn't help you for
the language to automatically generate a backing delegate.

I've got a working prototype which appears to provide this. It's not in a
form I consider final and it currently is limited to a single delegate
signature. If I have the time I want to write it using DynamicMethods to
spin up a class that will work with any delegate signature, but that's for
another day.

On the other hand, perhaps you are aware of an implementation that already
provides this functionality?

I'm not aware of any I'm afraid, no.

 

[David Levine]

Not with the code you showed. Here's an example:

er, I already said that the original code I showed was not what I intended
to use. I was originally unclear how events were related to delegates but
that is no longer an issue.

I agree that the code:
add
{
_theDelegate = value;
}

would overwrite the previous value...this is not the code I would use. As I
stated elsewhere, if I simply wanted to duplicate the existing semantics I
could either use
_theDelegate += value;
or
_theDelegate = Delegate.Combine(_theDelegate,value);

Neither of these is what I wound up using.

I suspect a List would actually give closer semantics to the "normal"
event semantics in terms of what happens if you add the same delegate
twice and remove it.

A List would work but would take more effort to manage.
For my purposes Dictionary provides the same external semantics and allows
me much greater control over the data that is associated with the delegate.
I can provide the same external semantics as a "normal" event.; that part is
easy.

Adding a delegate is easy; it captures the current synchronization context
and thread and saves that into a Dictionary entry, keyed by the delegate's
hashcode, wrapped in a class that contains all the context information,
which is then added to a list for that delegate instance. It's signature is:
Dictionary<int, List<EventContext>>
So long as each delegate instance yields a different hashcode it should work
without getting confused about delegate instances; can you think of a case
where this would not be true?

I originally used the delegate instance itself as the key but changed it to
use the hashcode so I could store a WeakReference to the delegate rather
then the delegate itself. This would allow subscribers that "forgot" to
unsubscribe to the event to be garbage collected (i.e. the delegate instance
itself would not keep the subscriber's instance alive). I currently have the
WeakReference disabled as it seemed to be garbage collecting instances that
I thought should not have been; that is something I intend to research
later.

As to removing it, that is more difficult to implement then simply adding a
delegate. Consider the case where the same Delegate instance is added
multiple times, but in several different contexts and threads, perhaps more
then once from each. This can occur in any number of combinations, with one
or more Delegate instances. When a delegate instance is removed there is no
guarantee that the call to Remove will occur in the same synchronization
context as the call to add the delegate, or from the same thread. The Remove
algorithm will first attempt to precisely match the current synchronization
context and threadID to the synchronization context/threadID used to add the
delegate, and if that cannot be located it attempts to find the best match
based on the Thread ID alone. If that fails then it defaults to removing the
first instance in the Dictionary's List that matches the Delegate instance
(which is similar to "normal" event behavior).

Good - in that case I hope you see why being able to call
GetInvocationList() on an event doesn't make sense (and how it might be
a pain to implement in your case anyway).

It was quite easy to implement, and it makes a lot of sense to do it because
the purpose was to provide synchronization semantics for each individual
subscriber. This requires handling each invocation differently so it was a
requirement that I either implement GetInvocationList or provide a semantic
equivalent. I implemented something quite similar, one that works with the
Dictionary.

Right. That all makes sense - but the straight assignment you used in
the sample code doesn't help you, and indeed it wouldn't help you for
the language to automatically generate a backing delegate.

We both agree that the straight assignment would not work without changing
the semantics; it's a minor point.

I think the language could have implemented events differently to make it
easier to work with. For example, they should have eliminated the need to
test the event itself for null before firing the event. I also don't like
that the legality of invoking methods (e.g. GetInvocationList) on events
changes depending on whether or not the Add/Remove accessors are
implemented - I did not expect that. They could have made these methods
virtual (or used some other mechanism) and allowed implementors to override
them.

 

[Barry Kelly]

I originally used the delegate instance itself as the key but changed it to
use the hashcode so I could store a WeakReference to the delegate rather
then the delegate itself. This would allow subscribers that "forgot" to
unsubscribe to the event to be garbage collected (i.e. the delegate instance
itself would not keep the subscriber's instance alive). I currently have the
WeakReference disabled as it seemed to be garbage collecting instances that
I thought should not have been; that is something I intend to research
later.

This won't work. The trouble is that you end up with a weak reference to
the wrong thing. You end up with this object graph:

dict -> weakref -> delegate -> object <- other_objects

That is, the only object referring to the delegate is the thing you wrap
the weak reference around. What you actually want is something more like
this:

dict -> delegate -> weakref -> object <- other_objects

I.e. you need a "weak delegate". That's something that the .NET
framework doesn't currently have, and I think it's something that is
needed. To implement it currently, you either have to use reflection, or
you've got to play with DynamicMethod and create a wrapper to avoid the
overhead of reflection invoke.

-- Barry

 

[David Levine]

This won't work. The trouble is that you end up with a weak reference to
the wrong thing. You end up with this object graph:

dict -> weakref -> delegate -> object <- other_objects

That is, the only object referring to the delegate is the thing you wrap
the weak reference around. What you actually want is something more like
this:

dict -> delegate -> weakref -> object <- other_objects

I.e. you need a "weak delegate". That's something that the .NET
framework doesn't currently have, and I think it's something that is
needed. To implement it currently, you either have to use reflection, or
you've got to play with DynamicMethod and create a wrapper to avoid the
overhead of reflection invoke.

That makes sense; thanks for pointing that out. I may play around with
DynamicMethods to see if I can alter this behavior.

 

[Jon Skeet [C# MVP]]

Adding a delegate is easy; it captures the current synchronization context
and thread and saves that into a Dictionary entry, keyed by the delegate's
hashcode, wrapped in a class that contains all the context information,
which is then added to a list for that delegate instance. It's signature is:
Dictionary<int, List<EventContext>>
So long as each delegate instance yields a different hashcode it should work
without getting confused about delegate instances; can you think of a case
where this would not be true?

Heck yes - hashcodes aren't designed to be unique. You'd have to be
unlucky to get a hit, but do you really want to rely on luck?

I originally used the delegate instance itself as the key but changed it to
use the hashcode so I could store a WeakReference to the delegate rather
then the delegate itself. This would allow subscribers that "forgot" to
unsubscribe to the event to be garbage collected (i.e. the delegate instance
itself would not keep the subscriber's instance alive). I currently have the
WeakReference disabled as it seemed to be garbage collecting instances that
I thought should not have been; that is something I intend to research
later.

If you're including the delegate itself somewhere, you've got the same
problem anyway. I don't have the time to get into this right this
minute, but I see you're talking to Barry about it anyway, so I'll
leave that at the moment.

I think the language could have implemented events differently to make it
easier to work with. For example, they should have eliminated the need to
test the event itself for null before firing the event. I also don't like
that the legality of invoking methods (e.g. GetInvocationList) on events
changes depending on whether or not the Add/Remove accessors are
implemented - I did not expect that. They could have made these methods
virtual (or used some other mechanism) and allowed implementors to override
them.

You're mixing terminology again - you've used the word "event" there
for both events and delegates. For instance, you can't test an event
for nullity, because an event is just a pair of methods. You can test
whether a delegate variable's value is null, however.

Similarly, you can never invoke methods on events - it's just that if
you use a field-like event declaration, that's declaring both an event
and a delegate variable.

 

[David Levine]

David Levine <[email protected]> wrote:

Heck yes - hashcodes aren't designed to be unique. You'd have to be
unlucky to get a hit, but do you really want to rely on luck?

I don't rely on that alone; it's useful to get the code pointed to a
particular list bucket, and then it retrieves the original delegate instance
and ensures they are the same.

If you're including the delegate itself somewhere, you've got the same
problem anyway. I don't have the time to get into this right this
minute, but I see you're talking to Barry about it anyway, so I'll
leave that at the moment.

Of course I didn't include the delegate itself - that's the whole point of
WeakReferences!!!!! The WeakReference wraps the delegate.

Barry very clearly addressed the reason why the delegate was getting
collected when I felt it shouldn't. There is no good workaround at the
moment; delegates need to be redesigned.

You're mixing terminology again - you've used the word "event" there
for both events and delegates. For instance, you can't test an event
for nullity, because an event is just a pair of methods. You can test
whether a delegate variable's value is null, however.

No, I'm aware of the distinction. Regardless of the terminology, when an
event is declared as:
public event DelegateSignature SomeEvent;
then it is possible to write

if ( SomeEvent != null )
{
}

or
SomeEvent.GetInvocationList();
or
SomeEvent(this,args);

etc.

Internally it may be wiring the calls to the delegate that backs the event
but from the perspective of the code using the event it appears that the
event itself is being tested and used this way.

You can argue that it isn't the event itself that is tested for null or that
is invoked, that it's really the delegate, but it sure looks that way when
writing the code.

And the implementation could have initialized the delegate backing the event
to an empty list, so that if there were no subscribers the code could still
invoke the event as...
SomeEvent(args);

without it throwing a NullReferenceException.

 

[Barry Kelly]

Regardless of the terminology, when an
event is declared as:
public event DelegateSignature SomeEvent;
then it is possible to write

if ( SomeEvent != null )
{
}

or
SomeEvent.GetInvocationList();
or
SomeEvent(this,args);

etc.

Internally it may be wiring the calls to the delegate that backs the event
but from the perspective of the code using the event it appears that the
event itself is being tested and used this way.

You can argue that it isn't the event itself that is tested for null or that
is invoked, that it's really the delegate, but it sure looks that way when
writing the code.

I think this is slightly unfortunate, but the desire behind it is
reasonable - it makes the common-place job of adding event declarations
to classes less onerous.

And the implementation could have initialized the delegate backing the event
to an empty list,

I wish to make clear for everybody (as you already know) that the empty
list for a delegate is in fact a null reference. Delegates are
effectively nodes in a single-linked list - so there can be no "empty
list". This implementation approach also reduces the number of objects
for unsubscribed events.

so that if there were no subscribers the code could still
invoke the event as...
SomeEvent(args);

-- Barry