14

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.

  • From what I see you are not opening the connection in the first place – paparazzo Nov 22 '15 at 15:58
  • @Frisbee Edited :) – GawdePrasad Nov 23 '15 at 7:42
12

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;
}
  • I am using the approach you named Eagerly Evaluate. Now will it be a memory overhead if my SQLCommand's Datareader returns multiple outputs (like myReader.NextResult()) and I construct a list of list? Or sending the datareader to the caller [I personally don't prefer it] will be much efficient? [but the connection will be open longer]. What I am precisely confused here is between the tradeoff of keeping the connection open longer than creating a list of list. You can safely assume that there will be close to 500+ people visiting my webpage which connects to DB. – GawdePrasad Oct 21 '15 at 9:41
  • 1
    @GawdePrasad It depends what the caller would do with the reader. If it would just put items in a collection itself, there's no significant overhead. If it'd do an operation that doesn't require keeping the whole lot of values in memory, then there's a memory overhead. However unless you're storing very large amounts, it's probably not an overhead you should care about. Either way, there shouldn't be a significant time overhead. – Ben Aaronson Oct 21 '15 at 10:50
  • How does the "push, don't pull" approach solves the "until you finish iterating over the result, you will keep the connection open" issue? The connection is still kept open until you finish iterating even with this approach, isn't it? – BornToCode Jul 25 '16 at 15:06
  • 1
    @BornToCode See the question: "if I use yield return as shown below and if I don't iterate through entire DataReader, will the DB connection stay open forever". The problem is that you return an IEnumerable and every caller who handles it has to be aware of this special gotcha: "If you don't finish iterating over me, I'll hold open a connection". In the push method, you take that responsibility away from the caller- they have no option to partially iterate. By the time control is returned to them, the connection is closed. – Ben Aaronson Jul 25 '16 at 15:58
  • 1
    This is an excellent answer. I like the push approach, because if you have a big query that you are presenting in a paged fashion, you can abort the read earlier for speed by passing in a Func<> rather than an Action<>; this feels cleaner than making the GetRecords function also do paging. – Whelkaholism Feb 6 '18 at 11:10
10

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.

  • 1
    This is extremely unreliable. For example: var records = GetRecords(); var first = records.First(); var count = records.Count() will actually execute that method twice, opening and closing the connection and reader each time. Iterating over it twice does the same thing. You might also pass the result around for a long time before actually iterating over it, etc. Nothing in the method signature implies that kind of behaviour – Ben Aaronson Oct 19 '15 at 12:17
  • 2
    @BenAaronson I focused on the specific question about the current implementation in my answer. If you look at the whole problem, I agree it's much better to use the way you suggested in your answer. – Jakub Lortz Oct 19 '15 at 12:36
  • 1
    @JakubLortz Fair enough – Ben Aaronson Oct 19 '15 at 12:47
  • 2
    @EsbenSkovPedersen Usually when you try to make something idiot-proof, you lose some functionality. You need to balance it out. Here we don't lose much by eagerly loading the results. Anyway, the biggest problem for me is the naming. When I see a method named GetRecords returning IEnumerable, I expect it to load the records to memory. On the other hand, if it was a property Records, I would rather assume it's some kind of abstraction over database. – Jakub Lortz Oct 19 '15 at 19:02
  • 1
    @EsbenSkovPedersen Well, see my comments on nvoigt's answer. I have no problem in general with methods returning an IEnumerable that will do significant work when iterated over, but in this case it doesn't seem to fit the implications of the method signature – Ben Aaronson Oct 20 '15 at 8:17
5

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.

  • The language allows iterating over IEnumerable to do all sorts of things, such as throwing totally unrelated exceptions or writing 5GB files to disk. Just because the language allows it doesn't mean that it's acceptible to return an IEnumerable that does it from a method which gives no indication that that will happen. – Ben Aaronson Oct 19 '15 at 15:30
  • 1
    Returning an IEnumerable<T> is the indication that enumerating it twice will result in two calls. You will even get helpful warnings in your tools if you enumerate an enumerable multiple times. The point about throwing exceptions is equally valid regardless of return type. If that method would return a List<T> it would throw the same exceptions. – nvoigt Oct 19 '15 at 15:48
  • I do understand your point and to some extent I agree- if you're consuming a method that gives you an IEnumerable then caveat emptor. But I also think that as a method writer, you have a responsibility to adhere to what your signature implies. The method is called GetRecords, so when it returns you'd expect it to have, y'know, gotten the records. If it was called GiveMeSomethingICanPullRecordsFrom (or, more realistically GetRecordSource or similar) then a lazy IEnumerable would be more acceptable. – Ben Aaronson Oct 19 '15 at 16:09
  • @BenAaronson I agree that for example in File, ReadLines and ReadAllLines can be easily told apart and that's a good thing. (Although it's more because of the fact that a method overload cannot differ by return type only). Maybe ReadRecords and ReadAllRecords would fit here as naming scheme as well. – nvoigt Oct 19 '15 at 16:37
2

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

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.