Archived

This forum has been archived. Please start a new discussion on GitHub.

Queue example and IceUtil::Monitor

Hi ZeroC guys,

On page 643 and 644 of the manual it shows an example of a "Queue" class implemented with Monitor locking. I am trying to create a similar class for managing a TraceHandler that holds a std::queue<std::string>.

I am a little bit confused on how the function "T get()" will exit correctly at shutdown? In particular:
T get() {
   IceUtil::Monitor<IceUtil::Mutex>::Lock lock(*this);
   while (_q.size() == 0)
   wait();
   T item = _q.front(); // Are these lines going to be called before exiting?
   _q.pop_front(); //                     "
   return item; //                          "
}

What sort of automagic needs to be done for the thread that called that method to exit out of the function correctly without hitting the last three lines of code? Or is the thread that runs that section of code destroyed in some special way that basically just stops the code from running altogether (such as when the Thread object is deleted - either explitcitly or when the ThreadPtr object goes out of scope)?

I should note that in my code the get() function has essentially been implemented as the run() interface of the IceUtil::Thread class. And also I have an extra loop conceptually around the get() function that more or less makes it run in an infinite loop.

Was this example fully contrived or do I need to place some extra test logic for when wait() returns?

I should also mention that my TraceHandler object implements this get()-like method and "has a" Monitor<Mutex> but not "is a" Monitor<Mutex>. Since it already "is a" Thread I didn't want TraceHandler to inherit multiply, though I suppose it could? Would this pattern break any of this sensitive logic here?


Regards,
Ryan

Comments

  • When I wrote this example, I wrote it to illustrate the principles of locking and how to use a monitor, not to show how to build real-life applications. As you discovered, you need more than principles for real applications :)

    Basically, to get this function to terminate cleanly, you need to have some other test criterion. For example, when the application decides that it's time to exit, it can set some sort of flag. Something like:
    void shutdown {
        IceUtil::Monitor<IceUtil::Mutex>::Lock lock(*this);
        _done = true;
        notifyAll();
    }
    

    Then modify the get() function along the following lines:
    T get() {
        IceUtil::Monitor<IceUtil::Mutex>::Lock lock(*this);
        while (_q.size() == 0 && !_done)
            wait();
        if (_done)
            return; // Well, almost...
        // Dequeue item and return it.
    }
    

    That (almost) captures what needs to happen. The problem we have here is that, as written, get() does not have a way to indicate whether it returned a valid item or whether it returned because the _done flag was set. You have the usual choice of options for dealing with this:
    • Have get() return two values, a boolean and an item, and use the boolean to indicate whether the item is valid.
    • Have get() return a dummy item that indicates "no value", such as null or some other dedicated sentinval value.
    • Throw a ShutdownException or similar that indicates to the caller that the function returned because the _done flag was set.
    Which one you use depends on your application and whether there is a convenient dummy value available. Personally, I would probably go with the last option. Even though shutdown is an expected occurence (which argues against using an exception), it happens rarely enough (once per program run) that an exception may just be appropriate. And an exception has the advantage that you do not have to worry about a dummy return value, and that an exception cannot be accidentally ignored by the caller.

    Cheers,

    Michi.
  • Ahh... thanks Michi! That makes more sense to me. I figured I needed to do a little more there to get that to work (automagic just never seems to work right in software :rolleyes: ). I appreciate the response.

    Regards,
    Ryan
  • Strange shutdown bug

    Hi Michi (et al),

    So I think I have set up my TraceHandler somewhat correctly (well obviously not completely correctly!), but I am still having problems shutting down gracefully.

    I have also created a TraceHandlerManager whose sole job it is to start and terminate the Thread object. However, there is a mysterious bug whereby if I place a Sleep(...) statement in the destructor of the manager between 2 seamingly benign calls then it will shutdown gracefully. Otherwise I stall everytime? The code is too long to put here (so I will attach the rest) but here is the destructor of the TraceHandlerManager:
    TraceHandlerManager::~TraceHandlerManager()
    {
       // DON'T MOVE THE ORDER OF ANY OF THESE STATEMENTS
       // OR SUFFER THE CONSEQUENCES!!!
       IceUtil::ThreadControl theControl = TraceHandler::instancePtr()->getThreadControl();
       // ????????????????????????????????????????????????????????????????
       // ????????????????????????????????????????????????????????????????
       Sleep(1); // IF I TAKE THIS SLEEP OUT I WON'T SHUTDOWN CORRECTLY???
       // ????????????????????????????????????????????????????????????????
       // ????????????????????????????????????????????????????????????????
       TraceHandler::instancePtr()->stop(); // Shutdown the TraceHandler Loop
       theControl.join(); // Wait for the TraceHandler Thread to exit.
       TraceHandler::destroy(); // Now kill it!
    }
    

    If anyone can take a look at this code that would be fantastic. Also if anyone wants to use this code I give it away freely. It essentially is a TraceHandler where I guarantee that messages will not get interleaved even in a fully threaded application (modeled loosely off of Michi's example in Chapter 28.9 of the ICE manual). Additionally, you may redirect the output of the TraceHandler to an ostream/ofstream of your choice at any time.

    If only it didn't have that automagic statement, would I be more comfortable with it!!!

    I am having this little bugger in Windows, using .NET 2003 and ICE 2.0.

    Regards,
    Ryan
  • mes
    mes California
    Hi Ryan,

    I've looked at the code and I have a couple of comments.

    First of all, TraceHandler derives from IceUtil::Thread, which derives from IceUtil::Shared. As such, it's not a good idea to explicitly delete a TraceManager instance. Rather, instances of IceUtil::Shared subclasses are meant to be managed by smart pointers (i.e., IceUtil::Handle).

    In order to avoid any potential errors in this area, I suggest reorganizing your class such that the thread is a separate object. The way we usually do this is by having a private nested subclass of IceUtil::Thread whose run() method simply invokes a method of the enclosing class.

    Off the top of my head:
    class TraceHandler : ...
    {
    public:
        TraceHandler()
        {
            _thread = new TraceHandlerThread(this);
        }
    
        void start() { _thread->start(); }
        void stop() { ... }
        void waitForStop()
        {
             _thread->getThreadControl().join();
        }
    
    private:
        void run() { ... }
    
        class TraceHandlerThread : public IceUtil::Thread
        {
         public:
            TraceHandlerThread(TraceHandler* th) : _th(th) {}
            virtual void run() { _th->run(); }
         private:
            TraceHandler* _th;
        };
        friend class TraceHandlerThread;
        IceUtil::ThreadPtr _thread;
    };
    
    Then ~TraceHandlerManager becomes
       TraceHandler::instancePtr()->stop(); // Shutdown the TraceHandler Loop
       TraceHandler::instancePtr()->waitForStop();
       TraceHandler::destroy(); // Now kill it!
    
    I can't promise this will address the hang you're experiencing, but it might make it easier to isolate the cause if not.

    Take care,
    - Mark
  • Hi Mark,

    Thanks for your suggestions. I was also a little concerned with my hacked TraceHandler using multiplte inheritance (that being both a Thread and a Singleton<>). So your method may kill 2 birds with one stone. I will give it a go.

    Thanks for your help,
    Ryan
  • Thread join problem is persistent bugger!

    Hi Mark, Michi, et al,

    Mark, thanks again for your tips from yesterday. I integrated your suggestions last night and was still able to reproduce the error. Though I was able to Ctrl-C singal the process to kill it much more easily which was definitely nice. But the join problem still persists. At first I thought I had some weird race-condition especially given the fact that I was using 2 mutexes. One to protect the input queue and one to protect the output stream (and redirect it). So I simplified the model and just used one Monitor<RecMutex> to do the job figuring I was really just doing software gymnastics anyway. But again the problem still existed.
    Finally, I read that I could implement a non-blocking "join" by calling the isAlive() function on the ThreadControl. This turns out to be the only way I can reliably and gracefully shutdown (Ran it out 1000 times anyway without any problems). In particular I merged together the stop() and waitForStop() (as contributed by Mark) functionality together which looks like this
    void TraceHandler::waitForStop()
    {
       while(myThread->getThreadControl().isAlive())
       {
          // Extra brackets to ensure that theQueueLock goes out
          // of scope no matter the compiler.
          {
             IceUtil::Monitor<IceUtil::RecMutex>::Lock theQueueLock(myQueueLockMonitor); //Lock the queue before writing to it!
             isAlive = false;
             myQueueLockMonitor.notifyAll();
          }
       }
       // This below apparently does not work 100% of the time?
       // ?????????????????????????????????????????????????????
       // ?????????????????????????????????????????????????????
       // ?????????????????????????????????????????????????????
       //{
       //   std::cout << "Attempting to send shutdown signal!\n";
       //   IceUtil::Monitor<IceUtil::RecMutex>::Lock theQueueLock(myQueueLockMonitor); //Lock the queue before writing to it!
       //   isAlive = false;
       //   myQueueLockMonitor.notifyAll();
       //   std::cout << "Sent shutdown notification!\n";
       //}
       //myThread->getThreadControl().join();
       // ?????????????????????????????????????????????????????
       // ?????????????????????????????????????????????????????
       // ?????????????????????????????????????????????????????
    }
    



    The block of code that is commented out does not work. My theory is that the handling thread which is looping in the run() method (below) is never receiving a notification (or not receiving it at the right time while "isAlive" is actually false). I tried making isAlive "volatile" to see if that would change anything and I guess it doesn't.
    // Virtual method as called by the Thread::start function
    void TraceHandler::run()
    {
       IceUtil::Monitor<IceUtil::RecMutex>::Lock theQueueLock(myQueueLockMonitor);
       isAlive = true;
       while(isAlive) // while theHandler is alive
       {
          while(isAlive && myReportQueue.empty())
          {
             myQueueLockMonitor.wait();
          }
          while(!myReportQueue.empty())
          {
             std::string theMessage = myReportQueue.front();
             myReportQueue.pop();
             writeOut(theMessage);
          }
       }
    }
    

    Anyway, I have a workaround. But I still am curious as to why the former method that I implemented does not work. There is only one thread waiting for notifications and as far as I can tell from my run() method once it has received this notification "isAlive" has to be false and therefore should run to completion. I say "isAlive" should theoretically be false because for one, in waitForStop() I hold the mutex while I write isAlive to false and for two, I do get a print out from the lines:
          std::cout << "Attempting to send shutdown signal!\n";
         std::cout << "Sent shutdown notification!\n";
    

    So for now I have a solution that works but it is a hack I guess. And if I am doing something wrong I would still like to know why?

    Stumped in Rhode Island,
    Ryan

    P.S. Again I am attaching the code if anybody wants a TraceHandler (sketchy as it might be.:( )