Skip to content

HTTPS clone URL

Subversion checkout URL

You can clone with HTTPS or Subversion.

Download ZIP

Loading…

Adds a `using` statement to a `Query` overload #198

Closed
wants to merge 2 commits into from

2 participants

@pbu88

Adds a using statement wrapping the DbDataReader access missing in
public IEnumerable<dynamic> Query(string sql, params object[] args)
method overload.

This caused database locked errors when you executed a query followed
immediately by an INSERT to the database in methods using this Query's
overload.

For example, using First method this way:

var obj = table.First(...).id;
table.Insert(...);

will yield a database locked error because the rdr in Query is
not being closed/disposed.

The other Query's overload had it so I assume this one must had been
forgotten.

Paulo Bu Adds a `using` statement to a `Query` overload
Adds a `using` statement wrapping the `DbDataReader` access missing in
`public IEnumerable<dynamic> Query(string sql, params object[] args)`
method overload.

This caused `database locked` errors when you executed a query followed
immediately by an `INSERT` to the database in methods using this `Query`'s
overload.

For example, using `First` method this way:

    var obj = table.First(...).id;
    table.Insert(...);

will yield a `database locked` error because the `rdr` in `Query` is
not being closed/disposed.

The other `Query`'s overload had it so I assume this one must had been
forgotten.
757453c
@robconery
Collaborator

I didn't wrap this in a using block because of the yield that's in there, I was under the impression that if you yielded from a using the Reader would go out of scope and everything shut down.

I'd like to see a bit more on this failure. When you exit the first using statement (for the connection) it's out of scope so it's closed. First should be doing this - if it's not we're in trouble... can you send me some failing code?

@pbu88

I did some research before posting this and managed to reproduce the problem exactly. I did some research also combining using and yield expressions to find what happens when you issue FirstOrDefault on an Iterator using another object. I'll pack up the testing and offending code I've made and send it.

I am under the impression that this error have to do strictly with SQLite3, because I saw a bug report with the System.Data.Sqlite that you need to explicitly close the database connection and the database reader. It's probably that it isn't necessary to do that with other database engines.

@pbu88

First of all, I've manage to narrow the problem to these few lines of code:

        dynamic table = new Box();
        var first = table.First();
        table.Insert(
            new
            {
                // this fields are required in my db
                // that's why I include them in the code
                label = "hola",
                label_num = 1000,
                id_type = 1
            });

This throws:

Unhandled Exception: System.Data.SQLite.SQLiteException: database is locked
database is locked
   at System.Data.SQLite.SQLite3.Step(SQLiteStatement stmt)
   at System.Data.SQLite.SQLiteDataReader.NextResult()
   at System.Data.SQLite.SQLiteDataReader..ctor(SQLiteCommand cmd, CommandBehavior behave)
   at System.Data.SQLite.SQLiteCommand.ExecuteReader(CommandBehavior behavior)
   at System.Data.SQLite.SQLiteCommand.ExecuteNonQuery()
   at Massive.SQLite.DynamicModel.Insert(Object o) in C:\Users\ike\Documents\Work\RBH\RBHFileStore\RBHFileStore\Massive\Massive.Sqlite.cs:line 465
   at CallSite.Target(Closure , CallSite , Object , <>f__AnonymousType0`3 ) at System.Dynamic.UpdateDelegates.UpdateAndExecuteVoid2[T0,T1](CallSite site, T0 arg0, T1 arg1) at TestMassive.Program.Main(String[] args) in C:\Users\ike\Documents\Work\RBH\TestMassive\TestMassive\Program.cs:line 21
Press any key to continue . . .

My first suspicion was about the need to explicitly close the conn object and it came from this:

        dynamic table = new Box();
        var first = table.First();
        // looping through the result makes the code
        // work ok.
        foreach (var e in first)
        {
            Console.WriteLine(e);
        }
        table.Insert(
            new
            {
                label = "hola",
                label_num = 1000,
                id_type = 1
            });

The result is this and the row is correctly inserted:

[id, 1]
[label, L-1]
[id_type, 2]
[label_num, 1]
Press any key to continue . . .

This could mean that iterating through the whole result may explicitly invoke Dispose on conn at the last iteration.

I noticed First invokes Query and then FirstOrDefault on Query's result. I suspected about FirstOrDefault not disposing an iterator resources so wrote some test to prove it and yes, it indeed dispose it, so the conn object was being correctly disposed. The other suspicious resource at Query's method was rdr (weird, because I've never needed to explicitly close a DataReader before with another db engines). So I followed my gut and in Query's body, turned this:

var rdr = CreateCommand(sql, conn, args).ExecuteReader();

into this:

using (var rdr = CreateCommand(sql, conn, args).ExecuteReader())

and tested the first offending code (the one without the foreach on the result). The result was satisfactory. I tried it a few times, placed it in a for loop to try it fast and everything worked ok.

To clarify more the thing for me, I forgot about Massive.Sqlite.cs and tried to query the database myself using .NET's System.Data mechanisms.

Wrote two methods, one just queried the database and the other modified it:

    static void CallDb()
    {
        // routine code for getting a Command and a Connection
        _factory = DbProviderFactories.GetFactory("System.Data.SQLite");
        var con = _factory.CreateConnection();
        con.ConnectionString = @"Data Source=c:\users\ike\documents\work\rbh\RBHFileStore\Ruby\db.sqlite;Version=3;";
        var command = _factory.CreateCommand();
        command.CommandText = "select * from box";
        command.Connection = con;

        //query starts here
        con.Open();
        var rdr = command.ExecuteReader();
        //rdr.Close()
        con.Close();
    }

    static void CallDb1()
    {
        // routine code for getting a Command and a Connection
        _factory = DbProviderFactories.GetFactory("System.Data.SQLite");
        var con = _factory.CreateConnection();
        con.ConnectionString = @"Data Source=c:\users\ike\documents\work\rbh\RBHFileStore\Ruby\db.sqlite;Version=3;";
        var command = _factory.CreateCommand();
        command.CommandText = "insert into box (label, label_num, id_type) values ('hola',1000,1)";
        command.Connection = con;

        //query starts here
        con.Open();
        var rdr = command.ExecuteNonQuery();
        con.Close();
    }
}

And called them like this:

        //query   
        CallDb();
        Console.WriteLine("done");
        // insert
        CallDb1();

When at CallDb(), rdr.Close() was commented, the result I got was this:

done
Unhandled Exception: System.Data.SQLite.SQLiteException: database is locked
database is locked
   at System.Data.SQLite.SQLite3.Step(SQLiteStatement stmt)
   at System.Data.SQLite.SQLiteDataReader.NextResult()
   at System.Data.SQLite.SQLiteDataReader..ctor(SQLiteCommand cmd, CommandBehavior behave)
   at System.Data.SQLite.SQLiteCommand.ExecuteReader(CommandBehavior behavior)
   at System.Data.SQLite.SQLiteCommand.ExecuteNonQuery()
   at TestMassive.Program.CallDb1() in C:\Users\ike\Documents\Work\RBH\TestMassive\TestMassive\Program.cs:line 67
   at TestMassive.Program.Main(String[] args) in C:\Users\ike\Documents\Work\RBH\TestMassive\TestMassive\Program.cs:line 18
Press any key to continue . . .

When I uncommented it and explicitly closed rdr. The program worked ok.

I went even further and toyed with GC object. If I comment again rdr.Close() but specifically issue GC.Collect() on the invoking code, the program works well!

        //query   
        CallDb();
        GC.Collect()
        Console.WriteLine("done");
        // insert
        CallDb1();

So this means that if no explicit dispose is issued to the reader, then it has to be collected before the next database call, otherwise it would be locked. So IMHO this is what is happening with the code at Masive.Sqlite's Query method, not wrapping its rdr in a using
statement.

For further reference, I found this answer at StackOverflow after I posted the issue and ended up convincing myself about the problem :). I'm pretty sure this problem has to do only with System.Data.Sqlite.dll, because of the way SQLite3 has to deal with concurrency.

Sorry for the extension of this explanation. I just wanted to make my point clear enough. Also I didn't knew if you wanted here or at your email. I guess here makes more sense.

Paulo Bu Changes `private` to `protected` protection modifier in `DynamicModel…
….BuildSelect`

I think it would be useful for custom models inheriting from `DynamicModel` to have access to `BuildSelect` method. This is the use-case where I found it useful to call `BuildSelect` from my model:

I have a ClientBox table, which is merely a many-to-many relationship to Client and Box, and wanted to override
`DynamicModel`'s `All` method to return the combined tables, not the relationship fields. So I went:

    public override IEnumerable<dynamic> All(string where = "", string orderBy = "", int limit = 0, string columns = "*", params object[] args)
    {
        var tableCombination = @"Client INNER JOIN Client_Box ON Client.id = Client_Box.id_client
                                 INNER JOIN Box ON Client_Box.id_box = Box.id";
        string sql = BuildSelect(where, orderBy, limit);
        return Query(string.Format(sql, columns, tableCombination), args);
    }

With `BuildSelect` being `private`, child classes can't take advantage from it and this kind of overriding.
57a5762
@robconery
Collaborator

I like this idea - but the thing about Massive is that it's a single file purposely created for you to alter as you need. When offered suggestions like yours I need to evaluate whether it's useful in the core distribution... in this case I see your need but I'm not sure if it translates to others.

@pbu88

That's perfectly understandable. At first, I didn't want to this idea be mixed with my fix suggestion to the SQLite3 engine, which was my main concern. But after doing the commit I just messed up a little with Github and they ended up both being in the same pull request :(

Did you had the time to check the SQLite3 issue? I did some research which explained in former comments. That's my main concern. You can loosely dispose the private to protected suggestion if it isn't needed by other users.

By the way, good job with Massive. I has been very useful for me. I'm accustomed to script languages (Python) and dealing with all the .NET's labyrinth of object for making just a single easy query is sometimes overwhelming.

@robconery robconery closed this
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Commits on Sep 4, 2013
  1. Adds a `using` statement to a `Query` overload

    Paulo Bu authored
    Adds a `using` statement wrapping the `DbDataReader` access missing in
    `public IEnumerable<dynamic> Query(string sql, params object[] args)`
    method overload.
    
    This caused `database locked` errors when you executed a query followed
    immediately by an `INSERT` to the database in methods using this `Query`'s
    overload.
    
    For example, using `First` method this way:
    
        var obj = table.First(...).id;
        table.Insert(...);
    
    will yield a `database locked` error because the `rdr` in `Query` is
    not being closed/disposed.
    
    The other `Query`'s overload had it so I assume this one must had been
    forgotten.
Commits on Sep 16, 2013
  1. Changes `private` to `protected` protection modifier in `DynamicModel…

    Paulo Bu authored
    ….BuildSelect`
    
    I think it would be useful for custom models inheriting from `DynamicModel` to have access to `BuildSelect` method. This is the use-case where I found it useful to call `BuildSelect` from my model:
    
    I have a ClientBox table, which is merely a many-to-many relationship to Client and Box, and wanted to override
    `DynamicModel`'s `All` method to return the combined tables, not the relationship fields. So I went:
    
        public override IEnumerable<dynamic> All(string where = "", string orderBy = "", int limit = 0, string columns = "*", params object[] args)
        {
            var tableCombination = @"Client INNER JOIN Client_Box ON Client.id = Client_Box.id_client
                                     INNER JOIN Box ON Client_Box.id_box = Box.id";
            string sql = BuildSelect(where, orderBy, limit);
            return Query(string.Format(sql, columns, tableCombination), args);
        }
    
    With `BuildSelect` being `private`, child classes can't take advantage from it and this kind of overriding.
This page is out of date. Refresh to see the latest.
View
4 Massive.Oracle.cs
@@ -316,7 +316,7 @@ public class DynamicModel : DynamicObject {
string sql = BuildSelect(where, orderBy, limit);
return Query(string.Format(sql, columns, TableName), args);
}
- private static string BuildSelect(string where, string orderBy, int limit) {
+ protected static string BuildSelect(string where, string orderBy, int limit) {
string sql = "SELECT {0} FROM {1} ";
if (!string.IsNullOrEmpty(where))
sql += where.Trim().StartsWith("where", StringComparison.OrdinalIgnoreCase) ? where : "WHERE " + where;
@@ -658,4 +658,4 @@ public class DynamicModel : DynamicObject {
return true;
}
}
-}
+}
View
4 Massive.PostgreSQL.cs
@@ -371,7 +371,7 @@ public virtual IEnumerable<dynamic> All(string where = "", string orderBy = "",
string sql = BuildSelect(where, orderBy, limit);
return Query(string.Format(sql, columns, TableName), args);
}
- private static string BuildSelect(string where, string orderBy, int limit)
+ protected static string BuildSelect(string where, string orderBy, int limit)
{
string sql = "SELECT {0} FROM {1} ";
if (!string.IsNullOrEmpty(where))
@@ -805,4 +805,4 @@ public override bool TryInvokeMember(InvokeMemberBinder binder, object[] args, o
return true;
}
}
-}
+}
View
12 Massive.Sqlite.cs
@@ -233,10 +233,12 @@ public virtual IEnumerable<dynamic> Query(string sql, params object[] args)
{
using (var conn = OpenConnection())
{
- var rdr = CreateCommand(sql, conn, args).ExecuteReader();
- while (rdr.Read())
+ using(var rdr = CreateCommand(sql, conn, args).ExecuteReader())
{
- yield return rdr.RecordToExpando(); ;
+ while (rdr.Read())
+ {
+ yield return rdr.RecordToExpando(); ;
+ }
}
}
}
@@ -489,7 +491,7 @@ public virtual IEnumerable<dynamic> All(string where = "", string orderBy = "",
string sql = BuildSelect(where, orderBy, limit);
return Query(string.Format(sql, columns, TableName), args);
}
- private static string BuildSelect(string where, string orderBy, int limit)
+ protected static string BuildSelect(string where, string orderBy, int limit)
{
string sql = limit > 0 ? "SELECT TOP " + limit + " {0} FROM {1} " : "SELECT {0} FROM {1} ";
if (!string.IsNullOrEmpty(where))
@@ -623,4 +625,4 @@ public override bool TryInvokeMember(InvokeMemberBinder binder, object[] args, o
return true;
}
}
-}
+}
View
4 Massive.cs
@@ -306,7 +306,7 @@ public class DynamicModel : DynamicObject {
string sql = BuildSelect(where, orderBy, limit);
return Query(string.Format(sql, columns, TableName), args);
}
- private static string BuildSelect(string where, string orderBy, int limit) {
+ protected static string BuildSelect(string where, string orderBy, int limit) {
string sql = limit > 0 ? "SELECT TOP " + limit + " {0} FROM {1} " : "SELECT {0} FROM {1} ";
if (!string.IsNullOrEmpty(where))
sql += where.Trim().StartsWith("where", StringComparison.OrdinalIgnoreCase) ? where : " WHERE " + where;
@@ -670,4 +670,4 @@ private dynamic BuildPagedResult(string sql = "", string primaryKeyField = "", s
return true;
}
}
-}
+}
Something went wrong with that request. Please try again.