Simple assert for ordered non re-entrant calling?

Question

I have two functions:

void prepare() and void finish() that will be called sequentially like:

prepare();
<do something>;
finish(); 
... 
prepare(); 
<do something>; 
finish();

I want to make a simple assertion to simply test that they are in fact being called this way and that they aren't being called concurrently or out-of-order in the application.

This application is a single-threaded application. This is a simple development/testing sanity check to make sure that these functions are being called in-order and that for whatever reason, they aren't being called concurrently. Furthermore, these assertions/sanity checks should be omitted from production code as performance is crucial!

would a simple assert() like this work best?

int test = 0;

void prepare() {
   assert(++test == 1);

   .
   .
   .
}

void finish() {
    assert(--test == 0);

    .
    .
    .
}

Answer 1

There's a race condition here: two concurrent instances of prepare might take the value of test at the same time, then both increment it in a register to both obtain 1, then do the comparison to get true.

Making it volatile is not going to help. Instead, you should put a mutex on test, like so:

boost::mutex mtx;
int test = 0;

void prepare()
{
    boost::mutex::scoped_try_lock lock(&mtx);
    assert(lock.owns_lock());
    assert(test++ == 0);
    // ...
}

void finish()
{
    boost::mutex::scoped_try_lock lock(&mtx);
    assert(lock.owns_lock());
    assert(--test == 0);
}

Answer 2

You might want to change

int test = 0;

to

#ifndef NDEBUG
int test = 0;
#endif

to satisfy your requirement that "any code relating to this test should be omitted from production".

Answer 3

you probably want:

int test = 0;

void prepare() {
    // enter critical section
    assert(test++ == 0);

    .
    .
    .
    // leave critical section 
}

void finish() {
    // enter critical section
    assert(--test == 0);

    .
    .
    .
    // leave critical section
}

Answer 4

Your code is OK, unless you need to allow nesting prepare and finish calls.

If nesting is not allowed, you could use a bool instead of an int:

bool locked = false;;

void prepare() {
    assert( ! locked );
    locked = true;
    ...
}

void finish() {
    assert( locked );
    locked = false;
    ...
}

Answer 5

Since you're using C++, why not use RAII? You'd still need to check for re-entrant use, but RAII simplifies things considerably. Combined with larsmans' mutex and Raedwald's elimination in NDEBUG:

struct Frobber {
  Frobber() {
    assert(mtx.try_lock());
#ifndef NDEBUG
    try {  // in case prepare throws
#endif
      prepare();
#ifndef NDEBUG
    }
    catch (...) {
      mtx.unlock();
      throw;
    }
#endif
  }

  void something();
  // And the other actions that can be performed between preparation and finishing.

  ~Frobber() {
    finish();
#ifndef NDEBUG
    mtx.unlock();
#endif
  }

private:
#ifndef NDEBUG
  static boost::mutex mtx;
#endif

  Frobber(Frobber const&);  // not defined; 0x: = delete
  Frobber& operator=(Frobber const&);  // not defined; 0x: = delete
};
#ifndef NDEBUG
boost::mutex Frobber::mtx;
#endif

void example() {
  Frobber blah;  // instead of prepare()
  blah.something();
  // implicit finish()
}

Inside example, you simply cannot do something without first preparing, and finishing will always happen, even if an exception is thrown.

Side note about NDEBUG: if you use it this way, make sure it is either always defined or always undefined in all translation units, as opposed to how it's used for assert (allowing it to be defined and undefined at various points).

Answer 6

If you put <do something>; into a class you can reduce the need for a check at all:

Just have the constructor call prepare and the destructor call finish. Then it's automatically enforced that they're called appropriately.

Note that concurrency and nesting issues still apply: If you want to prevent nesting then you'd still need some sort of global state (static class member?) to keep track of that, and if it's used in more than one thread access to that counter would need to be mutex protected.

Also note that you could also make private operator new/delete to prevent someone from creating one on the heap and not destroying it.