Ayende @ Rahien

Refunds available at head office

How NOT to write a logger

I am going over some library code at the moment, and I am following my usual routine of starting at the basic infrastructure to find the fault lines. Take a look at this:

public void Log(string pSource, string pMessage, EventLogEntryType pEntryType) {
    try {
        if (!EventLog.SourceExists(pSource)) {
            EventLog.CreateEventSource(pSource, "Application");
        }

        EventLog.WriteEntry(pSource, pMessage, pEntryType);
    }
    catch (Exception _ex) {
        Log("", _ex.ToString(), EventLogEntryType.Error);
    }
}
This is… impressive.

Comments

m4bwav
03/19/2009 05:43 PM by
m4bwav

awesome, I almost missed the recursion

Joel
03/19/2009 05:53 PM by
Joel

That is impressive. I think that will lead to a StackOverflowException, right?

According to the MSDN docs, calling EventLog.CreateEventSource or EventLog.WriteEntry with the source parameter set to an empty string ("") will generate an ArgumentException.

Therefore, if during the first run through the function it throws an exception, the catch block will try and call into the Log function again (recursively) using a blank string, which will generate another exception, until a stack overflow occurs.

That's my guess. :)

addy santo
03/19/2009 06:03 PM by
addy santo

down that path insanity lies :)

Dave
03/19/2009 07:00 PM by
Dave

And than they say programmers don't have humor ;-)

Mauríco
03/19/2009 07:52 PM by
Mauríco

This might be the most brilliant try...catch block I have ever seen.

Chris Smith
03/19/2009 08:16 PM by
Chris Smith

Genius. Sheer genius.

Nair
03/19/2009 09:05 PM by
Nair

I guess it is a joke right?

Nick
03/19/2009 11:40 PM by
Nick

While leads me to a question: What is the best way to log that your logger failed?

Jose
03/19/2009 11:45 PM by
Jose

Maybe works fine for EventLogEntryType.Error.

is a joke

Stefan
03/20/2009 12:07 AM by
Stefan

Heh I have seen this many times before, but on something like an error page, it would inherit the base page class which in on load would fail to connect to the DB and redirect to error page.

Endless redirect to error error error error, all the while sending error emails. That day I got 200,000 error emails from the site, fun fun fun.

configurator
03/20/2009 12:08 AM by
configurator

Reminds me of something similar that we have where I used to work:

void Save() {

Recalculate();

SaveToDatabase();

}

void Recalculate() {

try {

    // some stuff

} catch {

    Save();

}

}

It's still there. "The "some stuff" shouldn't fail so why fix that part"

Of course, every once in a while someone makes a change and "some stuff" does fail, resulting in nasty bugs.

Mr Berre
03/20/2009 01:26 AM by
Mr Berre

Ugh, reminds me of a bit of code I discovered recently in the web ui for a new project I was working on. All aspx pages inherit from a base class, which contains a check whether a user is allowed to even see anything in the UI. If a user wasn't allowed to, he was redirected to Error.aspx, which of course inherited from the base class and which of course triggered the check...

The UI code was based on another project's code (no need to re-invent the wheel) that has been in use for 6+ months. I'm still amazed that they've never had an issue with that, since usually the business people are very good at pointing people to apps they don't have access to.

miro
03/20/2009 11:06 AM by
miro

sfa

Tool
03/20/2009 01:42 PM by
Tool

If at first you don't succeed...

Paula
03/20/2009 02:12 PM by
Paula

Brillant!

ana
03/20/2009 08:44 PM by
ana

I have seen this many times before, but on something like an error page,

Comments have been closed on this topic.