Friday, 23 September 2011

The problem with 'global' variables

Those of us who used C and C++ were warned that global variables are not good. We mostly agreed but would sneak things in if we could! C# might not have globals per-se but actually it has static variables which can exhibit several similar problems. Consider the following method in an asp web app:
private static void OnApplicationError(object sender, EventArgs e)
        {
            HttpApplication context = HttpContext.Current.ApplicationInstance;
            
            Exception ex = context.Server.GetLastError();
            context.Server.ClearError();

            if (HttpContext.Current.Handler is IRequiresSessionState)
            {                
                context.Session["UnhandledException"] = FormatError(ex);
            }
            
            EventLogWriter.WriteEvent(Constants.EventLogSource_PoSSite, (long) Events.UnhandledException,
                                      (int) Categories.UnhandledException, EventLogEntryType.Error, ex);

            context.Response.Redirect("Error.aspx");
        }
All pretty boilerplate stuff. The session is used in this case to pass the exception detail into the error page to display if required.
In a live system, this method was crashing but we did not know it was crashing, all we knew was the system was intermittently crawling to a halt. I was looking into this method because the event log messages were NOT being logged even though this was the event handler.
The problem, as you might have guessed was caused by a 'global'. What was happening was that underneath the call to WriteEvent, a call was made to obtain the 'global' Context to add certain information automatically to the event. What had happened was that the Context logic was broken and it was in some scenarios attempting to access the OperationContext from a service which didn't exist which threw an exception before the event was written. This in term caused the app to automatically forward to the error page (this page!) which then tried to write the new exception and caused exactly the same thing to happen, falling into a tight loop. The way it should have been written was:
private static void OnApplicationError(object sender, EventArgs e)
        {
            // snip
            EventLogWriter.WriteEvent(GetCurrentContext(), Constants.EventLogSource_PoSSite, (long) Events.UnhandledException,
                                      (int) Categories.UnhandledException, EventLogEntryType.Error, ex);

            // snip
        }
where it would have been obvious that WriteEvent consumes the context and which would have given us a clue as to what might have not been set. As it was, the 'global' access was hiding this fact (the WriteEvent was in an external library so not obviously available).
In short, don't use globals!
Post a Comment