DynamicModel#First()/Single()/Last() don't close connection

[buunguyen]

Because they all call to Query().FirstOrDefault(). Also, results of All() or Find(), if not exhaustively enumerated, will leave the connection open.

[frankhale]

How are you tracking if the connection is still left open? I'd like to test this on my machine.

[buunguyen]

I've run the debugger through it. Querying() is implemented with yield return, so the using() block won't finish until the internal enumerator is exhausted.

[frankhale]

Yep, I can reproduce this. Now to figure out a fix.

[frankhale]

What about doing this, this allows the iterator to finish and the end of the using block is reached. This is probably crazy, I don't usually find myself working on ORM's much but here is my first attempt.

In the TryInvokeMember method:

if (justOne)
{
   //return a single record
  result = Query(sql + orderBy, whereArgs.ToArray()).ToArray().FirstOrDefault();
}

I added the ToArray before the FirstOrDefault.

[buunguyen]

There seems to be a bug that when Single/First/Last/Get, instead of building a query with TOP 1, the query retrieves everything. So when ToArray() is called, it will effectively load all records matching the query (w/o the TOP 1) into memory, very inefficient. I'll submit a pull request just now.

[frankhale]

Yeah, I don't know about that. I'm just a layman helping out because there isn't many others looking here it would seem. I'd like to hear what Rob thinks of this.

[frankhale]

How about this?

//build the SQL
var justOne = op.StartsWith("First") || op.StartsWith("Last") || op.StartsWith("Get") || op.StartsWith("Single");

if (justOne)
{
  sql = "SELECT TOP 1 " + columns + " FROM " + TableName + where;

  //Be sure to sort by DESC on the PK (PK Sort is the default)
  if (op.StartsWith("Last"))
  {
    orderBy = orderBy + " DESC ";
  }

  //return a single record
  result = Query(sql + orderBy, whereArgs.ToArray()).ToArray().FirstOrDefault();
}
else
{
  //default to multiple
  sql = "SELECT " + columns + " FROM " + TableName + where;

  //return lots
  result = Query(sql + orderBy, whereArgs.ToArray());
}

[buunguyen]

Yeah, that looks good

[robconery]

Thanks for the issue report - but I'm not certain this is a bug. Yielding inside a read loop is how you stream records - to close the connection and dispose, as you say, just exit the loop. If I read your comment right, you're saying that FirstOrDefault won't exit the loop... but in all the load tests and other things I've done, I haven't run into this problem.

If you would be so kind as to give me a failing test, I'd appreciate it.

[buunguyen]

I think there are 2 issues:

1. Line 651-660: "SELECT TOP 1..." only applies for Last() but not First() and Single() => isn't this a bug?
2. It doesn't close the connection, a breakpoint in the debugger will show that. It might run well in your tests, probably because a) ADO.NET is pretty good in managing its pool and b) your tests fit pooling scenario. a) is given, but b) might not always be the case for all users of Massive. How about reducing the pool size and simulate test with different connection strings (e.g. use different credentials)? I'll be happy to give this a try when I have time.

I'm not sure what you meant by "just exit the loop". Client code of course can exit the code anytime it wants, but as long as the internal enumerator (state machine) isn't exhausted (i.e. MoveNext() isn't invoked for enumerator.Count times), the loop wrapping yield return won't finish and Dispose() won't be called. Do I miss anything?

[frankhale]

I can confirm that setting a breakpoint at the end of the using statement will not be reached when debugging. Since we only care about one record I added a .ToArray() after the Query method call and that causes the end of the using statement to be reached. Is there a better way to do this?

You can see the full listing above.

//return a single record
result = Query(sql + orderBy, whereArgs.ToArray()).ToArray().FirstOrDefault();

[johnmbaughman]

Rob,

Here's what I posted on #129 (which was closed and I now see this is related)...

var model = new DynamicModel("database");
dynamic table1 = model.Query("INSERT INTO table1(field1, field2) VALUES (@0, @1); SELECT @@IDENTITY AS NewId FROM table1;",1, 2).FirstOrDefault();
for ((int i = 0; i < 10; i++) {
    model.Query("INSERT INTO table2 (field1, field2) VALUES (@0, @1);", 1, i);
}

Basically, nothing happens in the second query.

And... If I add .FirstOrDefault() to the second query, to try and return the @@IDENTITY for instance, I get that model.Query() is now missing that method.

I was able to solve my issue with this by using the following (which is also on #129):

And just as soon as I asked it I solved it (at least for myself):

var model = new DynamicModel("database", tableName: "table1");    
dynamic table1 = model.Insert(new { field1 = 1, field2 = 2 });
var model1 = new DynamicModel("database", tableName: "table2");
for ((int i = 0; i < 10; i++) {
    model2.Insert( new {field1 = 2, field2 = i} );
}

I still see a potential issue here, but the work around is pretty nice. Thanks for reading; now, hopefully, someone else can learn from my "mistake"

It, logically, appears that the first instance is now only available for its own query, until that query is done. Since the object can only handle one query at a time, the second one needs to be in a separate object and connection.

Oddly enough, I was able to get .Single() to work on the first model in another example...

Just my $.02...

[robconery]

Gents:

I appreciate what you're trying to get across, that "setting break points" and so forth "show that there's a bug". This isn't reproducing a bug in an execution context.

The basic premise here is that using First(), Last() etc will leave the connection open. I understand that. It's also true that streaming anything from inside a yield will leave the connection open - only to be disposed when the routine is disposed or collected.

Which happens at some point, always.

Now we switch gears to "this isn't happening instantly" - which I believe is what you're expressing here: that you don't want to have First() leave a connection open while another is established. Now we're at the point where I'm asking: have you seen this happen.

Again: I've run load tests and I don't have connection issues. I understand that you think there should be connection issues. I thank you for what you think. I need to confirm this and not just take your word and "well obviously" comments (with all due respect).

To that end: I need a failing bit of code, or better yet, a failing test. I don't want you to solve the problem for me, I don't need further explanation and "this is clearly going to fail" premises.

I need failing code.

[robconery]

(hit the wrong button)

[johnmbaughman]

Mine isn't with those, it's with Query directly and the LINQ functions off that such as FirstOrDefault(). My comment shows that, and not setting a break point. ;)

I figured out a workaround and am happy with that as the issue with Query is rather inherent in LINQ and how it pulls data (I think...)

-John Baughman

On Jul 18, 2012, at 11:34 PM, Rob Coneryreply@reply.github.com wrote:

Gents:

I appreciate what you're trying to get across, that "setting break points" and so forth "show that there's a bug". This isn't reproducing a bug in an execution context.

The basic premise here is that using First(), Last() etc will leave the connection open. I understand that. It's also true that streaming anything from inside a yield will leave the connection open - only to be disposed when the routine is disposed or collected.

Which happens at some point, always.

Now we switch gears to "this isn't happening instantly" - which I believe is what you're expressing here: that you don't want to have First() leave a connection open while another is established. Now we're at the point where I'm asking: have you seen this happen.

Again: I've run load tests and I don't have connection issues. I understand that you think there should be connection issues. I thank you for what you think. I need to confirm this and not just take your word and "well obviously" comments (with all due respect).

To that end: I need a failing bit of code, or better yet, a failing test. I don't want you to solve the problem for me, I don't need further explanation and "this is clearly going to fail" premises.

I need failing code.

---

Reply to this email directly or view it on GitHub:
#143 (comment)