Thread design for sending data to multiple servers

In the following code, checkServerExists function checks if the server exists in the vector.
If it does, then the new message is directly pushed in the vector, otherwise a new thread is created and then the message is pushed in the vector.

I need to know whether it makes sense to write the code, as I have done.

```
#include
#include
#include
#include
#include
#include

pthread_mutex_t demoMutex = PTHREAD_MUTEX_INITIALIZER;
pthread_cond_t conditionVariable = PTHREAD_COND_INITIALIZER;
unsigned int condition = 0;

struct serverInfo
{
unsigned int serverId;
pthread_t threadId;
std :: vector queue;
};
std :: vector serverInfoVector;

void printHello (void threadId)
{
pthread_t my_tid = (pthread_t )threadId;

pthread_mutex_lock (&demoMutex);

while (condition == 0)
pthread_cond_wait (&conditionVariable, &demoMutex);

unsigned int i = 0;
char found = false;

if (serverInfoVector.size () > 0)
{
while ((i 0)
{
while ((i < serverInfoVector.size ()) && (found == false))
{
if (serverNumber == serverInfoVector [i].serverId)
{
found = true;
break;
}
else
i++;
}
}

if (found == false)
{
// This server doesn't exist, so create a thread for it, create a queue for it, push the message in the corresponding queue.
// Push the server number in the serverNumberArray.

// Create a thread for it.
pthread_t newThread;
int returnValue;
if ((returnValue = pthread_create (&newThread,
NULL,
printHello,
(void*) &newThread)) != 0)
{
printf ("\nerror: pthread_create failed with error number %d", returnValue);
}
printf ("\nIn

Any function that is a callback from a C library must use the C ABI.

If your code is C++ (as this is) then you MUST declare the callback function appropriately to make sure the compiler gets the correct ABI:

extern "C" void* printHello (void* threadId);

Correct usage of the signal:

while (condition == 0)
    pthread_cond_wait (&conditionVariable, &demoMutex);

But you never decrement the variable condition. Thus after the first thread is created it will always be true. Thus I would do this:

while (condition == 0)
{
    // Note I always enclose the statement in {} what happens if pthread_cond_wait()
    //      had been a macro? You can never trust all third party libraries so it
    //      is better to be safe than struggle to find it with debugger.
    pthread_cond_wait (&conditionVariable, &demoMutex);
}
// Once you know it is your decrement.
--condition;

Normally you use pthread_exit(); if you are killing the thread deep inside the code. If it is directly inside the callback function then you can just return NULL. Also you must guard against exceptions propagating out of the callback (if they escape the application will terminate (usally: Its actually undefined what happens but I have found it usally terminates the whole application (not just the thread))).

void * printHello (void* threadId)
{
    void*  result = NULL;
    try
    {
        result = // Your code here
    }
    catch(std::exception const& e) // Optional
    {
        // Log Exception
        std::cerr << "Exception: " << e.what() << "\n";
    }
    catch(...) // MUST have this one.
    {
        // Log Exception
        std::cerr << "Exception: Unknown(...)\n";
    }
    return result;
}

You use lock/unlock pairs for the mutex. This is not exception safe. So I would create an object that will do the lock in the constructor and unlock in the destructor then use this to lock your mutexs. This will make your code more exception safe.

class MutexLocker
{
    pthread_mutex_t&  mutex;
    MutextLocker(pthread_mutex_t& mutex)
        : mutex(mutex)
    {
        pthread_mutex_lock(&mutex);
    }
    ~MutexLocker()
    {
        pthread_mutex_unlock(&mutex);
    }
};

Your code may seem threeaded but it is still inherently serial. This is because each thread maintain the mutex for its whole life this it prevents new threads from being created. You should unlock the mutext as soon as you have finished initialization (or just use the lock when accessing global objects that may be mutated by the master thread).

Also on thread startup you spend a lot of time finding your queue. Why not get the master thread to pass a pointer to the threads queue (via the void* parameter). Note the value you pass (the pthread_t pointer is not guranteed to be correctly filled out when the thread starts (it is only guranteed correct after pthread_create() returns which may be after the thread has started so it is not a good object to pass:

What I would have done is:

struct serverInfo
{
    unsigned int                  serverId;
    pthread_t                     threadId;

    // Make the queue a pointer
    // So even when this object is copied the queue is unaffected.
    // May want to use a smart pointer or something (needs slightly more thought).
    std::vector*     queue;
};

 std::auto_ptr> queue = new std::vector();
 // Pass a pointer to the queue to the thread.
 // Now the thread does not need to know or care about serverInfoVector
 // Which is good because this is being mutated by other people.
 pthread_create(&newThread, NULL, printHello, queue.get()); // Add error code

 if (/* Everything OK */)
 {
     serverInfoVector.push_back(serverInfo(id, newThread, queue.release());
 }

Minor Comments:

Add a constructor to you serverInfo class.

struct serverInfo
{
    unsigned int                  serverId;
    pthread_t                     threadId;
    std :: vector  queue;

    // Add this:
    serverInfo(unsigned int serverId, pthread_t threadId, std::string const& message)
        : serverId(serverId),
        , threadId(threadId)
    {
        queue.push_back(message);
    }
};

This will allow more readable code below.

AS a personal preference I allways have my types begin with an uppercase letter. This helps me distinguish types from data in an easy way that is not intrusive. This becomes more important when you start playing with templates.

Try and limit global variables.

std :: vector  serverInfoVector;

Easier to create these at the top level and then pass references to them around. If you have underlying functions that depend on global state it becomes really hard to test the code. In the long run it is best to pass everything as parameters then you can see exactly what the function depends on.

This is a horrible variable name. Try searching for all occurrences of the variable i to see where it is. I understand it short for index. Why not use index or my favorite `l

StackExchange Code Review Q#9739, answer score: 6