0

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]

Suvarna Pattayil
  • 5,136
  • 5
  • 32
  • 59
  • 1
    `read` is not `volatile`, so the thread may assume it doesn't change from outside. (Rather, the compiler assumes, and makes the thread read `read` only once.) – Daniel Fischer Apr 02 '13 at 13:16
  • I shall add "Race Conditions" to the array currently containing `[ "Undefined Behavior", "SQL Injection" ]`, of which the members should always be written in bold and blue. –  Apr 02 '13 at 13:18
  • 1
    @Daniel Fischer I made read as `char volatile read;` but the situation still persists. – Suvarna Pattayil Apr 02 '13 at 13:23
  • 1
    @SuvP: You should be using a mutex for this purpose (i.e. to make read/write atomic). Refer to `pthread_mutex_lock` and `pthread_mutex_unlock`. If you are feeling adventurous, this may be done better still with a condition variable (`pthread_cond_t`) which also requires a mutex (this will signal the waiting thread to continue rather than checking a flag). A mutex is a fancy name for a semaphore of value 1. A semaphore is a fancy name for a lock which allows a specified number of users into the critical section. – RageD Apr 02 '13 at 13:31
  • 1
    @DanielFischer: I doubt `volatile` would help. You might like to read here: http://stackoverflow.com/a/6858294/694576. What is missing here is a mutex, as *RageD* mentions, to protect `read` and `read_input` against concurrent access. – alk Apr 02 '13 at 15:02

4 Answers4

1

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.

stdcall
  • 27,613
  • 18
  • 81
  • 125
  • Even simpler, if he allocates a struct on the stack (assuming the stack persists throughout thread execution) or on the heap (this is simpler/safer), then he can simply pass that struct with the collection of variables to both threads as input data. – RageD Apr 02 '13 at 13:28
  • I agree, but this is a bit dangerous. – stdcall Apr 02 '13 at 13:33
1

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.

Daniel Fischer
  • 181,706
  • 17
  • 308
  • 431
0

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;
}
Sergey L.
  • 21,822
  • 5
  • 49
  • 75
0

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

Suvarna Pattayil
  • 5,136
  • 5
  • 32
  • 59