CODEWITHSUNDEEP
c++multithreading
Greg Brown
Greg Brown

Reputation: 1299

Reading two files and outputting to another file using threads

My program reads in two input files and alternates writing lines to an output file. I have it so it writes in the right order (first file, then second , then first again, ....) but the problem is it writes the last character in each file twice at the end. 

#include <iostream>
#include <fstream>
#include <thread>
#include <mutex>
using namespace std;


mutex mtx;
int turn = 1;

void print_line(ifstream * in_stream, ofstream * out_stream, int t);

int main(int argc, const char * argv[]){
    ifstream input_file_1;
    ifstream input_file_2;
    ofstream output_file;

    input_file_1.open("input_1");
    input_file_2.open("input_2");
    output_file.open("output");

    if (input_file_1.fail() || input_file_2.fail() || output_file.fail()) {
        cout << "Error while opening the input files\n";
        exit(EXIT_FAILURE);
    }
    else{
        thread input1 (print_line, &input_file_1, &output_file, 1);
        thread input2 (print_line, &input_file_2, &output_file, 2);
        input1.join();
        input2.join();
    }
    input_file_1.close();
    input_file_2.close();
    output_file.close();

    return 0;
}

void print_line(ifstream * in_stream, ofstream * out_stream, int t){
    char temp;
    while (!in_stream->eof()) {
        mtx.lock();
        if (turn == t) {
            *in_stream>>temp;
            *out_stream<<temp;
            if (turn == 1) {
                turn = 2;
            }
            else{
                turn = 1;
            }
        }
        mtx.unlock();
    }
}

input 1

a
c
e

input 2

b
d
f

output 

abcdefef

I don't know why it writes the last character again, also is there a better way to do the ordering part using threads, I understand that a mutex is used to make sure both threads don't write at the same time, however how can I guarantee that thread one executes before thread 2 and make sure it keeps alternating?
Thanks

Upvotes: 3

Views: 1772

Answers (2)

Felix Glas
Felix Glas

Reputation: 15524

The correct and idiomatic way of reading from an std::ifstream until EOF is reached is:

char temp;
while(in_stream >> temp) {
    // Will only be entered if token could be read and not EOF.
}

Compared to this. Assume all but last characters have already been read from stream:

while(!in_stream.eof()) {
    in_stream >> temp; // 1st iteration: reads last character, STILL NOT EOF.
                       // 2nd iteration: tries to read but reaches EOF.
                       //                Sets eof() to true. temp unchanged.
                       //                temp still equal to last token read.
                       //                Continue to next statement...
    /* More statements */
}

Secondly, your function print_line has some problems regarding synchronization. One way to solve it is to use a std::condition_variable. Here's an example:

condition_variable cv;

void print_line(ifstream& in_stream, ofstream& out_stream, int t){
    char temp;
    while (in_stream >> temp) {
        unique_lock<mutex> lock(mtx); // Aquire lock on mutex.

        // Block until notified. Same as "while(turn!=t) cv.wait(lock)".
        cv.wait(lock, [&t] { return turn == t; });
        out_stream << temp;
        turn = (turn == 1) ? 2 : 1;
        cv.notify_all(); // Notify all waiting threads.
    }
}

As you see in the example above I have also passed the streams as references instead of pointers. Passing pointers is error prone as it is possible to pass nullptr (NULL-value).

To pass the streams as references to the constructor of std::thread you must wrap them in a reference wrapper std::ref, e.g. like this: (ctor of std::thread copies the arguments)

thread input1(print_line, ref(input_file_1), ref(output_file), 1);

Live example (slightly modified to use standard IO instead of fstream)

Some other things:

1. Unnecessary code in main:

ifstream input_file_1;
ifstream input_file_2;
ofstream output_file;

input_file_1.open("input_1");
input_file_2.open("input_2");
output_file.open("output");

Use the constructor taking the filename directly here instead of using open:

ifstream input_file_1("input_1");
ifstream input_file_2("input_2");
ofstream output_file("output");

2. Use idiomatic way to check if stream is ready for reading:

if (!input_file_1 || !input_file_2 || !output_file) {

3. Unnecessary to use close in this case, as the dtor will close the resources (rely on RAII).

input_file_1.close(); // \
input_file_2.close(); //  } Unnecessary
output_file.close();  // /

4. Your design is somewhat poor as further accesses to any of the streams or turn in the main function will result in a data race.

(5.) Prefer to not pollute the namespace with using namespace std, instead use fully qualified names (e.g. std::ifstream) everywhere. Optionally declare using std::ifstream etc. inside relevant scope.

Upvotes: 2

JensG
JensG

Reputation: 13411

Regarding the EOF: There's a fairly good explanation over here: How does ifstream's eof() work?

Regarding the lock: Lock only for the time you do the output, to reduce lock contention and switching the turn variable.

Besides that, it is a somwewhat horrible design in my opinion. I'm not even sure, if a C++ stream can be used that way across threads, but even then I would doubt that it is a good idea.

Upvotes: 1

Related Questions