Implementing File class for both read and write operations on the file

Question

I need to implement a class which holds a regular text file that will be valid for both read and write operations from multiple threads (say, "reader" threads and "writers").

I am working on visual studio 2010 and can use only the available libraries that it (VS 2010) has, so I chose to use the std::fstream class for the file operations and the CreateThread function & CRITICAL_SECTION object from the header.

I might start by saying that I seek, at the beginning, for a simple solution - just so it works....:)

My idea is as follows:

I created a File class that will hold the file and a "mutex" (CRITICAL_SECTION object) as private members.

In addition, this class (File class) provides a "public interface" to the "reader/writer" threads in order to perform a synchronized access to the file for both read and write operations.

See the header file of File class:

class File {

    private:
        std::fstream iofile;
        int size;
        CRITICAL_SECTION critical;

    public:
        File(std::string fileName = " ");
        ~File();
        int getSize();

        // the public interface:
        void read();
        void write(std::string str);

};

Also see the source file:

#include "File.h"
File :: File(std::string fileName) 
{
    // create & open file for read write and append 
    // and write the first line of the file
    iofile.open(fileName, std::fstream::in | std::fstream::out | std::fstream::app);  // **1)**

    if(!iofile.is_open()) {
        std::cout << "fileName: " << fileName <<  " failed to open! " << std::endl;
      }

    // initialize class member variables
    this->size = 0;
    InitializeCriticalSection(&critical);   
}

File :: ~File()
{
    DeleteCriticalSection(&critical);
    iofile.close();  // **2)**
}

void File :: read()
{
    // lock "mutex" and  move the file pointer to beginning of file
    EnterCriticalSection(&critical);
    iofile.seekg(0, std::ios::beg); 

    // read it line by line
    while (iofile)
    {
        std::string str;
        getline(iofile, str);
        std::cout << str << std::endl;
    }

    // unlock mutex
    LeaveCriticalSection(&critical);

    // move the file pointer back to the beginning of file
    iofile.seekg(0, std::ios::beg); // **3)**

}

void File :: write(std::string str)
{
    // lock "mutex"
    EnterCriticalSection(&critical);

    // move the file pointer to the end of file
    // and write the string str into the end of the file
    iofile.seekg(0, std::ios::end);  // **4)**
    iofile << str;

    // unlock mutex
    LeaveCriticalSection(&critical);
}

So my questions are (see the numbers regarding the questions within the code):

1) Do I need to specify anything else for the read and write operations I wish to perform ?

2) Anything else I need to add in the destrutor?

3) What do I need to add here in order that EVERY read operation will occur necessarily from the beginning of the file ?

4) What do I need to modify/add here in order that each write will take place at the end of the file (meaning I wish to append the str string into the end of the file)?

5) Any further comments will be great: another way to implement , pros & cons regarding my implementation, points to watch out , etc'.....

Thanks allot in advance,

Guy.

Answer 1

1. You must handle exceptions (and errors in general).
2. No, you destructor even has superfluous things like closing the underlying fstream, which the object takes care of itself in its destructor.
3. If you always want to start reading at the beginning of the file, just open it for reading and you automatically are at the beginning. Otherwise, you could seek to the beginning and start reading from there.
4. You already opened the file with ios::app, which causes every write operation to append to the end (including that it ignores seek operations that set the write position, IIRC).

5. There is a bunch that isn't going to work like you want it to...

   • Most importantly, you need to define what you need the class to behave like, i.e. what the public interface is. This includes guarantees about the content of the file on disk. For example, after creating an object without passing a filename, what should it write to? Should that really be a file who's name is a single space? Further, what if a thread wants to write two buffers that each contain 100 chars? The only chance to not get interrupted is to first create a buffer combining the data, otherwise it could get interrupted by a different thread. It gets even more complicate concerning the guarantees that your class should fulfill while reading.
   • Why are you not using references when passing strings? Your tutorial should mention them.
   • You are invoking the code to enter and leave the critical section at the beginning and end of a function scope. This operation should be bound to the ctor and dtor of a class, check out the RAII idiom in C++.
   • When you are using a mutex, you should document what it is supposed to protect. In this case, I guess it's the iofile, right? You are accessing it outside the mutex-protected boundaries though...
   • What is getSize() supposed to do? What would a negative size indicate? In case you want to signal errors with that, that's what exceptions are for! Also, after opening an existing, possibly non-empty file, the size is zero, which sounds weird to me.
   • read() doesn't return any data, what is it supposed to do?
   • Using a while-loop to read something always has to have the form "while try-to-read { use data}", yours has the form "while success { try-to-read; use data; }", i.e. it will use data after failing to read it.
   • Streams have a state, and that state is sticky. Once the failbit is set, it remains set until you explicitly call clear().

BTW: This looks like logging code or a file-backed message queue. Both can be created in a thread-friendly way, but in order to make suggestions, you would have to tell us what you are actually trying to do. This is also what you should put into a comment section on top of your class, so that any reader can understand the intention (and, more important now, so that YOU make up you mind what it's supposed to be).