Is it safe to reuse a SqlDataRecord?

Question

When implementing table-valued parameters, one of the most common ways to generate an IEnumerable<SqlDataRecord> for use by the parameter is code like this (e.g., https://stackoverflow.com/a/10779567/18192 ):

public static IEnumerable<SqlDataRecord> Rows(List<int> simpletable)
{
    var smd = new []{ new SqlMetaData("id", SqlDbType.Int)};
    var sqlRow = new SqlDataRecord(smd);
    foreach (int i in simpletable)
    {
        sqlRow.SetInt32(0, i);
        yield return sqlRow;
    }
}
//...
var param = sqlCmd.Parameters.AddWithValue("@retailerIDs", Rows(mydata)); 
param.SqlDbType = SqlDbType.Structured;
param.TypeName = "myTypeName";

This code does seem to work. While reusing SqlMetaData does not set off too many alarm bells, declaring the SqlDataRecord outside the foreach loop feels incredibly suspicious to me:

A mutable object is modified and then yielded repeatedly.

As an example of why this is concerning, calling var x = Rows(new[] { 100, 200}.ToList()).ToList().Dump() in LinqPad spits out 200,200. This approach seems to rely on an implementation detail (that rows are processed individually), but I don't see any documentation which promises this.

Is there some mitigating factor which renders this approach safe?

Answer 1

This approach seems to rely on an implementation detail (that rows are processed individually), but I don't see any documentation which promises this.

Is there some mitigating factor which renders this approach safe?

As user1249190 points out, Reusing SQLDataRecord is explicitly recommended in the remarks section of https://learn.microsoft.com/en-us/dotnet/api/microsoft.sqlserver.server.sqldatarecord#remarks :

This class is used together with SqlPipe to send result sets to the client from managed code stored-procedures. When writing common language runtime (CLR) applications, you should re-use existing SqlDataRecord objects instead of creating new ones every time. Creating many new SqlDataRecord objects could severely deplete memory and adversely affect performance.

Obviously this recommendation does not apply to usage across threads: The documentation also explicitly warns that "Any instance members are not guaranteed to be thread safe."

Answer 2

If you don't need it outside of the foreach loop at all, I don't see why you would want to re-use it.

I found this question Is there a reason for C#'s reuse of the variable in a foreach? which links to this answer in another question Is it better coding practice to define variables outside a foreach even though more verbose? where Jon Skeet answered saying:

There's no advantage to declaring the variables outside the loop, unless you want to maintain their values between iterations.

(Note that usually this makes no behavioural difference, but that's not true if the variables are being captured by a lambda expression or anonymous method.)

Answer 3

void Main()
{
    //This code proves that the object is being modified.
   Thing prevRow = null;
    foreach (var curRow in Rows(new List<int>() { 1, 2, 3 }))
    {
        Console.WriteLine(curRow);
        Console.WriteLine(prevRow);
        prevRow = curRow;
    }
    
    //Because the object is modified instead of a new object being returned,
    // this code does something unexpected; it returns the same object 3
    // times! Instead of three unique objects representing the values 1, 2, 3.
    var rowsAsList = Rows(new List<int>() { 1, 2, 3 }).ToList();
    foreach (var curRow in rowsAsList)
    {
        Console.WriteLine(curRow);
    }
}

public class Thing
{
    public int i;
}

IEnumerable<Thing> Rows(List<int> simpletable)
{
    // Bad: Reusing the object will cause problems. Comment out the next line to fix the bug.
    var sqlRow = new Thing() {i=-1};
    
    foreach (int x in simpletable)
    {
        // Good: Do this instead! Uncomment the following line to fix the bug.
        // var sqlRow = new Thing() {i=x};
        sqlRow.i = x;
        yield return sqlRow;
    }
}