Will the Database connection be closed if we yield the datareader row and not read all the records?

Question

While understanding how yield keyword works, I came across link1 and link2 on StackOverflow which advocates the use of yield return while iterating over the DataReader and it suits my need as well. But It makes me wonder as what happens, if I use yield return as shown below and if I don't iterate through entire DataReader, will the DB connection stay open forever?

IEnumerable<IDataRecord> GetRecords()
{
    SqlConnection myConnection = new SqlConnection(@"...");
    SqlCommand myCommand = new SqlCommand(@"...", myConnection);
    myCommand.CommandType = System.Data.CommandType.Text;
    myConnection.Open();
    myReader = myCommand.ExecuteReader(CommandBehavior.CloseConnection);
    try
       {               
          while (myReader.Read())
          {
            yield return myReader;          
          }
        }
    finally
        {
            myReader.Close();
        }
}


void AnotherMethod()
{
    foreach(var rec in GetRecords())
    {
       i++;
       System.Console.WriteLine(rec.GetString(1));
       if (i == 5)
         break;
  }
}

I tried the same example in a sample Console App and noticed while debugging that the finally block of GetRecords() is not executed. How can I ensure then the closure of DB Connection? Is there a better way than using yield keyword? I am trying to design a custom class which will be responsible for executing select SQLs and stored procedures on DB and will return the result. But I don't want to return the DataReader to the caller. Also I want to make sure that the connection will be closed in all scenarios.

Edit Changed the answer to Ben's answer as it is incorrect to expect method callers to use the method correctly and with respect to DB connection it will be more expensive if the method is called multiple times for no reason.

Thanks Jakob and Ben for detailed explanation.

Answer 1

Yes, you will face the problem you describe: until you finish iterating over the result, you will keep the connection open. There's two general approaches I can think of to deal with this:

Push, don't pull

Currently you're returning an IEnumerable<IDataRecord>, a data structure you can pull from. Instead, you could switch your method to push its results out. The simplest way would be to pass in an Action<IDataRecord> which is called on each iteration:

void GetRecords(Action<IDataRecord> callback)
{
    // ...
      while (myReader.Read())
      {
        callback(myReader);
      }
}

Note that given you're dealing with a collection of items, IObservable/IObserver might be a slightly more appropriate data structure, but unless you need it, a simple Action is much more straightforward.

Eagerly Evaluate

An alternative is just to make sure the iteration entirely completes before returning.

You can normally do this by just putting the results in a list then returning that, but in this case there's the additional complication of each item being the same reference to the reader. So you need something to extract the result you need from the reader:

IEnumerable<T> GetRecords<T>(Func<IDataRecord,T> extractor)
{
    // ...
     var result = new List<T>();
     try
     {
       while (myReader.Read())
       {
         result.Add(extractor(myReader));         
       }
     }
     finally
     {
         myReader.Close();
     }
     return result;
}

Answer 2

Your finally block will always execute.

When you use yield return the compiler will create a new nested class to implement a state machine.

This class will contain all code from finally blocks as separate methods. It will keep track of the finally blocks that need to be executed depending on the state. All necessary finally blocks will be executed in Dispose method.

According to C# language specification, foreach (V v in x) embedded-statement is expanded to

{
    E e = ((C)(x)).GetEnumerator();
    try
    {
        while (e.MoveNext())
        {
            V v = (V)(T)e.Current;
            embedded - statement
        }
    }
    finally {
        … // Dispose e
    }
}

so the enumerator will be disposed even if you exit the loop with break or return.

To get more information about iterator implementation you can read this this article by Jon Skeet

Edit

The problem with such approach is that you rely on the clients of your class to use the method correctly. They could for example get the enumerator directly and iterate through it in a while loop without disposing it.

You should consider one of the solutions suggested by @BenAaronson.

Answer 3

Regardless of the specific yield behaviour, your code contains execution paths that will not dispose your resources correctly. What if your second line throws an exception or your third? Or even your fourth? You will need a very complex try/finally chain, or you can use using blocks.

IEnumerable<IDataRecord> GetRecords()
{
    using(var connection = new SqlConnection(@"..."))
    {
        connection.Open();

        using(var command = new SqlCommand(@"...", connection);
        {
            using(var reader = command.ExecuteReader())
            {
               while(reader.Read())
               {
                   // your code here.
                   yield return reader;
               }
            }
        }
    }
}

People have remarked that this is not intuitive, that people calling your method may not know that enumerating the result twice will call the method twice. Well, tough luck. That's the way the language works. That's the signal that IEnumerable<T> sends. There is a reason this does not return List<T> or T[]. People that do not know this need to be educated, not worked around.

Visual Studio has a feature called static code analysis. You can use it to find out if you have disposed resources properly.

Answer 4

What you did seems wrong, but will work fine.

You should use an IEnumerator<>, as it also inherits IDisposable.

But because you are using a foreach, the compiler is still using an IDisposable to generate an IEnumerator<> for the foreach.

in facts, the foreach involves a lot of thing internaly.

Check https://stackoverflow.com/questions/13459447/do-i-need-to-consider-disposing-of-any-ienumerablet-i-use