reading file line by line using pthreads ... exits unexpectedly

Question

I have the following code:

    /*//not important
    FILE * INFILE;
    list_file = optarg;
    if( ( INFILE = fopen( list_file, "a+" ) ) == NULL ) {
        fprintf( stderr, "Can't open input file\n");
        exit(0);
    }
    */

    pthread_mutex_t input_queue;
    pthread_mutex_init(&input_queue, NULL);

    for( i = 0 ; i < number_thread; i++)
    {
        if( pthread_create( &thread_id[i], NULL, &work, NULL) != 0 )
        {
            i--;
            fprintf(stderr, RED "\nError in creating thread\n" NONE);
        }
    }
    for( i = 0 ; i < number_thread; i++)
        if( pthread_join( thread_id[i], NULL) != 0 )
        {
            fprintf(stderr, RED "\nError in joining thread\n" NONE);
        }




    void * work(void * data)
    {
        unsigned long line;
        char buf[512];
        while ( !feof(INFILE) )
        {
            pthread_mutex_lock(&input_queue);
            fgets((char *)&buf, sizeof(buf), INFILE);
            if (buf[strlen (buf) - 1] == '\n')
                buf[strlen (buf) - 1] = '\0';
            line = (unsigned long)buf;
            pthread_mutex_unlock(&input_queue);
            do_work( line );
        }
        fclose(INFILE);
        return NULL;
    }

it reads lines from file but after a while it exits unexpectedly, no error message. I guess I messed up something.

How can I read the file line by line using pthreads but keep as much as possible the code unchanged (I mean not to mess up the whole program) ?

certainly, that's the reason I asked here where there are so many experts. I'm a newbie — ShaMora, Feb 23 '13 at 16:07
`fgets((char *)&buf, ...` is a bit weird, too - why not just `fgets(buf, ...`? — Carl Norum, Feb 23 '13 at 16:08
well, I searched a lot before posting here and I made that code from pieces found on other related questions :P — ShaMora, Feb 23 '13 at 16:10

user4815162342 · Accepted Answer · 2013-02-23T17:40:46.933

You are closing INFILE in the first thread that encounters EOF. Afterwards other threads will call feof() — and possibly fclose() — on the closed file, which will corrupt the heap and almost certainly lead to a crash. Also, your newline-chopping code can underrun your buffer at EOF, see remark below.

To fix the problem, protect feof() and fclose() with the same mutex, and set INFILE to NULL. When the mutex is acquired, check for INFILE being NULL and return immediately if so:

for (;;) {
  pthread_mutex_lock(&input_queue);
  if (!INFILE) {
    pthread_mutex_unlock(&input_queue);
    break;
  }
  if (feof(INFILE)) {
    INFILE = NULL;
    pthread_mutex_unlock(&input_queue);
    break;
  }

  fgets(buf, sizeof(buf), INFILE);
  pthread_mutex_unlock(&input_queue);

  // ...strip newline, do_work...
}

Several remarks:

your code writes to buf[strlen(buf) - 1] without checking whether strlen(buf) is zero. buf will be empty at EOF, so this is not a theoretical concern, it will happen exactly once in every execution.
line is of type unsigned long, but you are assigning it a pointer value. This will fail on platforms where long does not contain a pointer, such as Win64. Declare line and the argument of do_work as char * (or void * if it must accept other pointer types) instead.
avoid calling your mutex a "queue"; in multithreaded programming queue refers to a producer-consumer aware FIFO.
you don't need to protect individual stdio functions like fgets with mutexes. They are MT-safe, as mandated by POSIX. (However, in my modified code, fgets() does need to be mutex-protected because INFILE can become invalidated while the mutex is not being held.)
(char *) &buf doesn't make sense. Since buf is a char array, it already decays to a pointer to its first member, so you can simply send buf to fgets. If you insist on using the address-of operator, the correct expression is &buf[0].
as Carl Norum hinted, feof() is probably not what you want, since it only detects EOF condition already encountered by fgets(). The correct way to check for EOF is by testing whether fgets() returns an empty string — before stripping the newline.

thank you. shall I add your code to void work ? or inside main ? — ShaMora, Feb 23 '13 at 16:20
@ShaMora The loop is meant to be a replacement of the `while ( !feof(INFILE) ) { ... }` loop. — user4815162342, Feb 23 '13 at 16:21
still exits unexpectedly after the first threads do their job :( — ShaMora, Feb 23 '13 at 16:53
@ShaMora It's useless to comment further without seeing the **entire** code. — user4815162342, Feb 23 '13 at 17:01
the entire code has 2000 lines. If you say it's not because of that piece of code I will accept your answer and try to figure out what else is making the program exit unexpectedly — ShaMora, Feb 23 '13 at 17:04
@ShaMora In that case it is your task to strip down the code to a [minimal runnable example that demonstrates the problem](http://sscce.org/). I see another issue in your code: you are writing to `buf[-1]` when `fgets()` encounters EOF. See the edited answer. — user4815162342, Feb 23 '13 at 17:42

score 1 · Answer 2 · edited Jun 19 '14 at 03:06

if INFILE is a global variable then you have closed the refrence in the function of thread ,and if you have created multiple threads then flcose(INFILE) in other threads i.e fclose(NULL) is expected to crash. Cant guess what you are trying to do with multiple threads,but its better to close it in end when you are sure that INFILE no longer will be accessed by any other thread.i think you should close the INFILE refrence in main after all threads have joined with main and done their processing.

#include<stdio.h>
#include<pthread.h>
#include<string.h>
#include<stdlib.h>
#define number_thread 10

FILE * INFILE;
char *list_file = "test_thread";
pthread_mutex_t input_queue;

void do_work(unsigned long buf)
{
    printf("working on %u\n",buf);
}

void * work(void * data)
{
    unsigned long line;
    char buf[512];
    printf("IAM NEW THREAD\n" );

    while ( !feof(INFILE) )
      {
        pthread_mutex_lock(&input_queue);
        fgets((char *)&buf, sizeof(buf), INFILE);
        if (buf[strlen (buf) - 1] == '\n')
            buf[strlen (buf) - 1] = '\0';
        line = (unsigned long)buf;
        pthread_mutex_unlock(&input_queue);
        do_work( line );
      }

    return NULL;
}

int main()
{
    printf("IAM MAIN THREAD\n")
    pthread_mutex_init(&input_queue, NULL);
    if( ( INFILE = fopen( list_file, "a+" ) ) == NULL ) {
        fprintf( stderr, "Can't open input file\n");
        exit(0);
    }
    pthread_t thread_id[10];

    int i=0;
    for( i = 0 ; i < number_thread; i++)
      { 
        if( pthread_create( &thread_id[i], NULL, &work, NULL) != 0 )
          {
            i--;
            fprintf(stderr,  "\nError in creating thread\n");
          }
      }

    for( i = 0 ; i < number_thread; i++)
        if( pthread_join( thread_id[i], NULL) != 0 )
          {
            fprintf(stderr,  "\nError in joining thread\n" );
          }

    fclose(INFILE);
}

yes, I removed the fclose(INFILE); from void work and it closes in main, but still exits unexpectedly — ShaMora, Feb 23 '13 at 16:58
i have placed the code,in raw form as i cant still guess your intention for creating multiple threads.but the code is not crashing now,earlier there was a stack overwriting..:-) — Mohit Narain Chaudhary, Feb 24 '13 at 10:15
i have place your code ,having a rough idea editing ,it wont exit unexpectedly — Mohit Narain Chaudhary, Feb 24 '13 at 14:56
Putting aside its invalid use, here `while (!feof(INFILE))` access to `INFILE` is still unprotected. — alk, Oct 07 '18 at 06:19

reading file line by line using pthreads ... exits unexpectedly

2 Answers2