Disposing commands

Feb 23, 2015 at 5:39 AM
Hi.

Vita looks so impressive that it tempts to be used even for smaller projects. In that regard adding SQLite support is very welcome.

However, I realized that Vita does not care to dispose the DbCommand's it creates, similar to some other libraries. This turns out to be fatal for SQLite, which won't close the database connection while there are undisposed statements, leaving the file locked, which can mess up with the flow of the application or the tests in various ways.

Is there a reason for that and could it be changed? I looked at the code and it doesn't seem hard to do the needed modifications, but it would add a non-negligible maintenance cost when upgrading to future versions. It would be better if the library assumed the policy of always disposing any disposable objects in general.

Thanks,
Ivan
Coordinator
Feb 23, 2015 at 5:23 PM
Hi
DbCommand is not disposed on purpose - it is saved in EntitySession.LastCommand property, to be available for inspection in debugging. This is very useful facility, I can attest it from my own experience of debugging, especially for debugging LINQ queries. By the way, one of the complaints about EF is that it is extremely difficult to get to see the SQL that was generated and executed. With VITA, you can always put a breakpoint right after query.ToList() and inspect current session's LastCommand property.
Next, I've always believed there's nothing wrong with keeping DbCommand non-disposed, never found any warnings against this in docs, and it seems consistent with my experience. I admit, I'm not expert in SQLite, so can you please provide links to any docs that explain this?
I really hope you're a bit confused about DBCommand (otherwise we might have a problem). The typical situation with other data providers in .NET is that you have to close and dispose an active data reader (object returned by DbCommand.ExecuteReader) before you can close the connection. This is handled properly by VITA data layer, the data reader is closed after we complete reading the data after executing select. Then the connection is closed promptly (except special case in ReuseConnection mode when connection is kept open between multiple calls, to save on connection initialization - it is not free, even with connection pool).
So are you sure it is about DbCommand, or DataReader? Please provide some info - links to docs or smth. Another good thing to do is to build a simple example that shows how this works or does not work with SQLite. So far all my experience with SQLite (running numerous unit tests) did not indicate any troubles.
Roman
Feb 23, 2015 at 9:06 PM
DbCommand is not disposed on purpose - it is saved in EntitySession.LastCommand property, to be available for inspection in debugging. This is very useful facility, I can attest it from my own experience of debugging, especially for debugging LINQ queries. By the way, one of the complaints about EF is that it is extremely difficult to get to see the SQL that was generated and executed. With VITA, you can always put a breakpoint right after query.ToList() and inspect current session's LastCommand property.
This is indisputably a feature that cannot be forfeited at any cost.
Next, I've always believed there's nothing wrong with keeping DbCommand non-disposed, never found any warnings against this in docs, and it seems consistent with my experience. I admit, I'm not expert in SQLite, so can you please provide links to any docs that explain this?
SQLite's implementation of DbCommand uses the underlying native sqlite3_stmt object: http://www.sqlite.org/c3ref/stmt.html
It says that the object must be destroyed using sqlite3_finalize (a typical behavior for native C/C++ objects). The docs on sqlite3_finalize say: "The application must finalize every prepared statement in order to avoid resource leaks." Moreover, it seems the SQLite connection keeps a list of all currently open statements, since they can be iterated using the sqlite3_next_stmt function: http://www.sqlite.org/c3ref/next_stmt.html
I really hope you're a bit confused about DBCommand (otherwise we might have a problem). The typical situation with other data providers in .NET is that you have to close and dispose an active data reader (object returned by DbCommand.ExecuteReader) before you can close the connection. This is handled properly by VITA data layer, the data reader is closed after we complete reading the data after executing select. Then the connection is closed promptly (except special case in ReuseConnection mode when connection is kept open between multiple calls, to save on connection initialization - it is not free, even with connection pool).
So are you sure it is about DbCommand, or DataReader? Please provide some info - links to docs or smth. Another good thing to do is to build a simple example that shows how this works or does not work with SQLite. So far all my experience with SQLite (running numerous unit tests) did not indicate any troubles.
It looks like it's a matter of implementation of the SQLite ADO wrapper over the native SQLite API. Releasing the SQLiteDataReader is not enough, the SQLiteCommand needs to be told to finalize the prepared statement(s) it's holding. Looking at the code of SQLiteCommand I can see that this can be achieved not just by disposing the command, but also by setting the Connection or CommandText properties to null.

As for a test, here's a simple one:
    [TestMethod]
    public void TestSQLiteStatementFinalization()
    {
      var session = _app.OpenSession();
      var john = session.NewDriver( "D001", "John", "Dow" );
      session.SaveChanges();
      Assert.IsTrue( System.IO.File.Exists( @"VitaTestSQLite.db" ) );
      System.IO.File.Delete(@"VitaTestSQLite.db");
    }
I added this to 5.UnitTests\Vita.UnitTests.Basic\MiscTests.cs and changed ServerTypeForTestExplorer in App.config to SQLite. The File.Delete call throws an IOException - "The process cannot access the file '...\VitaTestSQLite.db' because it is being used by another process."

I can see a couple solutions. The simplest one would be to set to null the Connection property of commands that are no longer going to be used, before moving them to LastCommand. The harder one would be to take care to dispose commands, and to keep in LastCommand not the whole command object but rather just the parts of it that are important for diagnostics (the list of arguments besides the SQL text would be nice).

I feel that in the long run the "dispose everything IDisposable" approach is a better idea, especially for a library that's trying to be general and work correctly with extendable base classes. Once an interface or a base class has declared the use of IDisposable, you can never be sure what future implementation of that class/interface would decide to rely on Dispose() being called to release some important resources.
Coordinator
Feb 23, 2015 at 9:18 PM
thank you for detailed response, now I see the problem. Let me play with it; for now, I think the best solution would be to just set the Command.Connection property to null. As for IDisposable and your suggesting to Dispose all disposables, not sure, it might be there not because of nature of DbCommand itself, but because it might hold active DbConnection and DbTransaction refs - which should be disposed explicitly.
It seems to me the situation is not like VITA is out of line (not disposing IDbCommand explicitly), but SQLite.NET is the 'strange' dude behaving unlike other providers. So I think we'll just nullify command.Connection property after execution, make sure it fixes the problem for SQLite, and keep an eye on the case in future - if some error comes up for SQLite or other 'new' providers regarding this aspect.
thanks again
Roman
Coordinator
Feb 23, 2015 at 9:21 PM
Edited Feb 23, 2015 at 9:23 PM
Another consideration - I would prefer to avoid saving a 'copy' of executed command in session.LastCommand, just to avoid cost of copying. Note that this saved command is not actually used for anything at runtime, just for debugging. As of now, it is practically free (single assignment), and the saved real command contains all inherent details about the command and its parameters.
Feb 24, 2015 at 6:16 AM
Thank you for the understanding. The simple solution should do the job hopefully.

SQLite is indeed different from all other providers because it is actually an API for direct manipulation of the physical data (the file it's in), not for remote access to a server via some network protocol. On the other hand, the designers of the ADO wrapper cannot be blamed for using an interface that was put there by the designers of the framework.

And thinking more about it, it looks like commands were made disposable for a purpose. It is clear that a data reader should be disposable because it represents a session of retrieving data from the database, and it's always a good idea to signal explicitly the end of a session to a possibly remote server. But a command can be "prepared" and executed many times with different arguments, which also indicates a possible allocation of resources for the "preparation", that would be best released explicitly. And I think I vaguely remember something about DBMS's that allowed a command to be prepared on the server. So SQLite falls in this case, its commands are always "prepared" (the comment on their implementation of DbCommand.Prepare() is "Does nothing. Commands are prepared as they are executed the first time, and kept in prepared state afterwards"), so they need to be released. I also found this answer from http://system.data.sqlite.org/index.html/doc/trunk/www/faq.wiki : "(17) Why is System.Data.SQLite leaking memory, resources, etc? - All System.Data.SQLite objects that implement IDisposable, either directly or indirectly, should be explicitly disposed when they are no longer needed."

Copying the contents of the command is not necessary to be heavy, it depends on what will be copied. The text for example is just a string reference. The parameters are also just a reference to a DbParameterCollection, and keeping that after disposing the command doesn't sound dangerous (neither DbParameterCollection nor DbParameter use IDisposable). Looking at PetaPoco (which I'm currently using), it's keeping "string LastSQL" and "object[] LastArgs" properties from the last executed command. These are saved with the following two lines:
    _lastSql = cmd.CommandText;
    _lastArgs = (from IDataParameter parameter in cmd.Parameters select parameter.Value).ToArray();
Maybe they decided to be on the safe side and copied the parameter values instead of keeping the DbParameterCollection. And I think the copy has negligible performance impact.

Regards,
Ivan
Coordinator
Feb 24, 2015 at 5:38 PM
About prepared commands. MS SQL allows preparing commands (https://msdn.microsoft.com/en-us/library/system.data.sqlclient.sqlcommand.prepare%28v=vs.110%29.aspx),
as a separate step, so it is executed faster when it is executed. But the main thing to remember here is I think is that for 'real' DB Servers acting on events like command disposal involves non-negligible cost - roundtrip to server to let it know that the DB command is deallocated. This action is free for SQLite, as it is just direct in-process call, and allows having automatic performance boost (keeping prepared commands after first execution). The servers like MS SQL do have similar mechanisms - they keep caches of query plans and prepared commands, but this is managed on server side using expiration mechanism and memory pressure considerations. But they do not tie it to lifetime of DbCommand on the client - as it would require extra roundtrips. What I'm saying is this situation with command deallocation is most likely unique to SQLite, and we should not run into these for 'real' remote db servers.
So lets just disconnect the command. By the way, this can be done for SQLite only, db driver has a virtual method OnCommandExecuted or smth like that called right after the command was executed, for some custom actions - there are a few cases when we need to do special stuff, and this SQLite trouble seems to be just this. That means, no need to do this copy of DbCommand info, just to deallocate the object.

So, what's your opinion about VITA and its usability with SQLite?

thanks
Roman
Feb 25, 2015 at 10:06 AM
I implemented OnCommandExecuted for the SQLite driver to clear its Connection property, as you suggested.

This was not enough, since there were a few more places where commands were created and used not via ExecuteDbCommand(). I had to modify the following: ExecuteSelect() in DbModelLoader.cs, ApplyDbModelChanges() in DbModelUpdateManager.cs, and CreateDbCommandInfo() in SQLiteDbSqlBuilder.cs. I just wrapped the commands in using statements. This allowed the SQLite file to be deleted after using it. The basic and extended unit tests still pass with these changes.

BTW, given that Vita's source code does not use the default formatting rules, it would be a nice idea to include a Visual Studio settings file with Vita's rules. This would allow to easily setup VS to edit the code without breaking the formatting style.

I'm continuing with the evaluation of Vita for our needs. I don't expect any other problems with SQLite specifically. What would be different and would require some more changes is the need to use a separate interface representing the database entities, along with an object created by Vita that represents a specific instance (row) of that entity. Simpler ("mini") ORMs allow you to decorate and serialize your actual class as a DB entity. I suppose this separation is a better design choice in the long run (and I just found your blog post "About business logic" where you probably explain why), so I'm going to give it a try.

And one more question, though this is going off-topic. I'm looking for a way to represent complex members within an entity, but I can't find any. Here's an example:
public interface ICustomer
{
  string FirstName { get; set; }
  string LastName { get; set; }
  Address Address { get; set; }
}

public struct Address
{
  string City;
  string Street;
  string Zip;
}
The Address members should be columns within the Customer table, ideally prefixed by "Address." or "Address_". Is this possible with Vita?

Thanks,
Ivan
Coordinator
Feb 25, 2015 at 5:43 PM
About vs editor settings - found the way to do it, good suggestion, thanks! will do it.
About using complex objects - no, currently no support for this... not sure it is needed. Don't you think if implemented, it would encourage de-normalized design - which is bad, I think, in most cases. Like in your example, shouldn't you put address in a separate table?
I will make all changes regarding handling connection for SQLite in the next code push, thank you for help in this.
Feb 25, 2015 at 8:00 PM
rivantsov wrote:
About using complex objects - no, currently no support for this... not sure it is needed. Don't you think if implemented, it would encourage de-normalized design - which is bad, I think, in most cases. Like in your example, shouldn't you put address in a separate table?
Agreed, the example I gave is a cliche that doesn't fit well. There may be more than one person with the same address. However, entities are often large, with many properties, which often can be separated into logical groups. And these groups are still inherent to the entity and cannot be shared. In my case I have video recordings, which have a set of statistics like frame count, dropped frames, frames per second, etc. All these are even immutable, they are initialized once and never change. My current class wraps them into a RecordingStats struct (the struct emphasizes the idea that these cannot be shared). I admit it's just a matter of convenience to have these grouped together, it is possible to have them "flattened" within the entity.

I'm still figuring out what would be the best design and evaluating the options. For example, if I choose to keep my old "business" class describing the recording, I'll need something like Automapper to sync between it and the entity. And Automapper claims to support "flattening". Another option would be to embed the entity within the business class and have the exposing interface do the grouping.
Coordinator
Feb 25, 2015 at 10:36 PM
yea, I see what you mean, it is convenient. It is doable for Vita, to implement smth like this, but would involve some substantial coding, so it would not be high on priority list (there are a lot of things to fix currently). So try go around this. As for integrating with business objects, one suggestion is to create a bunch of extension methods - like DTO extensions in data service project, that convert between DTOs (web transfer objects) and entities. That might help to make mainstream logic less cluttered with this value transfer garbage.
Let me know how it goes.
Mar 1, 2015 at 3:23 PM
Let me know how it goes.
I've accumulated some more comments and questions, but I'll open a new topic.
Coordinator
Mar 1, 2015 at 3:37 PM
I'm planning new version push tonight, so it will be great if you can post your comments today, so I can address them in this push.
Although... have a nice Sunday!