Unit Testing Refcounted Critical Section Class

Question

Hello all :)

I'm looking at a simple class I have to manage critical sections and locks, and I'd like to cover this with test cases. Does this make sense, and how would one go about doing it? It's difficult because the only way to verify the class works is to setup very complicated threading scenarios, and even then there's not a good way to test for a leak of a Critical Section in Win32. Is there a more direct way to make sure it's working correctly?

Here's the code:

CriticalSection.hpp:

#pragma once
#include <windows.h>
#include <boost/shared_ptr.hpp>

namespace WindowsAPI { namespace Threading {

    class CriticalSectionImpl;
    class CriticalLock;
    class CriticalAttemptedLock;

    class CriticalSection
    {
        friend class CriticalLock;
        friend class CriticalAttemptedLock;
        boost::shared_ptr<CriticalSectionImpl> impl;
        void Enter();
        bool TryEnter();
        void Leave();
    public:
        CriticalSection();
    };

    class CriticalLock
    {
        CriticalSection &ref;
    public:
        CriticalLock(CriticalSection& sectionToLock) : ref(sectionToLock) { ref.Enter(); };
        ~CriticalLock() { ref.Leave(); };
    };

    class CriticalAttemptedLock
    {
        CriticalSection &ref;
        bool valid;
    public:
        CriticalAttemptedLock(CriticalSection& sectionToLock) : ref(sectionToLock), valid(ref.TryEnter()) {};
        bool LockHeld() { return valid; };
        ~CriticalAttemptedLock() { if (valid) ref.Leave(); };
    };

}}

CriticalSection.cpp:

#include "CriticalSection.hpp"

namespace WindowsAPI { namespace Threading {

class CriticalSectionImpl
{
    friend class CriticalSection;
    CRITICAL_SECTION sectionStructure;
    CriticalSectionImpl() { InitializeCriticalSection(&sectionStructure); };
    void Enter() { EnterCriticalSection(&sectionStructure); };
    bool TryEnter() { if (TryEnterCriticalSection(&sectionStructure)) return true; else return false; };
    void Leave() { LeaveCriticalSection(&sectionStructure); };
public:
    ~CriticalSectionImpl() { DeleteCriticalSection(&sectionStructure); };
};

void CriticalSection::Enter() { impl->Enter(); };
bool CriticalSection::TryEnter() { return impl->TryEnter(); };
void CriticalSection::Leave() { impl->Leave(); };
CriticalSection::CriticalSection() : impl(new CriticalSectionImpl) {} ;

}}

Answer 1

Here are three options and personally I favour the last one...

• You could create a 'critical section factory' interface that can be passed to your constructor. This would have functions that wrapped the API level functions that you need to use. You could then mock this interface up and pass the mock to the code when under test and you can be sure that the right API functions are called. You would, generally, also have a constructor that didn't take this interface and that instead initialised itself with a static instance of the factory that called directly to the API. Normal creation of the objects wouldn't be affected (as you have them use a default implementation) but you can instrument when under test. This is the standard dependency injection route and results in you being able to parameterise from above. The downside of all this is that you have a layer of indirection and you need to store a pointer to the factory in each instance (so you're probably losing out in both space and time).
• Alternatively you could try and mock the API out from underneath... A long time ago I looked into testing this kind of low level API usage with API hooking; the idea being that if I hooked the actual Win32 API calls I could develop a 'mock API layer' which would be used in the same way as more normal Mock Objects but would rely on "parameterise from below" rather than parameterise from above. Whilst this worked and I got quite a long way into the project, it was very complex to ensure that you were only mocking the code under test. The good thing about this approach was that I could cause the API calls to fail under controlled conditions in my test; this allowed me to test failure paths which were otherwise VERY difficult to exercise.
• The third approach is to accept that some code is not testable with reasonable resources and that dependency injection isn't always suitable. Make the code as simple as you can, eyeball it, write tests for the bits that you can and move on. This is what I tend to do in situations like this.

However....

I'm dubious of your design choice. Firstly there's too much going on in the class (IMHO). The reference counting and the locking are orthogonal. I'd split them apart so that I had a simple class that did critical section management and then built on it I found I really needed reference counting... Secondly there's the reference counting and the design of your lock functions; rather than returning an object that releases the lock in its dtor why not simply have an object that you create on the stack to create a scoped lock. This would remove much of the complexity. In fact you could end up with a critical section class that's as simple as this:

CCriticalSection::CCriticalSection()
{
   ::InitializeCriticalSection(&m_crit);
}

CCriticalSection::~CCriticalSection()
{
   ::DeleteCriticalSection(&m_crit);
}

#if(_WIN32_WINNT >= 0x0400)
bool CCriticalSection::TryEnter()
{
   return ToBool(::TryEnterCriticalSection(&m_crit));
}
#endif

void CCriticalSection::Enter()
{
   ::EnterCriticalSection(&m_crit);
}

void CCriticalSection::Leave()
{
   ::LeaveCriticalSection(&m_crit);
}

Which fits with my idea of this kind of code being simple enough to eyeball rather than introducing complex testing ...

You could then have a scoped locking class such as:

CCriticalSection::Owner::Owner(
   ICriticalSection &crit)
   : m_crit(crit)
{
   m_crit.Enter();
}

CCriticalSection::Owner::~Owner()
{
   m_crit.Leave();
}

You'd use it like this

void MyClass::DoThing()
{
   ICriticalSection::Owner lock(m_criticalSection);

   // We're locked whilst 'lock' is in scope...
}

Of course my code isn't using TryEnter() or doing anything complex but there's nothing to stop your simple RAII classes from doing more; though, IMHO, I think TryEnter() is actually required VERY rarely.