Visual Studio 2008 Managed Code Analysis

I had a quick look at the managed code analysis (FxCop) rules the other day to see what’s new and what’s been removed.  Unfortunately, one of the analysis engines wasn’t able to be “resurrected” in time for the release, so there’s a few really useful rules that haven’t stayed with Beta 2 the reslease of Visual Studio 2008.  Fortunately, there have been some additions too.  Fortunately, there is not a deficit, we’re up by 12 new rules.


Following are the new rules:


  • DoNotRaiseExceptionsInUnexpectedLocations
  • NormalizeStringsToUppercase
  • SpecifyMarshalingForPInvokeStringArguments
  • SpecifyStringComparison
  • UseOrdinalStringComparison
  • AvoidUnmaintainableCode
  • ReviewMisleadingFieldNames
  • AvoidExcessiveClassCoupling
  • IdentifiersShouldBeCasedCorrectly
  • IdentifiersShouldBeSpelledCorrectly
  • IdentifiersShouldDifferByMoreThanCase
  • IdentifiersShouldNotContainTypeNames
  • OnlyFlagsEnumsShouldHavePluralNames
  • ResourceStringCompoundWordsShouldBeCasedCorrectly
  • ResourceStringsShouldBeSpelledCorrectly
  • MarkAssembliesWithNeutralResourcesLanguage
  • RemoveEmptyFinalizers
  • CatchNonClsCompliantExceptionsInGeneralHandlers
  • TestForNaNCorrectly
  • AttributeStringLiteralsShouldParseCorrectly
  • SecurityTransparentAssembliesShouldNotContainSecurityCriticalCode
  • SecurityTransparentCodeShouldNotAssert
  • SecurityTransparentCodeShouldNotReferenceNonpublicSecurityCriticalCode
  • CatchNonClsCompliantExceptionsInGeneralHandlers

And following are the rules that were removed:




  • ProvideCorrectArgumentsToFormattingMethods

  • DisposeMethodsShouldCallBaseClassDispose


  • DoNotCallPropertiesThatCloneValuesInLoops


  • DoNotDisposeObjectsMultipleTimes


  • SpecifyMarshalingForPInvokeStringArguments


  • DoNotPassLiteralsAsLocalizedParameters


  • LongAcronymsShouldBePascalCased


  • ShortAcronymsShouldBeUppercase


  • AvoidUnnecessaryStringCreation

  • DoNotConcatenateStringsInsideLoops


  • SecureGetObjectDataOverrides

  • AssembliesShouldDeclareMinimumSecurity

Thread.Abort is a Sign of a Poorly Designed Program

Continuing the theme of Thead.Sleep is a sign of a poorly designed program, I’ve been meaning to provide similar detail on Thread.Abort and not just allude to it in other posts like ‘System.Threading.Thread.Suspend() is obsolete: ‘Thread.Suspend has been deprecated….

Many of the concepts I’ve discussed regarding Thread.Suspend also apply to Thread.Abort, and in much the same way that the ability to terminate a thread has existed for so long that the concept has remained ubiquitous when dealing with threads and it just keeps getting implemented without thought.  Thread.Abort is far more unsafe than Thread.Suspend; but, unfortunately Thread.Abort has yet to be deprecated.

Unlike Thread.Suspend, Thread.Abort stops your thread and it can’t continue afterwards.  In the same vein as Thread.Suspend, Thread.Abort doesn’t know anything about your thread, and if your thread isn’t in a try block that handles ThreadAbortException, it just stops it where it is, which may be in the middle of updating a complex invariant.  This means the thread can’t continue where the left off like Thread.Suspend and Thread.Resume and can never get that invariant out of that corrupt state (and nothing else can, it has no idea where that thread left off).

Thread.Abort isn’t a full-blown terminate; it does cause all finally blocks that it knows about to execute before your thread is stopped and won’t terminate your thread while it’s in a catch or finally block.

There’s several concepts that go along with Thread.Abort.  One I’ve already mentioned, the ThreadAbortException exception.  Catching ThreadAbortException is conceptually cooperative thread termination.  My issue with catching ThreadAbortException is that you’re essentially using exceptions for normal flow of logic; which I don’t agree with.  The other concept is critical regions.  You might think that wrapping critical code with Thread.BeginCritcalRegion and Thread.EndCriticalRegion will mean your thread won’t be aborted while it’s executing that code.  Unfortunately, the .NET Framework doesn’t really us Begin/EndCriticalRegion for anything.  But, that’s also not the intention, the intention is to signify to the runtime (currently only SQL Server vNext, I believe, uses that information) that should a Thread.Abort be called on that thread, or an unhandled exception occur, corruption has occurred and it should simply unload the thread’s AppDomain.

But, the concept of interrupting the thread in the middle of something is really why you should never use Thread.Abort.  It’s one thing to interrupt you own code, but Thread.Abort actually as the potential to interrupt lock statements and cause locks not to be unlocked.  As Joe Duffy points out, there’s a check when running code on the x86 that makes sure Thread.Abort can’t interrupt between a call to Monitor.Enter when it is adjacent to a protected region.  But, on current 64-bit JITs this check does not exist and a thread can be aborted between the call to Monitor.Enter and the start of the protected region.

Unfortunately, the x86 is not entirely immune.  In the C# compiler, when optimizations are turned off, it emits no-ops in various places.  I can’t really confirm why, but the prevalent theory is to allow the Visual Debugger to put breakpoints on source code that otherwise wouldn’t have corresponding IL. (like the start brace, for example).  Unfortunately there’s a bug in the generation of that IL that means Thread.Abort can cause locks not to be unreleased.

Eric Lippert recently provided some of the detail of the bug on his blog.  But, essentially, this is what the C# compiler generates for IL code for the lock statement:
            call void [mscorlib]System.Threading.Monitor::Enter(object)
Hole:       nop
StartTry:   nop
            ldstr “in lock”
            call void [mscorlib]System.Console::WriteLine(string)
            nop
            nop
            leave.s EndFinally
EndTry:     ldloc.0
            call void [mscorlib]System.Threading.Monitor::Exit(object)
            nop
            endfinally
EndFinally: nop
            ret
    .try StartTry to EntTry finally handler EndTry to EndFinally

Which is emitted (from both .NET 2.0 and .NET 3.5 Beta 2) for the compilation of the following:

            lock (locker)

            {

                Console.WriteLine(“in lock”);

            }


This means the lock is acquired (call to Monitor.Enter) and a nop (at Hole) is executed before the start of the protected region (try block).  This means there’s an instruction after the acquisition of the lock that isn’t in the try block.  It’s at that point if Thread.Abort were called, the finally block would never get executed and the lock would not be released.  Likely this would result in deadlocks because nothing can ever release that lock now.


If you never use Thread.Abort, this issue doesn’t affect you.  But, it really just provides another good reason why people keep saying you should never use Thread.Abort.


In ‘System.Threading.Thread.Suspend() is obsolete: ‘Thread.Suspend has been deprecated…. I provide an example of cooperatively terminating a thread, if you’re interested in terminating your thread without using Thread.Abort.


Like catching Exception, there is an instance when Thread.Abort can be safely used: when you’re terminating your application.  If you’re terminating your application and you know you have a foreground thread running and it’s not responding, Thread.Abort can safely be used to ensure your application terminates bcause you’re shutting down and not using any existing invariants or locks.

Exception Logging

There is often a requirement for an application to log unhandled (and sometimes “handled”) exceptions.  This logging could occur to a log file, to the Event Log, a logging server, etc.  There’s great reasons to log exceptions but logging exceptions can be fraught with problems.

I’ve have many clients that have been requested to log exceptions and simply threw (pardon the pun) in a call to some logging method in every catch block and added catch blocks around most code that could throw.  This certainly gets the exception logged but introduces at least one problem: if it’s an unhandled exception, when does the exception stop getting logged?  For example, given the following code:

    //…

    internal sealed class MyClass

    {

        private const int NAME_FIELDINDEX = 1;

        private IDAL dal;

        public MyClass ( )

        {

            // TODO: Base from configuration

            dal = MyOtherClass.Create();

        }

        public String GetName ( )

        {

            String name = String.Empty;

            try

            {

                dal.ReadField(NAME_FIELDINDEX, ref name);

            }

            catch (Exception exception)

            {

                Logger.LogException(exception);

                throw;

            }

            return name;

        }

    }

 

    //…

    internal sealed class MyOtherClass : IDAL

    {

        IDataReader dataReader;

        //…

        public static IDAL Create ( )

        {

            // TODO: do something real

            return new MyOtherClass();

        }

 

        // implements IDAL.ReadField<T>(int, ref T)

        public bool ReadField<T> ( int index, ref T value )

        {

            try

            {

                value = (T)dataReader.GetValue(index);

            }

            catch (InvalidCastException invalidCastException)

            {

                Logger.LogLogicError(Resources.GetMessage(Resources.MessageType.LogicError), invalidCastException);

                return false;

            }

            catch (Exception exception)

            {

                Logger.LogException(exception);

                throw;

            }

            return true;

        }

    }

 

    //…

    public static class Program

    {

        public static void Main ( )

        {

            // …

            MyClass myObject = new MyClass();

            String name = myObject.GetName();

            // …

        }

    }


…since no other method should know (or more importantly, rely-upon) the implementation details of another (like whether it logs exceptions or not) and with a notion that low-level exception logging should be implemented, each method is forced to log exceptions.  As a result, in this example, there would be two identical exceptions logged here.  Anyone debugging would have to know to begin researching the problem with the first in a series of identical exceptions; but, this could be a rat-hole because it assumes adjacently logged exceptions are really the same–which isn’t always going to be the case.  But, that’s the minor problem.  What if logging is performed to file and a StackException occurs?  Best case, nothing gets logged and several StackExceptions occur (calling another method in the presence of a StackException doesn’t go far and may just throw another StackException), worst case you’ve conceptually got an infinite loop and the remote possibility it can write the event, filling up the event log.  Same would occur with a file, quickly eating up disk space.  In low disk space situations or where there are policies to restrict the size of the Event Log you run into a denial of service problem (i.e. you’ve blocked all other applications from writing to the event log or to disk).

While the intentions are noble here, the result is not as useful as it was intended to be.  How do you correct this?

This situation can be corrected by mirroring accepted exception handling practices: only catch exceptions you can handle with the exception of a last-chance exception handler.  Since there’s the requirement of logging both unhandled and some handled exceptions; logging of handled exceptions needs to occur when the exception is handled, and the notion of low-level unhandled exception logging should be abandoned and unhandled exceptions would be logged at the last-chance handler.  This change would result in something like:

    //…

    internal sealed class MyClass

    {

        private const int NAME_FIELDINDEX = 1;

        private IDAL dal;

        public MyClass ( )

        {

            // TODO: Base from configuration

            dal = MyOtherClass.Create();

        }

        public String GetName ( )

        {

            String name = String.Empty;

            dal.ReadField(NAME_FIELDINDEX, ref name);

            return name;

        }

    }

 

    //…

    internal sealed class MyOtherClass : IDAL

    {

        IDataReader dataReader;

        //…

        public static IDAL Create ( )

        {

            // TODO: do something real

            return new MyOtherClass();

        }

 

        // implements IDAL.ReadField<T>(int, ref T)

        public bool ReadField<T> ( int index, ref T value )

        {

            try

            {

                value = (T)dataReader.GetValue(index);

            }

            catch (InvalidCastException invalidCastException)

            {

                Logger.LogLogicError(Resources.GetMessage(Resources.MessageType.LogicError), invalidCastException);

                return false;

            }

            return true;

        }

    }

 

    public static class Program

    {

 

        public static void Main ( )

        {

            try

            {

                MyClass myObject = new MyClass();

                String name = myObject.GetName();

                //…

            }

            catch (Exception exception)

            {

                // last-chance handler

                Logger.LogException(exception);

                throw; // optional re-throw

                // terminate

            }

        }

    }


I’ve added a last-change handler in Program.Main, removed the exception handler from MyClass.GetName, and removed the catch(Exception) block from MyOtherClass.GetField<T>. Now you’ve got simpler code (even in this example, more so in the real world), without the potential of duplicate exception logging and denial of service.  And taking into account the performance implications of try blocks, potentially faster code.