Firing events from a constructor.

[damiensawyer]

Hi,

I am creating a class (from a base class) that needs to trigger events
(code at bottom). I am instatiating the classes and wiring up the
events as follows.

clsDetermineConnection oDC = new
clsDetermineConnection(Request);
oDC.LogMessage += new
RunTraceBase.TraceArguments(LogMessagesFromEvents) ;

The issue is that, because the event is not wired up to
LogMessagesFromEvents until after the constructor fires, constructor
events aren't trapped. Is there any nice workaround to this that I'm
missing? Other of course than calling an 'initialise' method after
instatiation?

Thanks very much in advance,
Damien


/// <summary>
/// Abstract class to implement logging events on custom classes.
/// </summary>
abstract protected class RunTraceBase
{
public delegate void TraceArguments(object Sender,
TraceEventArgs args);
public event TraceArguments LogMessage;
protected void RaiseLogMessage(string Message)
{
if (LogMessage != null) LogMessage(this, new
TraceEventArgs(Message));
}

// custom attributes
public class TraceEventArgs : System.EventArgs
{
private string message;
public TraceEventArgs(string m)
{
this.message = m;
}
public string Message()
{
return message;
}
}
}

protected class clsDetermineConnection : RunTraceBase
{.........

/// <summary>
/// Constructor
/// </summary>
/// <param name="Request"></param>
public clsDetermineConnection(HttpRequest Request)
{
RaiseLogMessage("Request Host=blah blah blah"); // Note -
won't fire on the constructor if the event hasn't been wired up yet.
}

[Peter Duniho]

On Mon, 09 Jun 2008 22:54:05 -0700, da**********@yahoo.com.au
<da**********@yahoo.com.auwrote:

[...]
The issue is that, because the event is not wired up to
LogMessagesFromEvents until after the constructor fires, constructor
events aren't trapped. Is there any nice workaround to this that I'm
missing? Other of course than calling an 'initialise' method after
instatiation?

Some possibilities:

* Create a static event that allows logging of messages that occur
during object construction
* Pass a handler delegate to the constructor for logging of such
messages
* Put messages that happen during construction into a queue, and
implement a custom event with a setter that dequeues all the messages in
the queue when the first handler is subscribed (raising the event for each
one, of course)
* Fix the class API so that you aren't logging messages during
construction (this might mean adding a special "initializer" method or
something else...not enough information in your post to know)

Personally, I'd try really hard for the last solution. And I would avoid
the second-to-last solution like the plague. :)

The first two seem to me to be reasonable alternatives if the preferred
solution turns out to be completely unworkable.

Pete

[Peter Duniho]

Oh, and by the way...some comments about the code you posted:

On Mon, 09 Jun 2008 22:54:05 -0700, da**********@yahoo.com.au
<da**********@yahoo.com.auwrote:

/// <summary>
/// Abstract class to implement logging events on custom classes.
/// </summary>
abstract protected class RunTraceBase
{
public delegate void TraceArguments(object Sender,
TraceEventArgs args);

Since you're using the .NET pattern, you should probably follow their
naming convention. "TraceArgumentsHandler" is the correct delegate name
and "TraceArgumentsEventArgs" is the correct EventArgs-subclass name. Or
"TraceHandler" and "TraceEventArgs". The important thing is to pick an
event name, and then add "Handler" and "EventArgs" to that name for the
delegate and EventArgs-subclass, respectively. And ideally, the event
name would be "TraceArguments" or "Trace", respectively (depending on
which consistent naming you ultimate choose).

The main exception I can think of for the naming of the event would be if
you have some general-purpose EventArgs-subclass that is used by multiple
events. For example, obviously all of the events that just use EventArgs
directly have some completely different name, rather than the empty
string. :) A less obvious example would be events like MouseUp,
MouseDown, MouseMove, which all use a MouseEventHandler and MouseEventArgs
but of course are not all named "Mouse". :)

public event TraceArguments LogMessage;
protected void RaiseLogMessage(string Message)
{
if (LogMessage != null) LogMessage(this, new
TraceEventArgs(Message));
}

This is not thread-safe. You should store the event value in a local
variable and then use it:

protected void RaiseLogMessage(string Message)
{
TraceArguments handler = LogMessage;

if (handler != null)
{
handler(this, new TraceEventArgs(Message));
}
}

Otherwise, the value of "LogMessage" could change between the time you
check it for null and try to use it (with it getting set to null after you
check it being the obvious hazard :) ).

>
// custom attributes
public class TraceEventArgs : System.EventArgs
{
private string message;
public TraceEventArgs(string m)
{
this.message = m;
}
public string Message()
{
return message;
}

"Message" should be a property, not a method.

Pete

[Marc Gravell]

This is not thread-safe.

Just a minor point; most classes don't *need* to be thread-safe (and
indeed, aren't), so in most cases the local variable step is overkill.
This would only be an issue in a small handful of cases.

Also - one of Jon's tricks (that I tend not to use personally, as I'd
usually prefer to deal with the null than have an unnecessary heap
object per instance):

using System;

class Foo
{
public event EventHandler Bar = delegate { };

public void OnBar() // normally protected virtual...
{
Bar(this, EventArgs.Empty);
}

static void Main()
{
Foo foo = new Foo();
foo.Bar += delegate { Console.WriteLine("Hi"); };
foo.OnBar();
}
}

Finally - a bit random and maybe off-topic, but I was *going* to say
that if you have a number of events, you might throw in a helper
method like:
protected void OnEvent(EventHandler handler)
{
if (handler != null) handler(this, EventArgs.Empty);
}
to do this for you, so you can just write OnEvent(Bar); - but if you
have multiple events, then EventHandlerList is a better option since
if we assume that only a portion of events are hooked per instance, we
are making quite a saving by treating it as sparse - for example
(showing only 1 event) see below.

Marc

private EventHandlerList events;
protected void ClearEvents() { events = null; }
protected void AddHandler(object key, EventHandler handler)
{
if (key == null) throw new ArgumentNullException("key");
if (handler != null)
{
if (events == null) events = new EventHandlerList();
events.AddHandler(key, handler);
}
}
protected void RemoveHandler(object key, EventHandler handler)
{
if (key == null) throw new ArgumentNullException("key");
if (events != null && handler != null)
{
events.RemoveHandler(key, handler);
}
}
protected void OnEvent(object key)
{
if (key == null) throw new ArgumentNullException("key");
if (events != null)
{
EventHandler handler = (EventHandler)events[key];
if (handler != null) handler(this, EventArgs.Empty);
}
}

// actual events
private static readonly object BarKey = new object();
public event EventHandler Bar
{
add { AddHandler(BarKey, value); }
remove { RemoveHandler(BarKey, value); }
}
public void DoSomet()
{
OnEvent(BarKey);
}

[Peter Duniho]

On Tue, 10 Jun 2008 00:24:15 -0700, Marc Gravell <ma**********@gmail.com
wrote:

>This is not thread-safe.

Just a minor point; most classes don't *need* to be thread-safe (and
indeed, aren't), so in most cases the local variable step is overkill.
This would only be an issue in a small handful of cases.

I agree with the "most classes don't need to be thread-safe" comment.
However, I disagree somewhat with the implication that it applies here. :)

In particular, events themselves are by default thread-safe. That is,
subscribing and unsubscribing is a thread-safe operation for
compiler-generated events. While one might argue that nobody should be
assuming that they in fact are, the facts of life are that people will
make exactly that assumption even if you try to warn them against it.
Given that it's the cross-thread-unsubscribe scenario that is specifically
the issue here, I feel that it's worth making the event invocation itself
thread-safe as well, even though the class may generally not be
thread-safe. It's such a simple thing to do, and has such an
inconsequential effect on performance, I don't see any point in _not_
doing it.

[...]
Finally - a bit random and maybe off-topic, but I was *going* to say
that if you have a number of events, you might throw in a helper
method like:
protected void OnEvent(EventHandler handler)
{
if (handler != null) handler(this, EventArgs.Empty);
}
to do this for you, so you can just write OnEvent(Bar); - but if you
have multiple events, then EventHandlerList is a better option since
if we assume that only a portion of events are hooked per instance, we
are making quite a saving by treating it as sparse

Nice. I didn't even know that class existed. I wonder if that's what the
Control class uses (I know that it has a sparse look-up of some sort, but
don't know the specifics).

That said, I'd be a bit worried if my total number of events was high and
I had no way to ensure that subscribed events would be sparse (for
example, events that are semantically mutually exclusive), since
EventHandlerList uses a linear search. If you really have a large number
of events, a Dictionary of keys and handlers might be a safer bet, even if
it does have a little more memory overhead. This would be especially true
if the events are something that would be invoked frequently.

Pete

[Marc Gravell]

Yes, that is what Component (and thus Control) use to provide sparse
events; re the linear search, dictionary would only start to be
quicker for high numbers of events - for most typical classes (i.e.
<100 events), linear search is probably still the faster option.
Actually, the main gripe I have with EventHandlerList is that I can't
check whether it is empty. It would be nice to throw away the list if
the last handler unsubscribes...

Marc

[damiensawyer]

Thanks to both of you - that was great!

Lots to play with there - however chances are I'll implement most of
it.

For the record, with regards to my original question, I ended up
taking the option of passing in logging function using a delegate.

Cheers,
Damien

[Marc Gravell]

Damien; glad to help ;-p

Pete - did a quick test, and for me the "get" time balances
(dictionary vs eventhandlerlist) at around 150 (subscribed) events,
based on still using an object key; dictionary has the higher
insertion cost, of course. I've taken into account the need to do a
Delegate.Combine to be comparable to EventHandlerList.

Marc

using System;
using System.Collections.Generic;
using System.ComponentModel;
using System.Diagnostics;
static class Program
{

static void Main()
{
RunTest(1,1); // JIT

RunTest(10, 5000); // JIT
RunTest(50, 5000); // JIT
RunTest(100, 5000); // JIT
RunTest(150, 5000); // JIT
RunTest(200, 5000); // JIT
}
static void RunTest(int count, int repeat)
{
GC.Collect(GC.MaxGeneration, GCCollectionMode.Forced);
GC.Collect(GC.MaxGeneration, GCCollectionMode.Forced);
object[] keys = new object[count];
EventHandler[] handlers = new EventHandler[count];
for (int i = 0; i < keys.Length; i++)
{
keys[i] = new object();
handlers[i] = delegate { };
}

Stopwatch watch = Stopwatch.StartNew();
EventHandlerList list = null;
for (int r = 0; r < repeat; r++)
{
list = new EventHandlerList();
for (int i = 0; i < keys.Length; i++)
{
list.AddHandler(keys[i], handlers[i]);
}
for (int i = 0; i < keys.Length; i++)
{
list.AddHandler(keys[i], handlers[i]);
}
}
watch.Stop();
Console.WriteLine("EventHandlerList:Add 2x{0}x{1}; {2}",
count,repeat,watch.ElapsedMilliseconds);

watch.Reset();
watch.Start();
for (int r = 0; r < repeat; r++)
{
for (int i = 0; i < keys.Length; i++)
{
EventHandler handler = (EventHandler)list[keys[i]];
}
}
watch.Stop();
Console.WriteLine("EventHandlerList:Get {0}x{1}; {2}",
count, repeat, watch.ElapsedMilliseconds);

watch.Reset();
watch.Start();
Dictionary<object, EventHandlerdict = null;
for (int r = 0; r < repeat; r++)
{
dict = new Dictionary<object, EventHandler>();
for (int i = 0; i < keys.Length; i++)
{
EventHandler old;
if (dict.TryGetValue(keys[i], out old))
{
dict[keys[i]] =
(EventHandler)Delegate.Combine(old, handlers[i]);
}
else
{
dict.Add(keys[i], handlers[i]);
}
}
for (int i = 0; i < keys.Length; i++)
{
EventHandler old;
if (dict.TryGetValue(keys[i], out old))
{
dict[keys[i]] =
(EventHandler)Delegate.Combine(old, handlers[i]);
}
else
{
dict.Add(keys[i], handlers[i]);
}
}
}
watch.Stop();
Console.WriteLine("Dictionary:Add 2x{0}x{1}; {2}",
count, repeat, watch.ElapsedMilliseconds);

watch.Reset();
watch.Start();
for (int r = 0; r < repeat; r++)
{
for (int i = 0; i < keys.Length; i++)
{
EventHandler handler = dict[keys[i]];
}
}
watch.Stop();
Console.WriteLine("Dictionary:Get {0}x{1}; {2}",
count, repeat, watch.ElapsedMilliseconds);
}
}

[Chris Shepherd]

Marc Gravell wrote:

Also - one of Jon's tricks (that I tend not to use personally, as I'd
usually prefer to deal with the null than have an unnecessary heap
object per instance):

You could just have one static empty delegate that takes care of default
event subscription. I've used this before in some of my homebrew apps:
ie (simplifying):
public static EventHandler DefaultHandler = delegate { };
public event EventHandler myEvent = SomeClass.DefaultHandler;

If we had extension properties we could even add our own
EventHandler.Default or something along those lines.

It would be nice IMO if the compiler itself did that automatically for
you. You always want to be able to just say "invoke this event"
regardless of whether or not anyone is listening.

Chris.

[Marc Gravell]

If we had extension properties...

Well, if we bring extension methods into the party we can deal with
them at the caller:

public static class EventExt
{
public static void SafeInvoke(this EventHandler handler, object
sender)
{
if (handler != null) handler(sender, EventArgs.Empty);
}
public static void SafeInvoke<TEventArgs>(this
EventHandler<TEventArgshandler,
object sender, TEventArgs args) where TEventArgs : EventArgs
{
if (handler != null) handler(sender, args);
}
}

But you'd need to keep adding extension methods for the other event-
types...

Marc

[Jon Skeet [C# MVP]]

On Jun 10, 2:04 pm, Chris Shepherd <[e-mail]:

Marc Gravell wrote:

Also - one of Jon's tricks (that I tend not to use personally, as I'd
usually prefer to deal with the null than have an unnecessary heap
object per instance):

You could just have one static empty delegate that takes care of default
event subscription. I've used this before in some of my homebrew apps:
ie (simplifying):
public static EventHandler DefaultHandler = delegate { };
public event EventHandler myEvent = SomeClass.DefaultHandler;

Yes - the only problem with this is that each type of event handler
needs its own field. You could do it for a generic
EventHandler<TEventArgsthough. Using "delegate {}" directly just
means I can always be absolutely consistent for any delegate type
which doesn't have to return a value.

If we had extension properties we could even add our own
EventHandler.Default or something along those lines.

It would be nice IMO if the compiler itself did that automatically for
you. You always want to be able to just say "invoke this event"
regardless of whether or not anyone is listening.

Yes. I can see why it doesn't, but it would be nice to have some way
of asking it to do so in common cases - or have a general "build a no-
op handler for this if one doesn't already exist" library call.

Jon

[Chris Shepherd]

Jon Skeet [C# MVP] wrote:

Yes - the only problem with this is that each type of event handler
needs its own field. You could do it for a generic
EventHandler<TEventArgsthough. Using "delegate {}" directly just
means I can always be absolutely consistent for any delegate type
which doesn't have to return a value.

True. Come to think of it most of the events I've dealt with are UI
events, which are generally just the EventHandler type.
I wasn't criticizing btw, just offering up a discussion point. :)

Yes. I can see why it doesn't, but it would be nice to have some way
of asking it to do so in common cases - or have a general "build a no-
op handler for this if one doesn't already exist" library call.

Even an attribute to decorate the event would be an acceptable
compromise for me.

[AutoGenerateEventHandlerDelegate]
public event EventHandler myEvent;

Or possibly consider doing the reverse -- suppressing the
auto-generation in cases where it makes sense.
Chris.

[Chris Shepherd]

Marc Gravell wrote:

>If we had extension properties...

Well, if we bring extension methods into the party we can deal with
them at the caller:

[...]

But you'd need to keep adding extension methods for the other event-
types...

Good point. It looks like any way conceivable would require modification
to the compiler and/or a new language feature.

Chris.

[Jon Skeet [C# MVP]]

On Jun 10, 2:47 pm, Chris Shepherd <[e-mail]:

Yes - the only problem with this is that each type of event handler
needs its own field. You could do it for a generic
EventHandler<TEventArgsthough. Using "delegate {}" directly just
means I can always be absolutely consistent for any delegate type
which doesn't have to return a value.

True. Come to think of it most of the events I've dealt with are UI
events, which are generally just the EventHandler type.
I wasn't criticizing btw, just offering up a discussion point. :)

Ditto. If I ever start complaining because people bring up
alternatives to my own style, please take away my keyboard. :)

Yes. I can see why it doesn't, but it would be nice to have some wa
of asking it to do so in common cases - or have a general "build a no-
op handler for this if one doesn't already exist" library call.

Even an attribute to decorate the event would be an acceptable
compromise for me.

[AutoGenerateEventHandlerDelegate]
public event EventHandler myEvent;

Possibly. There would be some oddities around though - imagine an
event in a struct. (Admittedly that's clearly something to avoid to
start with...)

Or possibly consider doing the reverse -- suppressing the
auto-generation in cases where it makes sense.

I think I'd want to work out all the edge cases first, but it's an
interesting idea.

Jon

[Chris Shepherd]

Jon Skeet [C# MVP] wrote:

Possibly. There would be some oddities around though - imagine an
event in a struct. (Admittedly that's clearly something to avoid to
start with...)

Hmm. How do events on structs even work? Does it just copy the entire
list of subscribed events each time, or does it somehow go deeper than that?

Structs could simply have the caveat that they can't be auto-subscribed
to a default delegate.

>Or possibly consider doing the reverse -- suppressing the
auto-generation in cases where it makes sense.

I think I'd want to work out all the edge cases first, but it's an
interesting idea.

Once I get my subscription details setup at work again, I'll have to
peruse the Connect site and see if there's any existing suggestions
along these lines.

Chris.

[Jon Skeet [C# MVP]]

On Jun 10, 3:43 pm, Chris Shepherd <[e-mail]:

Possibly. There would be some oddities around though - imagine an
event in a struct. (Admittedly that's clearly something to avoid to
start with...)

Hmm. How do events on structs even work? Does it just copy the entire
list of subscribed events each time, or does it somehow go deeper than that?

It would just copy the field, which is a reference to a delegate. The
delegate object itself contains the list.

Structs could simply have the caveat that they can't be auto-subscribed
to a default delegate.

Yup.

Or possibly consider doing the reverse -- suppressing the
auto-generation in cases where it makes sense.

I think I'd want to work out all the edge cases first, but it's an
interesting idea.

Once I get my subscription details setup at work again, I'll have to
peruse the Connect site and see if there's any existing suggestions
along these lines.

I had one along the lines of having a method or language construct to
return a "no-op" delegate. Was closed as "won't fix" which I didn't
mind given the delegate{} kludge.

Jon