Proper dispose (#60)

* cleanup error handling

* properly dipose statements and connections

* db should be var now

* autodispose db

datastore/key/namespace.nim

@@ -1,6 +1,5 @@
 
 import std/hashes
-import std/strformat
 
 import pkg/questionable
 import pkg/questionable/results
@@ -19,32 +18,26 @@ type
 
 func init*(T: type Namespace, field, value: string): ?!T =
   if value.contains(Delimiter):
-      return failure (&"value string must not contain Delimiter '{Delimiter}'")
+      return failure ("value string must not contain Delimiter " & Delimiter)
-        .catch.expect("should not fail")
 
   if value.contains(Separator):
-    return failure (&"value string must not contain Separator {Separator}")
+    return failure ("value string must not contain Separator " & Separator)
-      .catch.expect("should not fail")
 
   if field.contains(Delimiter):
-    return failure (&"field string must not contain Delimiter {Delimiter}")
+    return failure ("field string must not contain Delimiter " & Delimiter)
-      .catch.expect("should not fail")
 
   if field.contains(Separator):
-    return failure (&"field string must not contain Separator {Separator}")
+    return failure ("field string must not contain Separator " & Separator)
-      .catch.expect("should not fail")
 
   success T(field: field, value: value)
 
 func init*(T: type Namespace, id: string): ?!T =
   if id.len > 0:
     if id.contains(Separator):
-      return failure (&"id string must not contain Separator {Separator}")
+      return failure (&"id string must not contain Separator " & Separator)
-        .catch.expect("should not fail")
 
     if id.count(Delimiter) > 1:
-      return failure (&"id string must not contain more than one {Delimiter}")
+      return failure (&"id string must not contain more than one " & Delimiter)
-        .catch.expect("should not fail")
 
   let
     (field, value) = block:

datastore/sql/sqliteds.nim

@@ -228,8 +228,7 @@ method query*(
 
   let
     queryStmt = QueryStmt.prepare(
-      self.db.env, queryStr).expect("should not fail")
+      self.db.env, queryStr).expect("Query prepare should not fail")
-
     s = RawStmtPtr(queryStmt)
 
   var
@@ -263,7 +262,7 @@ method query*(
       let
         key = Key.init(
           $sqlite3_column_text_not_null(s, QueryStmtIdCol))
-          .expect("should not fail")
+          .expect("Key should should not fail")
 
         blob: ?pointer =
           if query.value:
@@ -308,7 +307,7 @@ method query*(
   iter.dispose = proc(): Future[?!void] {.async.} =
     discard sqlite3_reset(s)
     discard sqlite3_clear_bindings(s)
-    s.dispose
+    iter.next = nil
     return success()
 
   iter.next = next

datastore/sql/sqlitedsdb.nim

@@ -279,17 +279,30 @@ proc getDBFilePath*(path: string): ?!string =
   except CatchableError as exc:
     return failure(exc.msg)
 
-proc close*(self: SQLiteDsDb) =
+proc close*(self: var SQLiteDsDb) =
-  self.containsStmt.dispose
+
-  self.getStmt.dispose
+  var
-  self.beginStmt.dispose
+    env: AutoDisposed[SQLite]
-  self.endStmt.dispose
+
-  self.rollbackStmt.dispose
+  defer:
-  self.getVersionedStmt.dispose
+    disposeIfUnreleased(env)
-  self.updateVersionedStmt.dispose
+
-  self.insertVersionedStmt.dispose
+  env.val = self.env
-  self.deleteVersionedStmt.dispose
+
-  self.getChangesStmt.dispose
+  if not RawStmtPtr(self.containsStmt).isNil:
+    self.containsStmt.dispose
+
+  if not RawStmtPtr(self.getStmt).isNil:
+    self.getStmt.dispose
+
+  if not RawStmtPtr(self.beginStmt).isNil:
+    self.beginStmt.dispose
+
+  if not RawStmtPtr(self.endStmt).isNil:
+    self.endStmt.dispose
+
+  if not RawStmtPtr(self.rollbackStmt).isNil:
+    self.rollbackStmt.dispose
 
   if not RawStmtPtr(self.deleteStmt).isNil:
     self.deleteStmt.dispose
@@ -297,7 +310,20 @@ proc close*(self: SQLiteDsDb) =
   if not RawStmtPtr(self.putStmt).isNil:
     self.putStmt.dispose
 
-  self.env.dispose
+  if not RawStmtPtr(self.getVersionedStmt).isNil:
+    self.getVersionedStmt.dispose
+
+  if not RawStmtPtr(self.updateVersionedStmt).isNil:
+    self.updateVersionedStmt.dispose
+
+  if not RawStmtPtr(self.insertVersionedStmt).isNil:
+    self.insertVersionedStmt.dispose
+
+  if not RawStmtPtr(self.deleteVersionedStmt).isNil:
+    self.deleteVersionedStmt.dispose
+
+  if not RawStmtPtr(self.getChangesStmt).isNil:
+    self.getChangesStmt.dispose
 
 proc open*(
   T: type SQLiteDsDb,
@@ -329,7 +355,7 @@ proc open*(
 
   open(absPath, env.val, flags)
 
-  let
+  var
     pragmaStmt = journalModePragmaStmt(env.val)
 
   checkExec(pragmaStmt)

datastore/sql/sqliteutils.nim

@@ -89,7 +89,8 @@ template checkErr*(op: untyped) =
     return failure $sqlite3_errstr(v)
 
 template dispose*(rawStmt: RawStmtPtr) =
-  discard sqlite3_finalize(rawStmt)
+  doAssert SQLITE_OK == sqlite3_finalize(rawStmt)
+  rawStmt = nil
 
 template checkExec*(s: RawStmtPtr) =
   if (let x = sqlite3_step(s); x != SQLITE_DONE):
@@ -113,16 +114,22 @@ template prepare*(
   s
 
 template checkExec*(env: SQLite, q: string) =
-  let
+  var
     s = prepare(env, q)
 
   checkExec(s)
 
 template dispose*(db: SQLite) =
+  # TODO: the assert bellow fails because we're
+  # not releasing all the statements at the time of
+  # releasing the connection. I suspect these are the
+  # query iterators that aren't being released on close
+  # doAssert SQLITE_OK == sqlite3_close(db)
   discard sqlite3_close(db)
 
 template dispose*(sqliteStmt: SQLiteStmt) =
-  discard sqlite3_finalize(RawStmtPtr(sqliteStmt))
+  doAssert SQLITE_OK == sqlite3_finalize(RawStmtPtr(sqliteStmt))
+  sqliteStmt = nil
 
 proc release*[T](x: var AutoDisposed[T]): T =
   result = x.val
@@ -171,7 +178,7 @@ proc sqlite3_column_text_not_null*(
   text
 
 template journalModePragmaStmt*(env: SQLite): RawStmtPtr =
-  let
+  var
     s = prepare(env, "PRAGMA journal_mode = WAL;")
 
   if (let x = sqlite3_step(s); x != SQLITE_ROW):
@@ -250,7 +257,7 @@ proc query*(
   query: string,
   onData: DataProc): ?!bool =
 
-  let
+  var
     s = ? NoParamsStmt.prepare(env, query)
     res = s.query((), onData)
 

tests/datastore/sql/testsqlitedsdb.nim

@@ -27,7 +27,7 @@ suite "Test Open SQLite Datastore DB":
     require(not dirExists(basePathAbs))
 
   test "Should create and open datastore DB":
-    let
+    var
       dsDb = SQLiteDsDb.open(
         path = dbPathAbs,
         flags = SQLITE_OPEN_READWRITE or SQLITE_OPEN_CREATE).tryGet()
@@ -39,7 +39,7 @@ suite "Test Open SQLite Datastore DB":
         fileExists(dbPathAbs)
 
   test "Should open existing DB":
-    let
+    var
       dsDb = SQLiteDsDb.open(
         path = dbPathAbs,
         flags = SQLITE_OPEN_READWRITE or SQLITE_OPEN_CREATE).tryGet()
@@ -54,7 +54,7 @@ suite "Test Open SQLite Datastore DB":
     check:
       fileExists(dbPathAbs)
 
-    let
+    var
       dsDb = SQLiteDsDb.open(
         path = dbPathAbs,
         flags = SQLITE_OPEN_READONLY).tryGet()