Should you manually close an fstream?

Originally Posted by King Mir

This is not always possible. consider:

Code:

{
  ifstream file1 ("file1.txt");
  //read from file1 here
  ifstream file2 ("file2.txt");
  //use file1 and file2 here
  file1.close();
  //keep using file 2 here
}

Now you could do this:

Code:

ifstream file2
{
  ifstream file1 ("file1.txt");
  //read from file1 here
  file2.open("file2.txt");
  //use file1 and file2 here
}
//keep using file 2 here

But that vialates RAII too.

Streams are the example, but they are not the only resouse that could encounter this problem.

Although this problem isn't that common, I don't think it's invariably a design problem when it does come up.

Sorry, but I agree with whiteflags that your example is contrived, since you have left out any consideration of the lifetime of file2. Your examples can easily be reorganised as

Code:

void FunctionToGiveAnAdditionalContainingScope()
{
     ifstream file2;

    {    // begin the scope as per your example
         ifstream file1 ("file1.txt");
          //read from file1 here
          file2.open("file2.txt");
          //use file1 and file2 here
     }
        // use file2 here
}    // file2 stream's destructor invoked here, so the stream will be closed.

I would probably organise the enclosed scope into its own function as well, but it is not technically necessary.

You are certainly correct that programs don't use one resource at at time, and release each resource before using another one. However, every resource is associated with a lifetime, even if lifetimes of distinct resources overlap. There is a point before which the resource is not needed. There is a point at which it is no longer needed. Those two points can be associated with the begin and end of a scope.

Even if a resource has infinite lifetime, it can be associated with a scope. For example, a program that never terminates.

Code:

int main()
{
     Resource some_resource;
     bool reality_exists = true;

     do
     {
            // use some_resource
     }
     while (reality_exists);
}

Short of some Armageddon event which (as far as the program is concerned) destroys all reality, some_resource will continue to exist. But it is still associated with a scope.

The first bit of code you posted is in violation of RAII ideals, because you're using 2-stage construction, which is about as bad as 2-stage destruction ala close. It's more intuitive to do it that way in some contexts, I agree, but I don't see why it is idiomatically or generally better. Why should 2 stage construction be preferred over 2-stage destruction?

This is not actually a 2-stage destruction. Closing a stream is a change of stream state, not a phase of destruction.

Technically, I could have changed the first code sample to

Code:

void FunctionToGiveAnAdditionalContainingScope()
{
     ifstream file2("file2.txt");
 
    {    // begin the scope as per your example
         ifstream file1 ("file1.txt");
          //use file1 and file2 here
     }
        // use file2 here
}    // file2 stream's destructor invoked here, so the stream will be closed.

but decided I didn't want to get into the debate if you were to adjust the example so the filename for file2 became derived from data in file1.

Incidentally, I just realised I could have also used the rule that "automatic destruction of objects due to scope is in the reverse order of their construction" and produced this

Code:

void FunctionToGiveAnAdditionalContainingScope()
{
     ifstream file1 ("file1.txt");
        // read data from file1
     ifstream file2(some_filename_derived_from_data_read_from_file1);
       //use file1 and file2 here

     file1.close();    //   this is strictly optional

      // use file2 here
}    // file2 stream's destructor invoked here, then file1's destructor, so the streams will be closed if not already closed.

Originally Posted by grumpy

This is not actually a 2-stage destruction. Closing a stream is a change of stream state, not a phase of destruction.

Closing a stream is resource release. Opening a stream is resource acquisition. Delaying opening a stream violates Resource Acquisition Is Initialization, trivially. If RAII is to imply that resource release is destruction, which it reasonably should, then releasing a resource, such as by closing a stream, violates RAII too.

Yet it's not a fault of design that resource lifetimes are sometimes intertwined like that.

Incidentally, I just realised I could have also used the rule that "automatic destruction of objects due to scope is in the reverse order of their construction" and produced this

Code:

void FunctionToGiveAnAdditionalContainingScope()
{
     ifstream file1 ("file1.txt");
        // read data from file1
     ifstream file2(some_filename_derived_from_data_read_from_file1);
       //use file1 and file2 here

     file1.close();    //   this is strictly optional

      // use file2 here
}    // file2 stream's destructor invoked here, then file1's destructor, so the streams will be closed if not already closed.

Closing file1 is not strictly optional, since closing a file can effect whether other programs can open the file, and the number of other files the program is allowed to open by the OS. In general, resources should be released as soon as they are no longer needed, and no later.

Most bugs are common misunderstandings. If you do something smart, make sure all people in the project are at least as smart as you. So if you aren't the dumbest member of your team, don't try smart things. You aren't a team player. Maybe other people aren't actually as smart as you. Or maybe they are the worlds top genius, but their language of choice is Java or PHP or something else. The best solution to your problem at hand is the simplest solution that solves the problem.

Think about somebody else reading your code. And maybe, he's just a normal guy and doesn't know the C++ standard by heart.

Code:

std::string read_smart()
{
	// ok, he has a text
	std::string result;

	// what's this? did he delete an if and forgot the braces?
	{
		// wtf is an if stream? does it stream if... if what? maybe that's why the if was missing?
		std::ifstream ifs("somefile.txt");

		// and now he bitshifts the maybe streaming thing by the text?
		ifs >> result;
	}

	// somehow, the file landed in here... I'm totally confused.
	return result;
}

std::string read_simple()
{
	// ok, he has a file.
	std::ifstream file;

	// and it's contents
	std::string file_content;

	// he opens the file
	file.open("somefile.txt");

	// and bitshifts? hm, somehow this line puts the file into the file_content... C++ is wierd
	file >> file_content;

	// he closes the file
	file.close();

	// now that was easy to understand
	return file_content;
}

Yeah sometimes I see code like this, and have a little chuckle:

Code:

ifstream file("somefile.txt");

string fileContent;

if ( file.is_open() ) 
{  // start if
    
   file >> fileContent;

} //end if

return fileContent;

But the comments there can be useful, say if the block stretches past what you can see on the screen. I think a comment explaining the extra block you put in is required, because it could answer many questions about it.

Originally Posted by nvoigt

Or maybe they are the worlds top genius, but their language of choice is Java or PHP or something else. The best solution to your problem at hand is the simplest solution that solves the problem.

Think about somebody else reading your code. And maybe, he's just a normal guy and doesn't know the C++ standard by heart.

My problem with that argument is that if you want to program for the benefit of such a person who lacks knowledge of a given library component that is used and does not bother to learn about it before jumping to a conclusion based on his/her existing knowledge of components from a different language or library, then you're going to have a pretty hard time doing anything that is idiomatically C++.

For example, your example has a latent bug: what happens if an exception is thrown before the close member function is called, e.g., an exception propagated from some std::string related function that is called when reading into the string variable? Clearly, we should catch the exception and then close the file, otherwise the file would not be closed when the ifstream object is destroyed. Oh, and it looks like C++ is deficient in its exception handling: a finally clause would help here. RAII? What's that? C++ is weird.

Originally Posted by King Mir

Closing file1 is not strictly optional, since closing a file can effect whether other programs can open the file, and the number of other files the program is allowed to open by the OS. In general, resources should be released as soon as they are no longer needed, and no later.

In either ways the file is closed, no?

I understand that leaving a file open is bad where not deallocating memory as soon as possible usually not. So you can consider that a file should need to manually be handled. But the alternative to that is to teach programmers to use a file strictly when they need it. So you can start your program as

Code:

std::string current_filename;
std::ifstream current_stream;
std::string fbuffer;
....
//a 100 lines
...
current_stream.open(current_filename);
...
//read into fbuffer
...
//a 100 lines
...
current_stream.close();
....
...

but I would prefer if you designed your program as

Code:

std::string current_filename;
std::string fbuffer;
....
//a 100 lines
....
{
  std::ifstream current_stream(current_filename);
  ...
  //read into fbuffer
  ...
}
...
...

because then I know exactly where the file is used and how long the file is open. The compiler doesn't keep track of open() and close() statements to throw a "fstream never closed" warning or "reading from fstream while it was already closed".

Originally Posted by King Mir

Closing a stream is resource release. Opening a stream is resource acquisition. Delaying opening a stream violates Resource Acquisition Is Initialization, trivially. If RAII is to imply that resource release is destruction, which it reasonably should, then releasing a resource, such as by closing a stream, violates RAII too.

The thing is, there are two types of resource here.

The first is the ifstream itself, which is all our code should worry about. As long as we manage the lifetime of that ifstream appropriately, we have met our responsibility for resource management.

The second is the resources that the ifstream itself manages. Our code should not need to worry about managing those ifstream-internal resources at all, and if we need to that is a design problem with the ifstream class. If we can rely on an ifstream appropriately managing its internal resources, and we manage the lifecycle of the stream object, we need do no more.

Originally Posted by King Mir

Yet it's not a fault of design that resource lifetimes are sometimes intertwined like that.

It is unnecessary effort for a programmer to worry about managing a resource that is internal to the ifstream class though.

I agree that resource lifetimes can overlap. However, if managing the lifecycle of one resource (an ifstream) completely manages the lifecycle or state of some other resource (the ifstream internals) then we don't need to manage that anything related to that "other resource".

Originally Posted by King Mir

Closing file1 is not strictly optional, since closing a file can effect whether other programs can open the file, and the number of other files the program is allowed to open by the OS. In general, resources should be released as soon as they are no longer needed, and no later.

As far as the user of file1 is concerned, closing the stream is optional, since we manage its lifecycle.

The fact that we can deduce that ifstream manages a resource internally does not mean we need to manage that internal resource.

In any event, you've just made your example more contrived. On one hand, you have a need for both streams to be open. But you need to close one of them to conserve resources, before you've finished with the other? Any design which is that fragile has a good chance of breaking before you can close file1.

Originally Posted by laserlight

My problem with that argument is that if you want to program for the benefit of such a person who lacks knowledge of a given library component that is used and does not bother to learn about it before jumping to a conclusion based on his/her existing knowledge of components from a different language or library, then you're going to have a pretty hard time doing anything that is idiomatically C++.

For example, your example has a latent bug: what happens if an exception is thrown before the close member function is called, e.g., an exception propagated from some std::string related function that is called when reading into the string variable? Clearly, we should catch the exception and then close the file, otherwise the file would not be closed when the ifstream object is destroyed. Oh, and it looks like C++ is deficient in its exception handling: a finally clause would help here. RAII? What's that? C++ is weird.

I agree that if we dumb down C++ enough then we end up using "C with classes" and nobody wants that. C++ has some neccessarily complicated ways of getting stuff done. However, I don't think this is one of it. If the goal is to close a file, then it really doesn't get simpler than file.close(); If someone opens up an extra scope because he knows that losing scope will call close behind the scenes, then I consider this unneccessarily complicated.

As far as I can see, both of my example codes, if an exception is thrown, will destruct and therefor close the file. Both file streams are contained in a scope that would be left with an exception. I'm not expecting anyone not experienced in C++ to write C++. But it should be as easy as possible to read. If I have the choice between good and readable, and great and unreadable code, I'd pick the former.

Originally Posted by nvoigt

I agree that if we dumb down C++ enough then we end up using "C with classes" and nobody wants that. C++ has some neccessarily complicated ways of getting stuff done. However, I don't think this is one of it. If the goal is to close a file, then it really doesn't get simpler than file.close(); If someone opens up an extra scope because he knows that losing scope will call close behind the scenes, then I consider this unneccessarily complicated.

While I agree with you at some points, I wouldn't consider an extra scope unnecessarily complicated. This idiom is all the rage right now and is being incorporated into other languages. C# already has the using statement which is just using + a scope, and there are talks of implementing something similar into Java.
I consider an extra scope something of the norm, rather than something of the odd. If a programmer does not know of this idiom, then I dare say they have failed to grasp an important concept which they should know. We should not make brittle code (or adapt our code) to these programmers. They should adapt to the normal idioms used in the programming world.
Well, that's my opinion on the matter, anyway.

C# already has the using constructor which is just using + a scope

The main difference and my only problem with the same idiom in C++ is that in C# the using construct is a very clear communication of intent. I'm using( ... ) a specific variable and I want it gone when the block ends. In C++ I have a scope... and the programmer is left guessing which variable in the scope I wanted to be gone... if any. I have seen a lot of useless scopes (deleted ifs with braces left, for example) that were the result of laziness or sloppy programming so I'd prefer clean communication.

I very rarely, in practice, use a naked pair of braces to introduce a new scope. Except in occasional forum discussions ....

Generally speaking, if I want to introduce an additional scope, I'll create a new function to provide it. Particularly if I can give a name to the function that communicate what it achieves.

If using C++-11 and feeling like doing something a little funky, I might even do it with a lambda

Originally Posted by nvoigt

I agree that if we dumb down C++ enough then we end up using "C with classes" and nobody wants that. C++ has some neccessarily complicated ways of getting stuff done. However, I don't think this is one of it. If the goal is to close a file, then it really doesn't get simpler than file.close(); If someone opens up an extra scope because he knows that losing scope will call close behind the scenes, then I consider this unneccessarily complicated.

I agree if you want to close a file you call close(). When you want to open it you call open(). Where I disagree is that if you just want to read from a file you need to manually open(), read() and close(). You also don't need to explain to someone how resources are allocated/deallocated. You just explain that the file follows the same lifecycle as fstream. They should be familiar with the scope of a variable so this is not something different.

Note that the only reason you bother adding brackets is because you are adding the requirement "close the file as soon as possible". If the requirement was "don't forget to leave the file open" then you don't even need to worry about the brackets. Let us assume that I have the "don't leave the file open" requirement. If the examples I read advice to use close() I assume it is mandatory. So I will add on my project the additional step of making sure all the fstream() used in all files of my project call close(). This is an additional cost for my project.