C# Language Specification - Delegates

[Guest]

First... let's deal with Delegates. Comments welcome.

1) Invoking a NULL delegate is annoying.
** (It should just do nothing rather than crash.)

2) It's too easy to accidently attach multiple redundant handlers onto
delegates.

3) A delegate might point to handlers on objects that are out of scope or
been disposed, causing those objects to crash.
** (Handlers should be removed automatically when the object that defines
them leaves scope).

4) ?
5) ?

There is sort of a potential for "delegate leak" where objects attach
handlers indefinitely while the handler functions get called an increasing
number of times per event...


Unfortunately, the way to prevent the aforementioned problems is not
convenient...

So, for protective coding... must we truly declare the laws of proper
delegate usage as follows? (I hope we can resolve a more convenient way).

////////////////////////////////////////////////////////
///// ALL CLASSES THAT ATTACH TO THE EVENT CHAIN MUST...

// declare member variable of this
System.EventHandler _hEventHandler;

// during construction of this:
_hEventHandler = new System.EventHandler(whatever);

// during initialization of this:
_obj._SomeEvent += _hEventHandler;

// during disposal of this: (all users must call Dispose)
_obj._SomeEvent -= _hEventHandler;

////////////////////////////////////////////////////////


////////////////////////////////////////////////////////
///// ALL CLASSES SERVING AN EVENT MUST...
////////////////////////////////////////////////////////
public event System.EventHandler _SomeEvent
{
add
{
// PREVENTS THE SAME HANDLER FROM BEING ATTACHED MORE THAN ONCE
Attach(ref _eSomeEvent, value);
}
remove
{
_eSomeEvent -= value;
}
}

private event System.EventHandler _eSomeEvent;

private void _FireSomeEvent()
{
Fire(_eSomeEvent, this, whatever);
}

public static void Fire(System.EventHandler h, object sender,
System.EventArgs args)
{
// PROTECTS FROM EVENT INVOKED WHEN NULL
if(h!=null) h(sender, args);
}

public static void Attach(ref System.EventHandler h1, System.EventHandler h2)
{
// PROTECTS FROM SAME HANDLER BEING ADDED MULTIPLE TIMES
if(h2==null) return;
if(h1!=null)
{
System.Delegate[] ar = h1.GetInvocationList();
foreach(System.Delegate dg in ar) if(dg == h2) return;
}
// (this is okay when h1 is null)
h1 += h2;
}

 

[Joanna Carter \(TeamB\)]

First... let's deal with Delegates. Comments welcome.

1) Invoking a NULL delegate is annoying.
** (It should just do nothing rather than crash.)

The class that invokes the delegate should always check for null before
calling it.

2) It's too easy to accidently attach multiple redundant handlers onto
delegates.

Not in well written code.

3) A delegate might point to handlers on objects that are out of scope or
been disposed, causing those objects to crash.
** (Handlers should be removed automatically when the object that defines
them leaves scope).

As long as the delegate in class that holds the event has a reference to a
method of another object, then that other object can't be garbage collected
and will remain valid.

There is sort of a potential for "delegate leak" where objects attach
handlers indefinitely while the handler functions get called an increasing
number of times per event...

This is unlikely in well-mannered code.

Unfortunately, the way to prevent the aforementioned problems is not
convenient...

So, for protective coding... must we truly declare the laws of proper
delegate usage as follows? (I hope we can resolve a more convenient way).

////////////////////////////////////////////////////////
///// ALL CLASSES THAT ATTACH TO THE EVENT CHAIN MUST...

// declare member variable of this
System.EventHandler _hEventHandler;

// during construction of this:
_hEventHandler = new System.EventHandler(whatever);

// during initialization of this:
_obj._SomeEvent += _hEventHandler;

// during disposal of this: (all users must call Dispose)
_obj._SomeEvent -= _hEventHandler;

////////////////////////////////////////////////////////

public delegate void NotificationHandler(object sender);


public class ReceiverClass
{
public void HandleNotification(object sender)
{
...
}
}

Then the code that creates an instance of this class can also attach its
handler method to the Sender class (see later code)

void MethodToCreateAndAttachToSender
{
SenderClasssc = new SenderClass();

ReceiverClass rc = new ReceiverClass();

sc.Notify += new NotificationHandler(rc.HandleNotification)
}

////////////////////////////////////////////////////////
///// ALL CLASSES SERVING AN EVENT MUST...
////////////////////////////////////////////////////////
public event System.EventHandler _SomeEvent
{
add
{
// PREVENTS THE SAME HANDLER FROM BEING ATTACHED MORE THAN ONCE
Attach(ref _eSomeEvent, value);
}
remove
{
_eSomeEvent -= value;
}
}

private event System.EventHandler _eSomeEvent;

private void _FireSomeEvent()
{
Fire(_eSomeEvent, this, whatever);
}

public static void Fire(System.EventHandler h, object sender,
System.EventArgs args)
{
// PROTECTS FROM EVENT INVOKED WHEN NULL
if(h!=null) h(sender, args);
}

public static void Attach(ref System.EventHandler h1, System.EventHandler h2)
{
// PROTECTS FROM SAME HANDLER BEING ADDED MULTIPLE TIMES
if(h2==null) return;
if(h1!=null)
{
System.Delegate[] ar = h1.GetInvocationList();
foreach(System.Delegate dg in ar) if(dg == h2) return;
}
// (this is okay when h1 is null)
h1 += h2;
}

This is overkill and not necessary; if you follow the guidelines for setting
up a multicast delegate as an event, then all you need to do is the
following :

public delegate void NotificationHandler(object sender);


public class SenderClass
{
private void OnNotify(object someObject)
{
if (Notify != null)
Notify(someObject);
}

public event NotificationHandler Notify;

public void SomeMethodThatNeedsToInvokeEvent
{
// make changes
OnNotify(this);
}

Why do you think you need all that extra code ??

Joanna

 

[david]

The class that invokes the delegate should always check for null before
calling it.

Well, obviously the poster is aware of that since he's suggesting the
behavior *should* be different.

 

[Joanna Carter \(TeamB\)]

Well, obviously the poster is aware of that since he's suggesting the
behavior *should* be different.

Since invoking any method on any null object will fail, I wanted to
reinforce that any other behaviour was not possible. The OP seems to be
making a simple principle unnecessarily complicated.

Joanna

 

[david]

First... let's deal with Delegates. Comments welcome.

1) Invoking a NULL delegate is annoying.
** (It should just do nothing rather than crash.)

This is something that I might have agreed with before 1.0, but at this
point I wouldn't want to change the language.

OTOH while I'd find this reasonable behavior for event handlers, I
wouldn't want callback delegates to silently fail when null is passed.
And treating the two circumstances differently feels too obscure for me.

2) It's too easy to accidently attach multiple redundant handlers onto
delegates.

When I first learned C#, I worried about this a lot. In practice, it
hasn't turned out to be an issue, at least not for me.

3) A delegate might point to handlers on objects that are out of scope or
been disposed, causing those objects to crash.
** (Handlers should be removed automatically when the object that defines
them leaves scope).

Making a delegate have built-in knowledge of Dispose feels very wrong to me.
And there's no generic way of telling if an object has been disposed.

I see what you're getting at here, but it's problematic in practice. For
one thing, there's the common pattern of having objects that exist only
to receive events.

For another, without deterministic finalization, I'm not sure you'd ever
get this to work correctly. At best, you'd have delegates interacting
with the GC in some very ugly ways, and this interaction would have be
to invoked every time you invoked a delegate, which can't be good for
performance.

 

[Guest]

This means, when the object is "disposed". Sorry.

 

[Guest]

1) Invoking a NULL delegate is annoying.

OTOH while I'd find this reasonable behavior for event handlers, I
wouldn't want callback delegates to silently fail when null is passed.
And treating the two circumstances differently feels too obscure for me.

Good point... neither would I come to think of it. Consider this:

I would rather check it for NULL when I'm interested, rather than be forced
to do it every time because it's going to crash my code when I don't.

We're sort of telling new programmers: Hey, look at this great short and
easy way to do this... oh, but don't forget you must also do these other
things every time. ;-)

When I first learned C#, I worried about this a lot. In practice, it
hasn't turned out to be an issue, at least not for me.

The thing I'm worried about, is any time code does this:
Object.Event += new FoobarEvent(eventHandler);
Or this:
Object.Event += delegate { whatever(); };

We are basically tying the lifetime of our this to the Object, because now
there's no way for this to clear the reference that Object has upon us.

If Object greatly outlives this, then we have a potential memory leak in the
program notwithstanding garbage collection, and to prevent it, Object needs
to flush its event system once in a while.

As you know, I am a proponent for deterministic finalization. I think it
doesn't have to change the wonderful heap we have now, but ohhh there are
things that can be done to make it better.

I really meant "dispose"... that doesn't change what you observed though. It
seems like you knew what I was aiming at.

 

[david]

The thing I'm worried about, is any time code does this:
Object.Event += new FoobarEvent(eventHandler);
Or this:
Object.Event += delegate { whatever(); };

We are basically tying the lifetime of our this to the Object, because now
there's no way for this to clear the reference that Object has upon us.

Actually, it seems wrong but you could clear it with

Object.Event -= new FoobarEvent(eventHandler);

I assume anonymous delegates work the same way, but I haven't played
with them much.

If Object greatly outlives this, then we have a potential memory leak in the
program notwithstanding garbage collection, and to prevent it, Object needs
to flush its event system once in a while.

I'm not entirely disagreeing. This would be a very useful thing to
have. But for reasons I posted before, I just don't see a reasonable
implementation of it. Not just from a CLR standpoint, but from a
standpoint of programmer expectations.

As you know, I am a proponent for deterministic finalization. I think it
doesn't have to change the wonderful heap we have now, but ohhh there are
things that can be done to make it better.

Ahh, I didn't realize that, I don't read this group that often. Yes,
with deterministic finalization delegates could work differently. But
now we're not talking about language changes, we're talking about a
completely different architecture.

 

[Guest]

Actually, it seems wrong but you could clear it with

Object.Event -= new FoobarEvent(eventHandler);

Sorry buddy... yes, it's wrong. The "new" operator will simply return a new
instance of a FoobarEvent handler.

You can add as many FoobarEvent handlers as you want:

Object.Event += new FoobarEvent(eventHandler);
Object.Event += new FoobarEvent(eventHandler);
Object.Event += new FoobarEvent(eventHandler);

And you can have your function called as many times as you want when the
event is invoked. It becomes a problem when Object is a long-lived object,
and let's say the Event is something like "CurrentSelectionChanged".

If this doesn't properly remove its event handler, as I showed in the
original post, then you get memory leaks on objects and event handlers
invoking after objects have disposed themselves, potentially crashing if the
handler erroneously uses disposed resources.

Therefore, doing:
Object.Event += new FoobarEvent(somefun);
- or -
Object.Event += delegate { somefunc(); };

Is bad coding practice.

NOTE: That problem not only promotes memory leaks, but it also risks
invoking functions on objects that think they've been disposed.

I assume anonymous delegates work the same way, but I haven't played
with them much.

Not unless you give the anonymous method a name by storing it in a variable,
and then removing that specific reference from the delegate... if possible.

Anyway. There is more to discuss on it. This is still a language in its
early stages, and I hope Microsoft will increase the number of people calling
shots in its development. Some of the things added in the language are stupid.

Although, it proudly deserves my endorsement as the most powerful language
available, and without a formal proof, you can take my word that I am highly
qualified to make that claim.

 

[david]

Sorry buddy... yes, it's wrong. The "new" operator will simply return a new
instance of a FoobarEvent handler.

public class CSimple
{
public event EventHandler MyEvent;
public void MyHandler(object sender, EventArgs e)
{

}

public CSimple()
{
MyEvent += new EventHandler(MyHandler);
Console.WriteLine(MyEvent.GetInvocationList().Length);
MyEvent += new EventHandler(MyHandler);
Console.WriteLine(MyEvent.GetInvocationList().Length);
MyEvent -= new EventHandler(MyHandler);
Console.WriteLine(MyEvent.GetInvocationList().Length);
MyEvent -= new EventHandler(MyHandler);
Console.WriteLine(MyEvent == null);

}

public static void Main(String[] args)
{
CSimple c = new CSimple();
Console.ReadLine();
}

}

This prints...

1
2
1
True

 

[Guest]

Thanks!

It certainly is possible to implement an object that has that behavior...
they must be looking at the contents of the EventHandler being removed,
rather than the address of the EventHandler.... Not very intuitive as a
language semantic.. it's not clearly pointed out in the documentation either.

Great experiment!

This will help me simplify my code a little bit.

Nice MFC styling, too ;-)

Sorry buddy... yes, it's wrong. The "new" operator will simply return a new
instance of a FoobarEvent handler.

public class CSimple
{
public event EventHandler MyEvent;
public void MyHandler(object sender, EventArgs e)
{

}

public CSimple()
{
MyEvent += new EventHandler(MyHandler);
Console.WriteLine(MyEvent.GetInvocationList().Length);
MyEvent += new EventHandler(MyHandler);
Console.WriteLine(MyEvent.GetInvocationList().Length);
MyEvent -= new EventHandler(MyHandler);
Console.WriteLine(MyEvent.GetInvocationList().Length);
MyEvent -= new EventHandler(MyHandler);
Console.WriteLine(MyEvent == null);

}

public static void Main(String[] args)
{
CSimple c = new CSimple();
Console.ReadLine();
}

}

This prints...

1
2
1
True

 

[Guest]

First... let's deal with Delegates. Comments welcome.

1) Invoking a NULL delegate is annoying.
** (It should just do nothing rather than crash.)

This would mean that the delegates would have some other way of having a
value of "no event handler is attached to this event". Although I agree
that it should've been this way when .NET was first conceived,
considering the amount of specialized code using events add anyway,
doing this in "magic" code shouldn't have been a problem.

Then again, this is how it is now, and I don't want them to change this.

2) It's too easy to accidently attach multiple redundant handlers onto
delegates.

Yes, if you don't have a clear picture of your code.

3) A delegate might point to handlers on objects that are out of scope or
been disposed, causing those objects to crash.
** (Handlers should be removed automatically when the object that defines
them leaves scope).

Having a delegate contain a reference to the object is in some cases one
of the few ways to keep an object alive. In any case, I'd hate for
Microsoft to add "special cases" to garbage collection: "Here, it's ok
to destroy that object because there's only delegates that keep it alive".

There is sort of a potential for "delegate leak" where objects attach
handlers indefinitely while the handler functions get called an increasing
number of times per event...

There is also the potential of memory "leak" where you add objects to an
arraylist and never remove them.

Unfortunately, the way to prevent the aforementioned problems is not
convenient...

So, for protective coding... must we truly declare the laws of proper
delegate usage as follows? (I hope we can resolve a more convenient way).

<snip>

To fix the "bug" of your code trying to add the same event handler more
than once, yes, but instead of treating the symtoms you should treat the
problem, make sure your code does not try to add the same event handler
more than once.

Additionally, you don't have to construct an object for your event so
that you can add and remove the exact same one from outside, you can
just do:

obj.event += new DelegateName(methodName)
obj.event -= new DelegateName(methodName)

The delegate code is smart enough to know that these will refer to the
same one.

 

[Jon Skeet [C# MVP]]

Sorry buddy... yes, it's wrong. The "new" operator will simply return a new
instance of a FoobarEvent handler.

Yes, but the behaviour of Equals in Delegate is:
<quote>
Determines whether the specified object and the current singlecast
(noncombinable) delegate share the same target, method, and invocation
list.

You can add as many FoobarEvent handlers as you want:

Object.Event += new FoobarEvent(eventHandler);
Object.Event += new FoobarEvent(eventHandler);
Object.Event += new FoobarEvent(eventHandler);

And you can have your function called as many times as you want when the
event is invoked.

And sometimes you may actually want to do that.

It becomes a problem when Object is a long-lived object,

and let's say the Event is something like "CurrentSelectionChanged".

If this doesn't properly remove its event handler, as I showed in the
original post, then you get memory leaks on objects and event handlers
invoking after objects have disposed themselves, potentially crashing if the
handler erroneously uses disposed resources.

Therefore, doing:
Object.Event += new FoobarEvent(somefun);
- or -
Object.Event += delegate { somefunc(); };

Is bad coding practice.

Not in my view.

Although, it proudly deserves my endorsement as the most powerful language
available, and without a formal proof, you can take my word that I am highly
qualified to make that claim.

Are you serious? Without a definition of what you mean by "most
powerful" it's a claim with no meaning.

 

[Paul Robson]

First... let's deal with Delegates. Comments welcome.

1) Invoking a NULL delegate is annoying.
** (It should just do nothing rather than crash.)

Matter of opinion. I suspect it comes from the function pointer
background....... one could argue that a delegate should have a handler
attached, and if it doesn't it's probably an error anyway. It's easy
enough to check for null.

2) It's too easy to accidently attach multiple redundant handlers onto
delegates.

Well, this is the developers fault, surely ? If there's a problem then
delegates could be added/removed through a method dedicated to that
which allows logging of handlers.

3) A delegate might point to handlers on objects that are out of scope or
been disposed, causing those objects to crash.
** (Handlers should be removed automatically when the object that defines
them leaves scope).

The created object should exist until it is garbage collected, which it
won't do if there are references to it still active .... (I think !)

 

[Guest]

Paul Robson wrote:

The created object should exist until it is garbage collected, which it
won't do if there are references to it still active .... (I think !)

Correct. If a delegate contains a reference to an object that has no
other live references, as long as the delegate is alive and kicking, the
object will be too.

 

[Jonathan Allen]

1) Invoking a NULL delegate is annoying.

** (It should just do nothing rather than crash.)

C# could (should?) add a RaiseEvent keyword like VB. But one of C#'s key
selling points is the lack of syntactic sugar, unlike VB, which adds
keywords for everything they can think of.

--
Jonathan Allen

First... let's deal with Delegates. Comments welcome.

1) Invoking a NULL delegate is annoying.
** (It should just do nothing rather than crash.)

2) It's too easy to accidently attach multiple redundant handlers onto
delegates.

3) A delegate might point to handlers on objects that are out of scope or
been disposed, causing those objects to crash.
** (Handlers should be removed automatically when the object that defines
them leaves scope).

4) ?
5) ?

There is sort of a potential for "delegate leak" where objects attach
handlers indefinitely while the handler functions get called an increasing
number of times per event...


Unfortunately, the way to prevent the aforementioned problems is not
convenient...

So, for protective coding... must we truly declare the laws of proper
delegate usage as follows? (I hope we can resolve a more convenient way).

////////////////////////////////////////////////////////
///// ALL CLASSES THAT ATTACH TO THE EVENT CHAIN MUST...

// declare member variable of this
System.EventHandler _hEventHandler;

// during construction of this:
_hEventHandler = new System.EventHandler(whatever);

// during initialization of this:
_obj._SomeEvent += _hEventHandler;

// during disposal of this: (all users must call Dispose)
_obj._SomeEvent -= _hEventHandler;

////////////////////////////////////////////////////////


////////////////////////////////////////////////////////
///// ALL CLASSES SERVING AN EVENT MUST...
////////////////////////////////////////////////////////
public event System.EventHandler _SomeEvent
{
add
{
// PREVENTS THE SAME HANDLER FROM BEING ATTACHED MORE THAN ONCE
Attach(ref _eSomeEvent, value);
}
remove
{
_eSomeEvent -= value;
}
}

private event System.EventHandler _eSomeEvent;

private void _FireSomeEvent()
{
Fire(_eSomeEvent, this, whatever);
}

public static void Fire(System.EventHandler h, object sender,
System.EventArgs args)
{
// PROTECTS FROM EVENT INVOKED WHEN NULL
if(h!=null) h(sender, args);
}

public static void Attach(ref System.EventHandler h1, System.EventHandler
h2)
{
// PROTECTS FROM SAME HANDLER BEING ADDED MULTIPLE TIMES
if(h2==null) return;
if(h1!=null)
{
System.Delegate[] ar = h1.GetInvocationList();
foreach(System.Delegate dg in ar) if(dg == h2) return;
}
// (this is okay when h1 is null)
h1 += h2;
}