Open, write and close a file

Question

I have a small 10-liner function that writes some data to a file using an std::ofstream. I did not explicitly call .close() at the end of my function, but it failed code review with the reason that it is better to explicitly call it for style and verbosity reasons. I understand there is no harm in calling .close() explicitly, but does calling it explicitly just before a return statement indicate a lack of understanding or faith in RAII?

The C++ standard says:

§27.8.1.2

virtual ~ basic_filebuf ();

[3] Effects: Destroys an object of class basic_filebuf<charT,traits>. Calls close().

Am I justified in my argument that calling .close() at the end of a function is redundant and/or unnecessary?

bool SomeClass::saveData()
{
    std::ofstream saveFile(m_filename);

    if (!saveFile.is_open())
        return false;

    saveFile << m_member1 << std::endl;
    saveFile << m_member2 << std::endl;

    saveFile.close(); // passed review only with this line
    return true;
}

The function is only supposed to return false if the file could not be opened for writing.

Answer 1

I would argue the exact opposite.

Explicitly closing a stream is probably not what you want to do. This is because when you close() the stream there is the potential for exceptions to be thrown. Thus when you explicitly close a file stream it is an indication you both want to close the stream and explicitly handle any errors that can result (exceptions or bad-bits) from the closing of the stream (or potentially you are saying if this fails I want to fail fast (exception being allowed to kill the application)).

If you don't care about the errors (ie you are not going to handle them anyway). You should just let the destructor do the closing. This is because the destructor will catch and discard any exceptions thus allowing code to flow normally. When dealing with the closing of a file this is what you normally want to do (if the closing fails does it matter?).

Answer 2

Assuming that the fstream object is local to the function, I would tend to argue against this. People need to become accustomed to letting RAII do its job, and closing an fstream object falls under that heading. Extra code that doesn't accomplish something useful is almost always a poor idea.

Edit: Lest I be misunderstood, I would argue against this, not only for this specific case, but in general. It's not merely useless, but tends to obscure what's needed, and (worst of all) is essentially impossible to enforce in any case -- people who think only in terms of the "normal" exit from the function really need to stop and realize that the minute they added exception handling to C++, the rules changed in a fundamental way. You need to think in terms of RAII (or something similar) that ensures cleanup on exit from scope -- and explicitly closing files, releasing memory, etc., does not qualify.

Answer 3

There is a middle ground here. The reason the reviewers want that explicit close() "as a matter of style and verbosity" is that without it they can't tell just from reading the code if you meant to do it that way, or if you completely forgot about it and just got lucky. It's also possible their egos were bruised from failing to notice or remember, at least at first, that close() would be called by the destructor. Adding a comment that the destructor calls close() isn't a bad idea. It's a little gratuitous, but if your coworkers need clarification and/or reassurance now, there's a good chance a random maintainer a few years down the road will too, especially if your team doesn't do a lot of file I/O.

Answer 4

I agree with Loki's answer: the difference between calling close explicitly, and letting the destructor call close, is that the destructor will implicitly catch (i.e. conceal) any exception thrown by close.

The destructor must do this (not propagate exceptions) because it may be called if/while there is an exception already being thrown; and throwing a 2nd exception during a 1st exception is fatal (therefore, all destructors should avoid throwing exceptions).

Unlike Loki I would argue that you do want to call close explicitly, precisely because you do want any exception from close to be visible. For example, perhaps the data is important and you want it written to disk; perhaps the disk is full, the << output operator is written to in-memory cache, and no-one notices that the disk is full until close implicitly calls flush. You're not allowed to return false because false is defined as meaning that the file couldn't be opened. IMO the only sane/safe thing you can do, then, is throw an exception.

It's up to the caller to catch any exception; having no exception thrown should be a guarantee that close was successful and the data safely written to the O/S.

Answer 5

I'm torn on this one. You are absolutely correct. However if a coding standard requires calling close() explicitly or it's a group people's consensus of doing that, there's not much you can do. If I were you, I would just go with the flow. Arguing such things is unproductive.

Answer 6

I believe you are asking two questions in one. Should you use exceptions or return values? Should you use RAII or not?

If exceptions are not permitted in your company, then fstream::exceptions() must be set globally for your project. And you have to interrogate the error flag too.

If RAII is not permitted in your company, then do not use C++. If you use RAII, then any exception thrown in the destructor will be swallowed. This sounds terrible and it is. However I agree with others that RAII is the way to go also because any error handling here is futile and creates unreadable code. This is because "flush" does not do what you may think it does. It instructs the operating system to do it on your behalf. The OS will do it when it believes it is convenient. Then, when the operating flushes (which may be a minute after your function returns) similar things may happen at the hardware level. The disk may have an SSD cache which it flushes to the rotating disks later (which may happen at night when it is less busy). At last the data ends on the disk. But the story does not end here. The data may have been saved correctly but the disk gets destroyed by whichever of the many possible causes. Hence if RAII is not transactionally safe enough for you, then you need to go to a lower API level anyway and even that will not be perfect. Sorry for being a party pooper here.

Answer 7

You also need to check that the write operations (<<) succeeded. So instead of checking is_open(), just go through the whole series of operations, and check failbit at the end:

bool save_data() const
{
    std::ofstream saveFile(m_filename);

    saveFile << m_member1 << '\n'
             << m_member2 << '\n';

    saveFile.close(); // may set failbit

    return saveFile;
}

Also, unless there's a pressing need to flush each line as it's written, prefer to use '\n' rather than std::endl (as I've shown), and let close() write it all in one go - that can make a significant difference to the speed, particularly when there's a large number of lines to be written.

Answer 8

After reading through the question and the answers I came to the conclusion that this is were comments can come into play. I had recently read this Q/A Guessing a number, but comments concerning and the accepted answer gave me insight to the situation here. Use comments to explain the why, let the code explain the how. I can use your function similarly for each case as an example:

bool SomeClass::saveData() {
    std::ofstream saveFile(m_filename);

    if (!saveFile.is_open())
        return false;

    saveFile << m_member1 << std::endl; // I would replace `endl` with `'\n'`
    saveFile << m_member2 << std::endl; // for performance reasons. 

    // I intentionally want to close the file before the closing scope to
    // free up limited resources and to log potential exceptions and errors.  
    saveFile.close(); 
    return true;
} 

bool SomeClass::saveData() {
    std::ofstream saveFile(m_filename);

    if (!saveFile.is_open())
        return false;

    saveFile << m_member1 << std::endl; // I would replace `endl` with `'\n'`
    saveFile << m_member2 << std::endl; // for performance reasons. 

    // Resources and errors are not a concern: relying on RAII no need
    // to call file.close();
    return true;
}

Then with the appropriate type of commenting for valid reasons might serve you well. Then at least this way; one would know that you intended to call or omit it and why! Well written code is the how and shouldn't need comments.