0

I am trying to solve the producer consumer problem using mutexes and a shared buffer, but am having trouble accessing values in my shared buffer struct, specifically the char array. When I invoke the producer.c file in one terminal and print the values (the input is a txt file of the alphabet) using

printf("%c", newBuff->bytes[newBuff->rear]); 

the chars do appear as normal, however when I do the same thing in consumer.c, but with

printf("%c", newBuff->bytes[newBuff->front]);

the values appear blank. The newBuff->front value is zero, so it should print the letter a. When I access other values in my struct in consumer.c like front, count, or rear they are correct. Share memory creation as well as attachment also works properly so I believe the issue is either I am not storing the char values properly in the array or I am trying to access them incorrectly. In the code below I placed the printf in the loop for producer.c and then outside the loop for consumer.c so I know for a fact a value is present before the consumer starts extracting data.

Consumer.c

typedef struct buffer{
    pthread_mutex_t lock;
    pthread_cond_t shout;
    int front;
    int rear;
    int count;
    int endOfFile;
    char bytes[1024];
} buffer;


int main(int argc, char const *argv[]) {
    int i=0;
    FILE *file = fopen(argv[1], "w");
    if (argc != 2){
        printf("You must enter in a file name\n");
    }
    int shmid, swapCount=0;
    char swapBytes[] = "";
    char path[] = "~";
    key_t key = ftok(path, 7);
    buffer* newBuff;
    if ((shmid = shmget(key, SIZE, 0666 | IPC_CREAT | IPC_EXCL)) != -1) {
        newBuff = (buffer*) shmat(shmid, 0, 0);
        printf("successful creation\n");
        newBuff->front = 0;
        newBuff->count = 0;
        newBuff->endOfFile = 0;
        pthread_mutexattr_t attr;
        pthread_condattr_t condAttr;

        pthread_mutexattr_init(&attr);
        pthread_mutexattr_setpshared(&attr, PTHREAD_PROCESS_SHARED);
        pthread_mutex_init(&newBuff->lock, &attr);

        pthread_condattr_init(&condAttr);
        pthread_condattr_setpshared(&condAttr, PTHREAD_PROCESS_SHARED);
        pthread_cond_init(&newBuff->shout, &condAttr);
    } //shared memory creation

    else if ((shmid = shmget(key, 0, 0)) != -1){
        printf("%d\n", shmid);
        printf("successful attachment\n" );
        newBuff = (buffer*) shmat(shmid, 0, 0);
        printf("%c\n", newBuff->count);
    }
    else{
        printf("oops\n");
        exit(0);
    }
    pthread_mutex_lock(&newBuff->lock);
    printf("%c\n", newBuff->bytes[newBuff->front]);
    while (newBuff->endOfFile != 1)
    {
        while (newBuff->count == 0){
            pthread_cond_signal(&newBuff->shout);
            pthread_cond_wait(&newBuff->shout, &newBuff->lock);
        }
        newBuff->front = ((newBuff->front + 1)%SIZE);
        newBuff->count--;
    }
    pthread_mutex_unlock(&newBuff->lock);
    shmdt(&newBuff);

    //pthread_mutexattr_destroy(&attr);
    //pthread_condattr_destroy(&condAttr);*/

    return 0;
}

Producer.c

typedef struct buffer{
    pthread_mutex_t lock;
    pthread_cond_t shout;
    int front;
    int rear;
    int count;
    int endOfFile;
    char bytes[1024];
} buffer;


int main(int argc, char const *argv[]) {
    FILE *file = fopen(argv[1], "r");
    if (argc != 2){
        printf("You must enter in a file dumbass\n");
    }
    int shmid;
    char path[] = "~";
    key_t key = ftok(path, 7);
    buffer* newBuff;
    printf("dfasdfasdf\n");
    if ((shmid = shmget(key, SIZE, 0666 | IPC_CREAT | IPC_EXCL)) != -1) {
        newBuff = (buffer*) shmat(shmid, 0, 0);
        printf("successful creation\n");


        newBuff->front = 0;
        newBuff->count = 0;
        newBuff->endOfFile=0;
        pthread_mutexattr_t attr;
        pthread_condattr_t condAttr;

        pthread_mutexattr_init(&attr);
        pthread_mutexattr_setpshared(&attr, PTHREAD_PROCESS_SHARED);
        pthread_mutex_init(&newBuff->lock, &attr);

        pthread_condattr_init(&condAttr);
        pthread_condattr_setpshared(&condAttr, PTHREAD_PROCESS_SHARED);
        pthread_cond_init(&newBuff->shout, &condAttr);
    } //shared memory creation

    else if ((shmid = shmget(key, 0, 0)) != -1){
        printf("successful attachment\n" );
        newBuff = (buffer*) shmat(shmid, 0, 0);
    }
    else{
        printf("oops\n");
        exit(0);
    }
    printf("%d\n", shmid);
    pthread_mutex_lock(&newBuff->lock);
    while (fscanf(file, "%c", &newBuff->bytes[newBuff->rear]) != EOF) //read in file
    {
        printf("%c\n", newBuff->bytes[newBuff->rear]);
        while (newBuff->count >= SIZE){ //buffer is full
            //("%c\n", newBuff->bytes[newBuff->rear]);
            pthread_cond_signal(&newBuff->shout);
            pthread_cond_wait(&newBuff->shout, &newBuff->lock);
        }
        //printf("%c\n", newBuff->bytes[newBuff->rear]);
        newBuff->rear = ((newBuff->front + 1)%SIZE);
        newBuff->count++;
    }
    newBuff->endOfFile = 1;
    pthread_cond_signal(&newBuff->shout);
    pthread_mutex_unlock(&newBuff->lock);

    shmdt(&newBuff);

    //pthread_mutexattr_destroy(&attr);
    //pthread_condattr_destroy(&condAttr);

    return 0;
}
Tormund Giantsbane
  • 355
  • 1
  • 4
  • 12
Eddy
  • 3
  • 4
  • 2
    At some point either your formatting is broken or you have code outside of a function. Please fix that. Also, please remove things that are not necessary to reproduce the problem. I'd also suggest you remove the brute-force casts, they typically only serve to hide problems, like e.g. when casting an int into a pointer. – Ulrich Eckhardt Feb 07 '18 at 19:37
  • 1
    Beware that path must exist in a call to `ftok`, this is a requirement. And using `~` is wrong because it is expanded by shells only. SO use a real path name of your choice. This may not be the cause of your problem, but it is a requirement... – Jean-Baptiste Yunès Feb 07 '18 at 19:44
  • Call to `shmget(key, 0, 0)` is also wrong, size is 0. You should use your `SIZE` constant (as I understand). – Jean-Baptiste Yunès Feb 07 '18 at 19:46
  • Unless you need to reference the _name_ of the struct, (as far as I can see, you do not) then you do not need it, and in general, you should avoid creating a `type` with the same symbol as that used for a `name`, i.e. you have created a new type with the symbol `buffer`, as well as a name with the same symbol. Replace what you have with `typedef struct {...}buffer;` – ryyker Feb 07 '18 at 19:53
  • I would account that a style consideration, @rykker. It certainly is *unnecessary* to declare a structure type with a tag and to also declare the same identifier as an alias for that type, as the OP does, but it is not inherently harmful, and some coding styles call for it. – John Bollinger Feb 07 '18 at 19:57
  • @JohnBollinger - I cannot think of a coding _style_ that requires both. I know that linked lists need to self-reference, thus requiring the tag (but not necessarily the typedef to a new type). What examples can you cite? (I am truly curious as I cannot think of one). And if one of the two are not needed, would it not be considered a readability/maintenance issue to have them? – ryyker Feb 07 '18 at 20:02
  • @rykker, examples positively *abound* of coding styles that call for structures to be both tagged and typedefed. Here's an SO question that talks about it: https://stackoverflow.com/questions/3538170/standard-for-typedefing. It is less common that the tag and typedefed identifier are called upon to match in C, but some people with C++ background tend to do it because C++ provides the equivalent by default. Nevertheless, here's a C code style document from a Cornell University CS course that recommends it: http://www.cs.cornell.edu/courses/cs314/2004fa/tutorials/cstyle.pdf (page 6). – John Bollinger Feb 07 '18 at 20:21
  • What operating system API? POSIX? The C language knows nothing of "another" process. Please tag your question appropriately. – mlp Feb 07 '18 at 20:21
  • @Jean-BaptisteYunès, as far as I know or can determine, the second argument to `shmget()` is only significant when `IPC_CREAT` is specified among the flags in the third argument, and no segment with the specified key already exists. Otherwise, *i.e.* when opening an existing SystemV shared memory segment, the size of the segment is whatever it is, and the second argument is therefore ignored. – John Bollinger Feb 07 '18 at 20:29
  • @JohnBollinger you are correct with the implementation of shmget() – Eddy Feb 07 '18 at 20:31
  • You have a potential synchronization problem surrounding initialization of your mutex and condition variable. Whichever process succeeds at creating the shared-memory segment initializes these, but that itself is not synchronized, so the other process(es) may try to use them before that initialization is complete. – John Bollinger Feb 07 '18 at 20:35
  • @JohnBollinger i believe the initiation is completed because when i invoke producer.c first to create the shared memory segment, it exit the if statement and then goes into the loop to put data into the shared buffer. – Eddy Feb 07 '18 at 20:45
  • @Eddy, whether it's responsible for the behavior you describe or not, you *do* have a synchronization problem such as I describe. That may be something you can ignore or work around, but you should at minimum be aware of it. – John Bollinger Feb 07 '18 at 20:48
  • Your use of `SIZE` is suspicious. I do not find a declaration anywhere in the posted code, but you seem to be using it both as the capacity of the buffer and as the overall size of the shared-memory segment. These are not the same thing. – John Bollinger Feb 07 '18 at 20:50
  • @JohnBollinger I accidentally deleted the constant i defined for size which is 1024, but i checked the synchronization of variables, i.e made newBuff->front = 20 in the producer, then printed that member in consumer and got 20, and it seems as thought it is only for the char buffer->bytes that is having the issue – Eddy Feb 07 '18 at 20:58
  • @Eddy, that doesn't change the fact that in your `shmget()` calls, you are asking only for `SIZE` bytes of shared memory. That's not enough. It does get rounded up to a multiple of the page size, but you still should be asking for at least as much as you need: `sizeof(struct buffer)`. – John Bollinger Feb 07 '18 at 21:17
  • @JohnBollinger I changed it to sizeof(newBuff) and the same issue exists. – Eddy Feb 07 '18 at 21:54

1 Answers1

0

There are several difficulties with your code, some already addressed in comments:

  • ftok() requires the path passed to it to designate an existing file, but the path you are passing does not.

  • You request less shared memory than you actually need: only the size of the buffer content, not of a whole struct buffer. Because the amount of shared memory actually allocated will be rounded up to a multiple of the page size, this may end up being ok, but you should ensure that it will be ok by requesting the amount you actually need.

  • System V shared memory segments have kernel persistence, so once created, they will continue to exist until they are explicitly removed or the system is rebooted. You never remove yours. You also initialize its contents only when you first create it. Unless you manually delete it between runs, therefore, you'll use old data -- with the end-of-file indicator set, for instance -- on the second and subsequent runs. I suggest having the consumer schedule it for removal.

  • The consumer prints only one byte of data from the buffer, and it does so before verifying that there is anything to read.

  • After adding a byte to the buffer, the producer does not update the available byte count until after signaling the consumer. At best, this is wasteful, because the consumer will not see the change in count until the next time (if any) it wakes.

  • The producer updates the rear index of the buffer incorrectly, based on the current front value instead of on the current rear value. The data will therefore not be written into the correct places in the buffer array.

  • Once the producer sets the endOfFile flag, the consumer ignores all but one of any remaining unread bytes.

  • If the producer leaves the count zero when it finishes, the consumer will deadlock.

I find that modified versions of your programs addressing all of these issues successfully and accurately communicate data through shared memory.

Update:

Also,

  • The way in which consumer and / or producer initializes the mutex and condition variable is not itself safe. It is possible for whichever process attempts the shmget() second (or third, or ...) to access those objects before the first finishes initializing them. More generally, once a shared memory segment is attached, there is no inherent memory barrier involved in writing to it. To address these issues, the natural companion to SysV shared memory is SysV semaphores.
John Bollinger
  • 160,171
  • 8
  • 81
  • 157
  • All excellent points. Worth adding a mention of memory fences too, for good measure – bazza Feb 07 '18 at 23:03
  • Thank you for taking the time to help me with my issue. As for the issues addressed not in bold, they were mostly addressed and fixed. The points made in old however were truly the issues that fixed my code. It appears my professor gave me the wrong algorithm to change the rear index so that was the reason I was not receiving the proper output in my consumer.c I will post the finished code after i tackle the final issue of swapping every other byte in the output file. – Eddy Feb 09 '18 at 04:16