17

If I recall correctly that when I used yield inside using SqlConnection blocks I got runtime exceptions.

using (var connection = new SqlConnection(connectionString))
{
    var command = new SqlCommand(queryString, connection);
    connection.Open();

    SqlDataReader reader = command.ExecuteReader();

    // Call Read before accessing data.
    while (reader.Read())
    {
        yield reader[0];
    }

    // Call Close when done reading.
    reader.Close();
}

Those problems were solved when I replaced yield by a List where I added items each iteration.

The same problem didn't happen yet to me when inside using StreamReader blocks

using (var streamReader = new StreamReader(fileName))
{
    string line;
    while ((line = streamReader.ReadLine()) != null)
    {
        yield return line;
    }
}

Is there any explanation why Exceptions happened in the former case and not in the latter? Is this construction advisable?

EDIT To get the error (early disposal) that I did in the past you should call the first method below:

IEnumerable<string> Read(string fileName)
{
    using (var streamReader = new StreamReader(fileName))
    {
        return Read(streamReader);
    } // Dispose will be executed before ReadLine() because of deffered execution
}

IEnumerable<string> Read(StreamReader streamReader)
{
    string line;
    while ((line = streamReader.ReadLine()) != null)
    {
        yield return line;
    }
}

The same error can be achieved with other ways of deferring execution, such as System.Linq.Enumerable.Select()

Jader Dias
  • 88,211
  • 155
  • 421
  • 625

2 Answers2

5

See this post for a good explanation of the issues with using and yield. Because you return in enumerator, the using block will already have destroyed the context before anything is accessed. The answers have good solutions, basically, either make the wrapper method an enumerator, or build a list instead.

Also it's usually more practical to have using around the reader, not the connection, and use CommandBehavior.CloseConnection to ensure resources are released when the reader's done. Though it doesn't really matter in your situation, if you ever return a data reader from a method, this will ensure the connection is closed properly when the reader is disposed.

   using(SqlDataReader reader = 
             command.ExecuteReader(CommandBehavior.CloseConnection)) {
        while (reader.Read())
        {
            yield reader[0];
        }
   }
Community
  • 1
  • 1
Jamie Treworgy
  • 23,934
  • 8
  • 76
  • 119
  • You're right. `yield return`s can run into early disposals if the `using` directive is not in the same method. This is probably the exception I vaguely recalled when I posted this question. – Jader Dias Apr 19 '11 at 12:47
  • 3
    In this situation I would apply `using` to both the Reader _and_ the Connection. – H H Apr 19 '11 at 12:56
  • 1
    I agree it is best practice, but SQL connections are kind of a special case because it is a normal practice (and supported by `CommandBehavior.CloseConnection`) to create a data reader and pass it outside the context in which it was created. `Dispose` on a connection just calls `Close` (and erases the connection string). So in that sense it's not really necessary to use `using` around connections, and the way many DB management subsystems are created, it wouldn't be practical. – Jamie Treworgy Apr 19 '11 at 13:08
  • 1
    As a rule of thumb I should say: Never accept `IDisposable` parameters in methods that use `yield return` or other deferring execution methods – Jader Dias Apr 19 '11 at 13:19
2

The compiler should handle the yield inside the using block correctly in both cases. There's no obvious reason why it should throw an exception.

One thing to be aware of is that the connection will only be disposed once you've completed iterating and/or manually disposed the enumerator object. If you expose this code in a public method then it's possible that stupid or malicious code could keep your connection open for a long time:

 var enumerable = YourMethodThatYieldsFromTheDataReader();
 var enumerator = enumerable.GetEnumerator();
 enumerator.MoveNext();
 Thread.Sleep(forever);    // your connection will never be disposed
LukeH
  • 263,068
  • 57
  • 365
  • 409