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;
}

"Marshal" <(E-Mail Removed)> a écrit dans le message de
news: 9EBD9425-70AD-45D5-8B09-(E-Mail Removed)...

> 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

--
Joanna Carter
Consultant Software Engineer

On 2005-09-18, Joanna Carter (TeamB) <(E-Mail Removed)> wrote:
> "Marshal" <(E-Mail Removed)> a écrit dans le message de
> news: 9EBD9425-70AD-45D5-8B09-(E-Mail Removed)...
>
>> 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.

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

"david" <(E-Mail Removed)> a écrit dans le message de news:
(E-Mail Removed)ldomain...

> 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

--
Joanna Carter
Consultant Software Engineer

On 2005-09-18, Marshal <(E-Mail Removed)> wrote:
>
> 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.

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

> 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).

>> 1) Invoking a NULL delegate is annoying.
>> ** (It should just do nothing rather than crash.)
> 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. ;-)

> > 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.

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.

> > 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).

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

>
> 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.
>
>
>

On 2005-09-19, Marshal <(E-Mail Removed)> wrote:
>> > 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.
>
> 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.

> 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.

On 2005-09-19, Marshal <(E-Mail Removed)> wrote:
>> 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.

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