pthread does not seem to use updated global data value

Question

I am new to threads. I want to make two threads xthread prints 'X'; and ythread prints 'Z'; continuously until the user inserts 'C' or 'c' at stdin. I have made use of select to check if there is any userinput. If there is a userinput I use scanf to obtain it in read and do the comparison.

I have kept read as global. [Is there any other way of sharing non-global data between threads? ] . I have assumed that, when the user enters 'c' at stdin the thread which is currently running reads it and stores it in read and breaks out. I have used the flag read_input to indicate to other threads that input has already been taken and you don't need to take userinput again.

Problem:

user enters 'c'

xthread exits [or ythread]

However, ythread still keeps looping and exits only after i enter 'c' again. [My assumption is it has read the previous value of read and is still using the same value for comparing]

What have I done wrong?

#include<stdio.h>
#include<sys/select.h>
#include<pthread.h>
static unsigned int i =0;
char read;
int read_input = 0;

void* print_fn(void* arg)
{
    int fd = fileno(stdin);
    struct timeval tv = {0,0};
    fd_set fdset;
    int s;
    char *buffer = NULL;
    unsigned int len;

    while(1)
    {
        struct timespec t = {0,433300000};
        const struct timespec * tp = &t;
        nanosleep(tp,&t);

        printf("\nValue of read is %d",read);

        //sleep(1); 
        FD_ZERO(&fdset);
        FD_SET(fd, &fdset);
        printf("\n%p prints %c and i is %d",pthread_self(),*((char*)arg),i++);  
        if((s = select(fd+1, &fdset, NULL, NULL, &tv)) == 1)
        {
            printf("\nValue of s is %d",s);
            if(!read_input)
                scanf("%c",&read);
            fflush(stdin);
            printf("\nValue of read is %d",read);
            printf("\nChecked for %d or % d",'C','c');
            if(read == 'C' || read == 'c')
            {
                read_input = 1;
                break;
            }
        }
        printf("\nHere");
    }
    printf("\nI, %p survived while(1)",pthread_self());
    return NULL;
}

int main()
{

pthread_t xthread,ythread,checkThread;  
char c1 = 'X', c2 = 'Z';
pthread_create(&xthread,NULL,print_fn,&c1);
pthread_create(&ythread,NULL,print_fn,&c2);
pthread_join(xthread,NULL);
pthread_join(ythread,NULL);

return 0;
}

If there is a better way of taking userinput,please let me know. I don't know if using pthread_cond_t would solve my issue. I don't find the necessity to use a mutex. [Correct me if I am wrong]

Answer 1

Apart from the other problems(as pointed by other StackOverflow-ers) in the code I posted. I realized, the main issue was the line /*3--->*/ if((s = select(fd+1, &fdset, NULL, NULL, &tv)) == 1). According to my logic, If one thread reads the char c from the user it sets read_input to 1. And, other thread when it accesses read_input reads the updated value (since its a global) and exits the while loop.

Now, since i was doing the check /*2--->*/ if(!read_input) within the if block of /*3--->*/ if((s = select(fd+1, &fdset, NULL, NULL, &tv)) == 1) something of this sort happens,

• User inputs c
• xThread reads c and sets read_input as 1
• yThread can't use the updated value of read_input until I enter something on the console[even hitting the enter key would do] again. [That indicates to select that some input is available] and we enter the if(select...block and can test read_input

Keeping rest of the stuff as it is,

shifting line //1----> to /*2--->*/ and the required else /*4--->*/ does the trick.

/*2--->*/   if(!read_input)
        {
/*3--->*/   if((s = select(fd+1, &fdset, NULL, NULL, &tv)) == 1)
            {
            printf("\nValue of s is %d",s);
//1---->        if(!read_input) 
                printf("\nValue of read is %d",read);
                printf("\nChecked for %d or % d",'C','c');
                if(read == 'C' || read == 'c')
                {
                    read_input = 1;
                    break;
                }
            }
        }
/*4--->*/   else
            break;
        printf("\nHere");
    }
    printf("\nI, %p survived while(1)",pthread_self());
}

Note: volatile was not needed

Answer 2

The main problem of your code is that read is not volatile as Daniel said. This means the compiler does not know that it can be changed by an unforeseeable outside force like another thread.

Aside from that your code has a lot of errors and bad practices:

• Do not define symbols that have standard lib name equivalents like read.
• Don't use select on the same file descriptors in multiple threads. This is a recipe for disaster. If both threads return from the select simultaneously they will both attempt to read from stdin and only one will succeed, the other will block. If you want to use a file descriptor for synchronisation set it to nonblocking and use read, it's not signal safe, but better then a full-scale race condition.
• always include pthread.h first.
• You are aware that you are incrementing i non-atomically (race-condition)? I didn't change that, but __sync_fetch_and_add would do the trick atomically.

This is a way:

#include <pthread.h>
#include <unistd.h>
#include <fcntl.h>
#include <stdio.h>

static unsigned int i =0;
volatile char char_read; // has to be volatile since you are checking it with both threads
volatile int read_input = 0; // has to be volatile

void* print_fn(void* arg)
{
    // int fd = fileno(stdin);
    int fd = 0; // stdin is always 0

    while(1)
    {
        struct timespec t = {0,433300000};
        const struct timespec * tp = &t;
        nanosleep(tp,&t);

        printf("\nValue of read is %d",char_read);

        printf("\n%p prints %c and i is %d",pthread_self(),*((char*)arg),i++);  
        if(read_input || scanf("%c",&char_read) > 0) // attempt to read 1 byte
        {
            // printf("\nValue of s is %d",s);
            printf("\nValue of read is %d",char_read);
            printf("\nChecked for %d or % d",'C','c');
            if(char_read == 'C' || char_read == 'c')
            {
                read_input = 1;
                break;
            }
        }
        printf("\nHere");
    }
    printf("\nI, %p survived while(1)\n",pthread_self());
    return NULL;
}

int main()
{
    // make stdin non-blocking
    fcntl(0, F_SETFL, fcntl(0, F_GETFL) | O_NONBLOCK);

pthread_t xthread,ythread,checkThread;  
char c1 = 'X', c2 = 'Z';
pthread_create(&xthread,NULL,print_fn,&c1);
pthread_create(&ythread,NULL,print_fn,&c2);
pthread_join(xthread,NULL);
pthread_join(ythread,NULL);

return 0;
}

Answer 3

The possibility of the compiler optimising away reads of read (bad name, by the way, if one wants to #include <unistd.h>) due to it not being volatile aside,

if((s = select(fd+1, &fdset, NULL, NULL, &tv)) == 1)
{
    printf("\nValue of s is %d",s);
    if(!read_input)
        scanf("%c",&read);
    fflush(stdin);
    printf("\nValue of read is %d",read);
    printf("\nChecked for %d or % d",'C','c');
    if(read == 'C' || read == 'c')
    {
        read_input = 1;
        break;
    }
}

you have the test that breaks the while(1) loop inside the if(select(...)).

So after the first thread read a 'C' or 'c' and exited, the other thread only ever checks the condition when new input is available from stdin (which on my system requires the Return key to be pressed).

Move that condition outside the if (select(...)) for the second thread to have a chance to exit without select reporting that more input is available.

Also,

fflush(stdin);

is undefined behaviour. Although several implementations promise that it does something sensible, you should not rely on it.

Answer 4

Is there any other way of sharing non-global data between threads?

Yes, it's called IPC (inter-proccess communication) And it's possible to use it with pthreads. This includes: Sockets, Pipes, shared memory, etc.

Regarding the Program itself, asDaniel Fischer wrote in the comment, read_input is not volatile, so the compiler is free to optimize it.