67
\$\begingroup\$

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.

\$\endgroup\$
  • 7
    \$\begingroup\$ If the reviewers need reassurance that close is called automatically, then C++ is probably not the best language choice. The "verbosity" reason is particularly alarming. \$\endgroup\$ – Fred Nurk Feb 2 '11 at 11:33
  • 2
    \$\begingroup\$ “it failed code review with the reason that it is better to explicitly call it for … verbosity reasons ” – Please explain that last bit, it doesn’t make any sense. \$\endgroup\$ – Konrad Rudolph Feb 2 '11 at 14:16
  • \$\begingroup\$ Obviously your reviewers' style conflict with the standard design of the standard library. \$\endgroup\$ – kizzx2 Feb 2 '11 at 16:11
  • 3
    \$\begingroup\$ @Konrad: By verbosity, I mean that we are closing the file even knowing it will be closed anyway in the destructor, because it shows that we know we are done with it. In some instances, we have files open for a long time; so we have a rule that we close every stream explicitly. \$\endgroup\$ – dreamlax Feb 3 '11 at 2:34
  • 1
    \$\begingroup\$ @dreamlax: but how is verbosity ever an advantage? I agree that explicitness may be, and that this sometimes entails verbosity – but this is always a trade-off between the two. I have never seen a situation where verbosity would be an advantage in itself. That’s what I meant by “it doesn’t make any sense”. \$\endgroup\$ – Konrad Rudolph Feb 3 '11 at 8:21
70
\$\begingroup\$

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?).

\$\endgroup\$
  • 2
    \$\begingroup\$ "if the closing fails does it matter?" For output streams, I think it probably does, as the output may be buffered until the call to close(). \$\endgroup\$ – Roddy Feb 2 '11 at 10:40
  • 2
    \$\begingroup\$ @Roddy: but you can’t do anything to stop this from failing. It may matter to report this failure, but no other meaningful action can be taken. \$\endgroup\$ – Konrad Rudolph Feb 2 '11 at 14:17
  • 3
    \$\begingroup\$ @Konrad. Agree you can't stop it failing (but that's true with many exceptions). If the user selects "Save..." in a GUI app, and close() throws, then notifying the user and allowing him to re-try the save (maybe to a different volume) is probably very meaningful... \$\endgroup\$ – Roddy Feb 2 '11 at 15:11
  • \$\begingroup\$ @Roddy: granted. But something different: does this error actually occur? I can’t remember ever seeing something like that (except when you prematurely plug off a USB drive etc. …). \$\endgroup\$ – Konrad Rudolph Feb 2 '11 at 15:16
  • 2
    \$\begingroup\$ @Roddy: Of course there are times when it does matter and you want ot report it (GUI applications). then you would use close() explicitly and check the error. Weather you care or not is totally situational and only the dev can decide that at the point of usage. In the above example it does not sound like any checking was done and thus they did not care. \$\endgroup\$ – Martin York Feb 2 '11 at 17:19
39
\$\begingroup\$

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.

\$\endgroup\$
  • \$\begingroup\$ Sorry, your assumption is correct, I forgot to mention that the fstream is local to the function. \$\endgroup\$ – dreamlax Feb 2 '11 at 5:24
  • \$\begingroup\$ I feel a little bit like asking "what's the point in having destructors if we're doing the work manually..." but I know it won't go far. I put the explicit call in just to satisfy the code review but I felt extremely reluctant. \$\endgroup\$ – dreamlax Feb 2 '11 at 6:41
  • \$\begingroup\$ @dreamlax: Its to give you flexibility. In most cases you don't care if the close() works or not (there is nothing you can do about it (except log the information)) so the destructor works perfectly. But in the situations were you do care you have the option of calling close() manually and checking to see if the close worked (and if not sending that 2AM SMS to support telling them the file system is down and needs immediate replacement). \$\endgroup\$ – Martin York Feb 2 '11 at 8:09
  • \$\begingroup\$ re. Exceptions: The next (il)logical step is to put a try/catch construct in so that the unnecessary 'close()' function still gets called if an exception is throw.... \$\endgroup\$ – Roddy Feb 2 '11 at 10:54
  • \$\begingroup\$ One reason that the project manager gave was that under stressed conditions where file handles are scarce, we need to close files as soon as we're done with them. By creating a rule that you close every stream, regardless of how trivial the function, you force the developer to think about the lifetime of the object. \$\endgroup\$ – dreamlax Feb 3 '11 at 2:54
17
\$\begingroup\$

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.

\$\endgroup\$
  • \$\begingroup\$ There shouldn't be any "just got lucky" w.r.t. resources in C++. There should be "just works". That's how RAII is designed, and how it works when used properly. \$\endgroup\$ – Sebastian Redl Feb 24 '14 at 16:28
  • 5
    \$\begingroup\$ I disagree. This is C++, if you need a comment reminding you that an fstream will close on its own, then you're not a C++ programmer. \$\endgroup\$ – jliv902 Mar 25 '14 at 22:30
10
\$\begingroup\$

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.

\$\endgroup\$
9
\$\begingroup\$

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.

\$\endgroup\$
  • 9
    \$\begingroup\$ The problem with small group consensus (i.e. development teams). It can often be wrong because the group is swayed by the loudest voice. For group consensus to work the group has to be large enough so that loud individuals can be counteracted by the correct decision of a lot of people (this is why SO works (loud people may get an initial vote up but eventually the correct answer usually get the votes (eventually))). \$\endgroup\$ – Martin York Feb 2 '11 at 7:57
  • 1
    \$\begingroup\$ @Martin: Nice nested parenthesis. \$\endgroup\$ – GManNickG Feb 3 '11 at 0:32
  • \$\begingroup\$ @grokus I agree with Tux-D's sentiment. But, I don't believe in design/review by committee. If someone has the power to dictate these types of standards, they need to be shown another way or proven incorrect. The strategy for this will completely depend on the individual. The only thing that is going to happen in large meetings is arguing, and eventual increase in volume unti a mediator steps in and makes the decision for you. The best response to this is to construct an app in which .close() is called and throws. Show it kills the app. Then give them a choice, handle all errors or use RAII. \$\endgroup\$ – Andrew T Finnell Sep 4 '11 at 13:51
5
\$\begingroup\$

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.

\$\endgroup\$
2
\$\begingroup\$

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.

\$\endgroup\$
1
\$\begingroup\$

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.

\$\endgroup\$

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

Not the answer you're looking for? Browse other questions tagged or ask your own question.