Refactor ExecuteQuery

[alkampfergit]

If you look at the code of method SqlPersistenceEngine::ExecuteQuery you can notice that nor connection nor transaction gets disposed if the code is successful.

Compare ExecuteQuery with the implementation of SqlPersistenEngine::ExecuteCommand that correctly disposes everything.

This can lead to serious problem, and actually probably it did not generated error because the standard implementation of OpenTransaction returns null.

I think that all the disposable object should be wrapped in using clause.

[fschmied]

I think in the success case, the PagedEnumerationCollection and the CommonDbStatement.Dispose methods are responsible for disposing the connection and transaction after enumeration has finished. As currently implemented, SqlPersistenceEngine.ExecuteQuery cannot dispose them in the success case (as enumeration is controlled by the caller).

However, this is really not intuitive and quite difficult to reason about, so I think it should be refactored. For example, the SqlPersistenceEngine.ExecuteQuery method could use an iterator block (foreach and yield return) wrapped in a using statement. That way, the Dispose methods would be executed whenever and however iteration finished.

[alkampfergit]

I agree with the refactoring, I've checked the code and all object seems to be correctly disposed by all code that calls that function, but the code is quite hard to read.