Code inspecting Stroustrup (TC++PL4) (2nd issue)

Since I learned C++ reading one of the first edition of TC++PL and even if I'm programming using C++ since 2000 or so I'm reading the new TC++PL4 carefully as if this language is totally new to me. It seems my last post about an error found on this book will be not the unique and here we are again.
In 5.3.4.1 he introduces conditions and how two threads can interact each other communicating using events, it's presented the classical producer / consumer interaction exchanging Messages trough a queue, and this is the poor implementation proposed:

  class Message {
    // ...
  };

  queue mqueue;
  condition_variable mcond;
  mutex mmutex;

  void consumer() 
  {
     while(true) {
        unique_lock lck{mmutex};
        while (mcond.wait(lck)) /* do nothing */;
        
        auto m = mqueue.front();
        mqueue.pop();
        lck.unlock();
        // ... process m...
     }
  }

  void producer() 
  {
     while(true) {
        Message m;
        // ... fill the message ...
        unique_lock lck{mmutex};
        mqueue.push(m);
        mcond.notify_one();
     }
  }

This implementation is affected by at least three issues:

• Unless very lucky the queue will grow indefinitely: that's because basically the consumer will wait at each cycle even if queue contains something, at the same time it has a chance (I repeat a "chance") to exit from the  condition_variable::wait() only each time the producer puts something in the queue.
• The consumer can miss the condition_variable::notify_one event, indeed if the producer does the notify_one() but the other thread hasn't yet executed the wait() the consumer will block for no reason
• The producer holds the unique_lock for more time than needed, the mutex has to only protect the queue not the condition as well

Let see how those producer / consumer should have been implemented:

  void consumer() 
  {
     while(true) {
        unique_lock lck{mmutex};
        while (mqueue.empty()) { // the empty condition has to be recheck indeed the thread
                                               // can get sporadic wakeup without any thread doing a notify
          mcond.wait(lck);
        }
        auto m = mqueue.front();
        mqueue.pop();
        lck.unlock();
        // ... process m...
     }
  }

  void producer() 
  {
     while(true) {
        Message m;
        // ... fill the message ...
        {
           unique_lock lck{mmutex};
           mqueue.push(m);
        }  // This extra scope is in here to release the mmutex asap
         mcond.notify_one();
     }
  }

In my opinion this should have been the version of the producer/consumer in TC++PL4, as you can see
with a simple extra scope and the right while(...) the issues reported in the bullets are solved.

There is another problem, I have to admit that this issue most of the times is a minore one:

• The producer can issue notify_one() even if not needed, and this can be a performance issue 

to address it the producer has to "forecast" if the consumer can be in a blocked status, and this can happen only if after having acquired the mmutex the queue is empty, this is the final version of producer:

   void producer() 
  {
     while(true) {
        Message m;
        bool notifyIsNeeded = false;
        // ... fill the message ...
        {
           unique_lock lck{mmutex};
           if (mqueue.empty()) {
             notifyIsNeeded  = true;
           }
           mqueue.push(m);
        }  // This extra scope is in here to release the mmutex asap
        if ( notifyIsNeeded ) {
           mcond.notify_one();
        }
     }
  }

Writing correct code is not easy and writing correct multi-threaded is damn hard.

Code inspecting Stroustrup (TC++PL4)

TC++PL4 is now on my desk and carefully reading it I have to say that even Stroustrup makes stupid mistakes in his classes implementations.

He illustrates a typical Vector implementation (pag. 73):

    class Vector {
    private:
        double* elem;
        int sz;
    public:
        Vector(int s);
        ~Vector() { delete [] elem; }
        
        Vector(const Vector& a);
        Vector& operator=(const Vector& a);

        double& operator[](int i);
        const double& operator[](int i) const;

        int size() const;
    };

and given the fact this class needs a copy constructor implemented he "implements" it (pag. 74):

     Vector::Vector(const Vector& a)
         :elem{new double[sz]},
          sz{a.sz}
     {
         for (int i = 0; i != sz; ++i)
         elem[i] = a.elem[i];
     }

as you can see the elem vector is built with a size retrieved from a not yet initialized variable, being it defined at page 74 I had my last hope flipping the page and checking at page 73 if "int sz" was declared before "double* elem" but it was not the case.

I'm sad.