0
votes

I'm doing an assignment where we are creating 4 threads that all share some memory. Just want to ask if the design of my monitor looks good, since I'm having a deadlock/ stalling somewhere in my code when I try to cancel all threads.

Previously, I have identified the stalling to threads locking up the mutex when they are cancelled, leaving a thread deadlocking on waiting for the mutex to unlock. I've implemented some changes but still seems to stall when I pipe some data into it using

cat input.txt |./app

However if I read the data directly from the file using getline() then it does not stall and all threads are cancelled.

Currently, the monitor contains the 2 shared lists(these are created using the same pool of nodes), a mutex control access to this list, with 4 condition variables 2 per list.

//shared data
static List *sendList;
static List *receivelist;

//locks
static pthread_cond_t sendListIsFullCondVar = PTHREAD_COND_INITIALIZER;
static pthread_cond_t sendListIsEmptyCondVar = PTHREAD_COND_INITIALIZER;
static pthread_cond_t receiveListIsFullCondVar = PTHREAD_COND_INITIALIZER;
static pthread_cond_t receiveListIsEmptyCondVar = PTHREAD_COND_INITIALIZER;

static pthread_mutex_t listAccessMutex = PTHREAD_MUTEX_INITIALIZER;

The monitor interface consist of an add function and a get function for each list. Each threads are expected to be either adding to the list or getting from the list, but not both.

More specifically, the keyboardThread puts data into the sendlist, sendThread get data from sendlist, receiveThread put data into receivelist, and printThread get data from receivelist.

void Monitor_sendlistAdd(void *item)
{

    pthread_mutex_lock(&listAccessMutex);

    if (List_count(sendList) == MAX_LIST_SIZE)
    {
        pthread_setcancelstate(PTHREAD_CANCEL_ENABLE, NULL);
        pthread_cond_wait(&sendListIsFullCondVar, &listAccessMutex);
    }
    pthread_setcancelstate(PTHREAD_CANCEL_DISABLE, NULL);

    List_prepend(sendList, item);
    pthread_cond_signal(&sendListIsEmptyCondVar);
    pthread_mutex_unlock(&listAccessMutex);

    pthread_setcancelstate(PTHREAD_CANCEL_ENABLE, NULL);
}

void *Monitor_sendlistGet()
{
    pthread_mutex_lock(&listAccessMutex);
    if (List_count(sendList) == 0)
    {
        pthread_setcancelstate(PTHREAD_CANCEL_ENABLE, NULL);
        pthread_cond_wait(&sendListIsEmptyCondVar, &listAccessMutex);
    }
    pthread_setcancelstate(PTHREAD_CANCEL_DISABLE, NULL);

    void *item = List_trim(sendList);

    pthread_cond_signal(&sendListIsFullCondVar);
    pthread_mutex_unlock(&listAccessMutex);
    pthread_setcancelstate(PTHREAD_CANCEL_ENABLE, NULL);
    return item;
}

(I didn't include the interface for receiveList since its is identical.)

I'm changing the cancel states after the if-statement to make sure a thread is not cancelled while locking the mutex, which would stall any process from cancelling if it is waiting for the mutex.

I also giving each thread's a cleaup handler that releases its mutex, again to ensure that no thread cancel without keeping the mutex locked.

void *keyboardThread()
{
    pthread_cleanup_push(Monitor_releaseListAccessMutex, NULL);
    while (1)
    {
     ... some codes
    }
    pthread_cleanup_pop(1);
    return;

So yeah, I'm just completely stumped as to where else could be blocking in my code. The rest of the code are just making connections to sockets to shoot data between ports and mallocs. I have looked into the manual and it seems like mutex_lock is the only function in my code that can block a thread cancel.

1
From a cursory inspection of your code, you seem to be able to cancel threads that have mutex(es) locked, thereby blocking all other threads from ever being able to lock those mutex(es) in the future. FWIW, relying on being able to cancel threads and then continue processing in any form is a REALLY bad idea. It makes keeping track of all but the most trivial of states an all but intractable problem.Andrew Henle

1 Answers

1
votes

Do not use thread cancellation if you can possibly help it. It has deep problems associated with it.

To break threads out of a pthread_cond_wait(), use pthread_cond_signal() or pthread_cond_broadcast(). Or use pthread_cond_timedwait() in the first place and just let the timeout expire.

"But, wait!" I imagine you saying, "Then my threads will just proceed as if they had been signaled normally!" And there's the rub: your threads need to be able to handle spurious returns from their waits anyway, as those can and do happen. They must check before waiting whether they need to wait at all, and then they must check again, and potentially wait again, after returning from their wait.

What you can do, then, is add a shared flag that informs your thread(s) that they should abort instead of proceeding normally, and have them check that it is not set as one of the conditions for waiting. If it is set before any iteration of the wait loop then the thread should take whatever action is appropriate, such as (releasing all its locked mutexes and) terminating.

You remark:

I also giving each thread's a cleaup handler that releases its mutex

That's probably a bad idea, and it may be directly contributing to your problem. Threads must not attempt to unlock a mutex that they do not hold locked, so for your cleanup handlers to work correctly, you would need to track which mutexes each thread currently has locked, in a form that the cleanup handlers can act upon. It's conceivable that you could do that, but at best it would be messy, and probably fragile, and it might well carry its own synchronization issues. This is among the problems attending use of thread cancellation.