Only one thread gets created despite the attempt to create two

Question

#include <pthread.h>
#include <stdio.h>
#include <unistd.h>
#include <vector>
#include <string>
#include <iostream>

FILE*            fp;
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<std::string> queue;
};
std::vector<serverInfo> 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 <= serverInfoVector.size()) && (found == false)) {
          if (*my_tid == serverInfoVector[i].threadId) {
             found = true;
             break;
          }
          else
             i++;
       }
    }

    while (!serverInfoVector[i].queue.empty()) {
       std::cout << "\nThread: " << pthread_self() << ", poped from queue: " << serverInfoVector[i].queue.front();
       serverInfoVector[i].queue.pop_back();
    }

    pthread_mutex_unlock(&demoMutex);
    pthread_exit(NULL);
}

void checkServerExists(unsigned int serverNumber, std::string message)
{
    unsigned int i = 0;
    char found = false;

    pthread_mutex_lock(&demoMutex);

    if (serverInfoVector.size () > 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 checkServerExists()`: thread id %ld\n", newThread);

       // Push the message in its queue.
       serverInfo obj;
       obj.serverId = serverNumber;
       obj.threadId = newThread;
       obj.queue.push_back(message);
       serverInfoVector.push_back(obj);

       condition++;
       pthread_cond_signal(&conditionVariable);
       pthread_mutex_unlock(&demoMutex);

       for (unsigned int i = 0; i < serverInfoVector.size(); i++)
          pthread_join(serverInfoVector[i].threadId, NULL);
    }
    else {
       // This server exists, so lookup its thread and queue, push the message in the corresponding queue.
       printf("\nIn else ()`: thread id %ld\n", serverInfoVector[i].threadId);
       serverInfoVector[i].queue.push_back(message);

       condition++;
       pthread_cond_signal(&conditionVariable);
       pthread_mutex_unlock(&demoMutex);

       for (unsigned int i = 0; i < serverInfoVector.size(); i++)
          pthread_join(serverInfoVector[i].threadId, NULL);
    }
}

int main()
{
   fp = fopen("xyz", "w");

   checkServerExists(1, "anisha");
   checkServerExists(2, "kaul");
   checkServerExists(1, "sanjeev");
   checkServerExists(2, "sharma");
}

Output:

In checkServerExists ()`: thread id 140233482061584

Thread: 140233482061584, poped from queue: anisha
In checkServerExists ()`: thread id 140233482061584

In else ()`: thread id 140233482061584

In else ()`: thread id 140233482061584

The problem is that it seems that only one thread is getting created! I have called the function checkServerExists 4 times in main() and 2 times with different serverID, so two threads should be created?

What am I missing?

Answer 1

EDITED: the real problem is that the threads terminate and are joined as soon as they are created, as pointed out by hmjd. I'm leaving this, rather than deleting it, because the following are also problems.

I see two creations of a new thread in the output you post: "In checkServerExists" is only output if you create a new thread. I also see undefined behavior in the printf: newThread has type pthread_t, which can be anything the system wants it to be, and is likely something other than a long, which is what is required by the format you are passing to printf. There is, as far as I know, no way of (portably) outputting a pthread_t (other than a hex-dump of its bytes); the values you display as thread id's don't mean anything. Also, you can't compare pthread_t using ==, you need to use pthread_equal. (On at least one platform I've used, pthread_t was a struct.)

There are a number of other strange things with your code. Why declare found with type char, rather than type bool, for example. And why found == false, rather than !found. And why the break; in the loop, since you have the condition in the loop control variable. A much more idiomatic form of the start of checkServerExists would be:

for ( std::vector<ServerInfo>::iterator current = serverInfoVector.begin();
        current != serverInfoVector.end() && current->serverId != serverNumber;
        ++ current ) {
}
if ( current == serverInfoVector.end() ) {
    //  not found...
} else {
    //  found...
}

Assuming you didn't create a predicte object for the lookup, and just use std::find.

Answer 2

I am unsure if this is contributing to the behaviour, but the following is an error:

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

serverInfoVector[i] will be accessing one too many due to the <= condition in the if. Change to:

while ((i < serverInfoVector.size ()) && (found == false))

EDIT:

I think this is the problem: when checkServerExists() is called, it seems to wait for the thread that it started to complete:

for (unsigned int i = 0; i < serverInfoVector.size(); i++)
    pthread_join(serverInfoVector[i].threadId, NULL);

This means that thread id 140233482061584 is no longer in use and is available again to be associated with a new thread. When the next call to checkServerExists() is made the thread id is reused, giving the impression that only one thread was started.

EDIT 2:

As pointed out by Schwarz this is incorrect:

if (*my_tid == serverInfoVector[i].threadId) {

you need to use pthread_equal() to compare two pthread_t. Change to:

if (pthread_equal(*my_tid, serverInfoVector[i].threadId)) {

or alternatively pass the serverId as the argument to the thread.

Answer 3

      if (*my_tid == serverInfoVector[i].threadId) {

You cannot compare pthread_ts that way. This is a C interface, not a C++ interface. So there's no operator overloading to make this comparison work sensibly. It's wrong for the same reason this is wrong:

    const char *foo="foo";
    if(foo == "foo") ...

You have to use a sensible comparison function. In my example, strcmp. In your code, pthread_equal.

Also, after you pthread_join a thread, its pthread_t is no longer valid. You must not pass it to any pthread_* function again. That's as bad as dereferencing a pointer after passing it to free.

(You may want to fix some all of the bugs reported in this thread and post a new question with your updated code and a description of any issues you still have with it.)

Answer 4

I can see why it is failing to work the way you expect. Your server side is using the unique identifier passed in from main to determine what its thread id should be, but the thread function themselves is using the thread id.

The first worker thread you created terminated, and the second one is being created with the same id as the first, as this id is now available.

The main thread is putting the string onto the queue of the second element in the vector but your thread is picking up the first element of the vector because it has a matching thread id.

All the things they previous posters have said should also be considered, by the way. Just that it is not those that produce your behaviour.

void* printHello(void* serverptr) 
{ 
    serverInfo * info = static_cast< serverInfo * >(serverptr);

  // look through the queue
}

Change the collection type to std::list or std::deque so it will not invalidate the pointer if you subsequently do a push_back whilst the thread is processing it.

In checkServerExists, pass the address of serverInfo into the thread function rather than the address of the thread_id.

You might also "index" your collection with a map from int to serverInfo* or to the list iterator if you use a list. You should not use std::map<int, serverInfo> though because it might invalidate your pointers if you add new entries to the map.

Incidentally, your worker thread appears to terminate too early, because when you send the later info to the old ids, the threads have already gone.

The queue being empty should not be the condition on which to terminate the thread and you should use some other means.

Incidentally, whilst it is thread-safe, your mutex is locked for so long that you are not really going to achieve any decent performance by using multiple threads here.