Bug in the File.ReadLines(..) method of the .net framework 4.0

Question

This code :

IEnumerable<string> lines = File.ReadLines("file path");
foreach (var line in lines)
{
    Console.WriteLine(line); 
}
foreach (var line in lines)
{ 
    Console.WriteLine(line); 
} 

throws an ObjectDisposedException : {"Cannot read from a closed TextReader."} if the second foreach is executed. It seems that the iterator object returned from File.ReadLines(..) can't be enumerated more than once. You have to obtain a new iterator object by calling File.ReadLines(..) and then use it to iterate.

If I replace File.ReadLines(..) with my version(parameters are not verified, it's just an example):

public static IEnumerable<string> MyReadLines(string path)
{
    using (var stream = new TextReader(path))
    {
        string line;
        while ((line = stream.ReadLine()) != null)
        {
            yield return line;
        }
    }
}

it's possible to iterate more than once the lines of the file.

An investigation using .Net Reflector showed that the implementation of the File.ReadLines(..) calls a private File.InternalReadLines(TextReader reader) that creates the actual iterator. The reader passed as a parameter is used in the MoveNext() method of the iterator to get the lines of the file and is disposed when we reach the end of the file. This means that once MoveNext() returns false there is no way to iterate a second time because the reader is closed and you have to get a new reader by creating a new iterator with the ReadLines(..) method.In my version a new reader is created in the MoveNext() method each time we start a new iteration.

Is this the expected behavior of the File.ReadLines(..) method?

I find troubling the fact that it's necessary to call the method each time before you enumerate the results. You would also have to call the method each time before you iterate the results of a Linq query that uses the method.

Answer 1

I know this is old, but i actually just ran into this while working on some code on a Windows 7 machine. Contrary to what people were saying here, this actually was a bug. See this link.

So the easy fix is to update your .net framefork. I thought this was worth updating since this was the top search result.

Answer 2

I don't think it's a bug, and I don't think it's unusual -- in fact that's what I'd expect for something like a text file reader to do. IO is an expensive operation, so in general you want to do everything in one pass.

Answer 3

It isn't a bug. But I believe you can use ReadAllLines() to do what you want instead. ReadAllLines creates a string array and pulls in all the lines into the array, instead of just a simple enumerator over a stream like ReadLines does.

Answer 4

If you need to access the lines twice you can always buffer them into a List<T>

using System.Linq;

List<string> lines = File.ReadLines("file path").ToList(); 
foreach (var line in lines) 
{ 
    Console.WriteLine(line);  
} 
foreach (var line in lines) 
{  
    Console.WriteLine(line);  
} 

Answer 5

I don't know if it can be considered a bug or not if it's by design but I can certainly say two things...

1. This should be posted on Connect, not StackOverflow although they're not going to change it before 4.0 is released. And that usually means they won't ever fix it.
2. The design of the method certainly appears to be flawed.

You are correct in noting that returning an IEnumerable implies that it should be reusable and it does not guarantee the same results if iterated twice. If it had returned an IEnumerator instead then it would be a different story.

So anyway, I think it's a good find and I think the API is a lousy one to begin with. ReadAllLines and ReadAllText give you a nice convenient way of getting at the entire file but if the caller cares enough about performance to be using a lazy enumerable, they shouldn't be delegating so much responsibility to a static helper method in the first place.

Answer 6

I believe you are confusing an IQueryable with an IEnumerable. Yes, it's true that IQueryable can be treated as an IEnumerable, but they are not exactly the same thing. An IQueryable queries each time it's used, while an IEnumerable has no such implied reuse.

A Linq Query returns an IQueryable. ReadLines returns an IEnumerable.

There's a subtle distinction here because of the way an Enumerator is created. An IQueryable creates an IEnumerator when you call GetEnumerator() on it (which is done automatically by foreach). ReadLines() creates the IEnumerator when the ReadLines() function is called. As such, when you reuse an IQueryable, it creates a new IEnumerator when you reuse it, but since the ReadLines() creates the IEnumerator (and not an IQueryable), the only way to get a new IEnumerator is to call ReadLines() again.

In other words, you should only be able to expect to reuse an IQueryable, not an IEnumerator.

EDIT:

On further reflection (no pun intended) I think my initial response was a bit too simplistic. If IEnumerable was not reusable, you couldn't do something like this:

List<int> li = new List<int>() {1, 2, 3, 4};

IEnumerable<int> iei = li;

foreach (var i in iei) { Console.WriteLine(i); }
foreach (var i in iei) { Console.WriteLine(i); }

Clearly, one would not expect the second foreach to fail.

The problem, as is so often the case with these kinds of abstractions, is that not everything fits perfectly. For example, Streams are typically one-way, but for network use they had to be adapted to work bi-directionally.

In this case, an IEnumerable was originally envisioned to be a reusable feature, but it has since been adapted to be so generic that reusability is not a guarantee or even should be expected. Witness the explosion of various libraries that use IEnumerables in non-reusable ways, such as Jeffery Richters PowerThreading library.

I simply don't think we can assume IEnumerables are reusable in all cases anymore.

Answer 7

It's not a bug. File.ReadLines() uses lazy evaluation and it is not idempotent. That's why it's not safe to enumerate it twice in a row. Remember an IEnumerable represents a data source that can be enumerated, it does not state it is safe to be enumerated twice, although this might be unexpected since most people are used to using IEnumerable over idempotent collections.

From the MSDN:

The ReadLines(String, System) and ReadAllLines(String, System) methods differ as follows: When you use ReadLines, you can start enumerating the collection of strings before the whole collection is returned; when you use ReadAllLines, you must wait for the whole array of strings be returned before you can access the array.Therefore, when you are working with very large files, ReadLines can be more efficient.

Your findings via reflector are correct and verify this behavior. The implementation you provided avoids this unexpected behavior but makes still use of lazy evaluation.