Implement autoclose on missed types. (#59)

* Implement autoclose on missed types.

* Cleanup using templates and add more tests.

rocksdb/backup.nim

@@ -39,7 +39,8 @@ proc openBackupEngine*(
   let backupEnginePtr = rocksdb_backup_engine_open(
     backupOpts.cPtr, path.cstring, cast[cstringArray](errors.addr)
   )
-  bailOnErrors(errors, backupOpts = backupOpts)
+  bailOnErrorsWithCleanup(errors):
+    autoCloseNonNil(backupOpts)
 
   let engine =
     BackupEngineRef(cPtr: backupEnginePtr, path: path, backupOpts: backupOpts)
@@ -93,5 +94,4 @@ proc close*(backupEngine: BackupEngineRef) =
     rocksdb_backup_engine_close(backupEngine.cPtr)
     backupEngine.cPtr = nil
 
-    if backupEngine.backupOpts.autoClose:
-      backupEngine.backupOpts.close()
+    autoCloseNonNil(backupEngine.backupOpts)

rocksdb/columnfamily/cfopts.nim

@@ -9,12 +9,14 @@
 
 {.push raises: [].}
 
-import ../lib/librocksdb, ../options/tableopts
+import ../lib/librocksdb, ../internal/utils, ../options/tableopts
 
 type
   SlicetransformPtr* = ptr rocksdb_slicetransform_t
+
   SlicetransformRef* = ref object
     cPtr: SlicetransformPtr
+    autoClose*: bool # if true then close will be called when it's parent is closed
 
   ColFamilyOptionsPtr* = ptr rocksdb_options_t
 
@@ -23,6 +25,8 @@ type
     # type - CF options are a subset of rocksdb_options_t - when in doubt, check:
     # https://github.com/facebook/rocksdb/blob/b8c9a2576af6a1d0ffcfbb517dfcb7e7037bd460/include/rocksdb/options.h#L66
     cPtr: ColFamilyOptionsPtr
+    sliceTransform: SlicetransformRef
+    tableOpts: TableOptionsRef
     autoClose*: bool # if true then close will be called when the database is closed
 
   Compression* {.pure.} = enum
@@ -36,8 +40,11 @@ type
     xpressCompression = rocksdb_xpress_compression
     zstdCompression = rocksdb_zstd_compression
 
-proc createFixedPrefix*(value: int): SlicetransformRef =
-  SlicetransformRef(cPtr: rocksdb_slicetransform_create_fixed_prefix(value.csize_t))
+proc createFixedPrefix*(value: int, autoClose = false): SlicetransformRef =
+  SlicetransformRef(
+    cPtr: rocksdb_slicetransform_create_fixed_prefix(value.csize_t),
+    autoClose: autoClose,
+  )
 
 proc isClosed*(s: SlicetransformRef): bool {.inline.} =
   s.cPtr.isNil()
@@ -66,6 +73,9 @@ proc close*(cfOpts: ColFamilyOptionsRef) =
     rocksdb_options_destroy(cfOpts.cPtr)
     cfOpts.cPtr = nil
 
+    autoCloseNonNil(cfOpts.sliceTransform)
+    autoCloseNonNil(cfOpts.tableOpts)
+
 template opt(nname, ntyp, ctyp: untyped) =
   proc `nname=`*(cfOpts: ColFamilyOptionsRef, value: ntyp) =
     doAssert not cfOpts.isClosed
@@ -125,13 +135,21 @@ proc defaultColFamilyOptions*(autoClose = false): ColFamilyOptionsRef =
 
 proc `setPrefixExtractor`*(cfOpts: ColFamilyOptionsRef, value: SlicetransformRef) =
   doAssert not cfOpts.isClosed()
+  doAssert cfOpts.sliceTransform.isNil()
+    # don't allow overwriting an existing sliceTransform which could leak memory
+
   rocksdb_options_set_prefix_extractor(cfOpts.cPtr, value.cPtr)
+  cfOpts.sliceTransform = value
 
 proc `blockBasedTableFactory=`*(
     cfOpts: ColFamilyOptionsRef, tableOpts: TableOptionsRef
 ) =
   doAssert not cfOpts.isClosed()
+  doAssert cfOpts.tableOpts.isNil()
+    # don't allow overwriting an existing tableOpts which could leak memory
+
   rocksdb_options_set_block_based_table_factory(cfOpts.cPtr, tableOpts.cPtr)
+  cfOpts.tableOpts = tableOpts
 
 # https://github.com/facebook/rocksdb/wiki/MemTable
 proc setHashSkipListRep*(

rocksdb/internal/utils.nim

@@ -9,42 +9,32 @@
 
 {.push raises: [].}
 
-import
-  std/locks,
-  ../lib/librocksdb,
-  ../options/[dbopts, readopts, writeopts, backupopts],
-  ../transactions/txdbopts,
-  ../columnfamily/cfdescriptor
+import std/locks, ../lib/librocksdb
 
 proc createLock*(): Lock =
   var lock = Lock()
   initLock(lock)
   lock
 
-template autoCloseNonNil*(opts: typed) =
-  if not opts.isNil and opts.autoClose:
-    opts.close()
+template autoCloseNonNil*(closable: typed) =
+  if not closable.isNil and closable.autoClose:
+    closable.close()
 
-template bailOnErrors*(
-    errors: cstring,
-    dbOpts: DbOptionsRef = nil,
-    readOpts: ReadOptionsRef = nil,
-    writeOpts: WriteOptionsRef = nil,
-    txDbOpts: TransactionDbOptionsRef = nil,
-    backupOpts: BackupEngineOptionsRef = nil,
-    cfDescriptors: openArray[ColFamilyDescriptor] = @[],
-): auto =
+template autoCloseAll*(closables: openArray[typed]) =
+  for c in closables:
+    if c.autoClose:
+      c.close()
+
+template bailOnErrorsWithCleanup*(errors: cstring, cleanup: untyped): auto =
   if not errors.isNil:
-    autoCloseNonNil(dbOpts)
-    autoCloseNonNil(readOpts)
-    autoCloseNonNil(writeOpts)
-    autoCloseNonNil(txDbOpts)
-    autoCloseNonNil(backupOpts)
-
-    for cfDesc in cfDescriptors:
-      if cfDesc.autoClose:
-        cfDesc.close()
+    cleanup
 
     let res = err($(errors))
     rocksdb_free(errors)
     return res
+
+template bailOnErrors*(errors: cstring): auto =
+  if not errors.isNil:
+    let res = err($(errors))
+    rocksdb_free(errors)
+    return res

rocksdb/options/cache.nim

@@ -5,9 +5,10 @@ type
 
   CacheRef* = ref object
     cPtr*: CachePtr
+    autoClose*: bool # if true then close will be called when it's parent is closed
 
-proc cacheCreateLRU*(size: int): CacheRef =
-  CacheRef(cPtr: rocksdb_cache_create_lru(size.csize_t))
+proc cacheCreateLRU*(size: int, autoClose = false): CacheRef =
+  CacheRef(cPtr: rocksdb_cache_create_lru(size.csize_t), autoClose: autoClose)
 
 proc close*(cache: CacheRef) =
   if cache.cPtr != nil:

rocksdb/options/dbopts.nim

@@ -9,7 +9,7 @@
 
 {.push raises: [].}
 
-import std/cpuinfo, ../lib/librocksdb, ./[cache, tableopts]
+import std/cpuinfo, ../lib/librocksdb, ../internal/utils, ./[cache, tableopts]
 
 export cache, tableopts
 
@@ -18,6 +18,7 @@ type
 
   DbOptionsRef* = ref object
     cPtr: DbOptionsPtr
+    cache: CacheRef
     autoClose*: bool # if true then close will be called when the database is closed
 
 proc newDbOptions*(autoClose = false): DbOptionsRef =
@@ -94,7 +95,11 @@ opt avoidUnnecessaryBlockingIo, bool, uint8
 
 proc `rowCache=`*(dbOpts: DbOptionsRef, cache: CacheRef) =
   doAssert not dbOpts.isClosed()
+  doAssert dbOpts.cache.isNil()
+    # don't allow overwriting an existing cache which could leak memory
+
   rocksdb_options_set_row_cache(dbOpts.cPtr, cache.cPtr)
+  dbOpts.cache = cache
 
 proc defaultDbOptions*(autoClose = false): DbOptionsRef =
   let opts: DbOptionsRef = newDbOptions(autoClose)
@@ -118,3 +123,5 @@ proc close*(dbOpts: DbOptionsRef) =
   if not dbOpts.isClosed():
     rocksdb_options_destroy(dbOpts.cPtr)
     dbOpts.cPtr = nil
+
+    autoCloseNonNil(dbOpts.cache)

rocksdb/options/tableopts.nim

@@ -1,4 +1,4 @@
-import ../lib/librocksdb, ./cache
+import ../lib/librocksdb, ../internal/utils, ./cache
 
 type
   # TODO might eventually wrap this
@@ -6,11 +6,15 @@ type
 
   TableOptionsRef* = ref object
     cPtr*: TableOptionsPtr
+    cache: CacheRef
+    filterPolicy: FilterPolicyRef
+    autoClose*: bool # if true then close will be called when it's parent is closed
 
   FilterPolicyPtr* = ptr rocksdb_filterpolicy_t
 
   FilterPolicyRef* = ref object
     cPtr*: FilterPolicyPtr
+    autoClose*: bool # if true then close will be called when it's parent is closed
 
   IndexType* {.pure.} = enum
     binarySearch = rocksdb_block_based_table_index_type_binary_search
@@ -22,14 +26,17 @@ type
     binarySearchAndHash =
       rocksdb_block_based_table_data_block_index_type_binary_search_and_hash
 
-proc createRibbon*(bitsPerKey: float): FilterPolicyRef =
-  FilterPolicyRef(cPtr: rocksdb_filterpolicy_create_ribbon(bitsPerKey))
+proc createRibbon*(bitsPerKey: float, autoClose = false): FilterPolicyRef =
+  FilterPolicyRef(
+    cPtr: rocksdb_filterpolicy_create_ribbon(bitsPerKey), autoClose: autoClose
+  )
 
 proc createRibbonHybrid*(
-    bitsPerKey: float, bloomBeforeLevel: int = 0
+    bitsPerKey: float, bloomBeforeLevel: int = 0, autoClose = false
 ): FilterPolicyRef =
   FilterPolicyRef(
-    cPtr: rocksdb_filterpolicy_create_ribbon_hybrid(bitsPerKey, bloomBeforeLevel.cint)
+    cPtr: rocksdb_filterpolicy_create_ribbon_hybrid(bitsPerKey, bloomBeforeLevel.cint),
+    autoClose: autoClose,
   )
 
 proc isClosed*(policy: FilterPolicyRef): bool =
@@ -40,8 +47,8 @@ proc close*(policy: FilterPolicyRef) =
     rocksdb_filterpolicy_destroy(policy.cPtr)
     policy.cPtr = nil
 
-proc createTableOptions*(): TableOptionsRef =
-  TableOptionsRef(cPtr: rocksdb_block_based_options_create())
+proc createTableOptions*(autoClose = false): TableOptionsRef =
+  TableOptionsRef(cPtr: rocksdb_block_based_options_create(), autoClose: autoClose)
 
 proc isClosed*(opts: TableOptionsRef): bool =
   isNil(opts.cPtr)
@@ -51,6 +58,9 @@ proc close*(opts: TableOptionsRef) =
     rocksdb_block_based_options_destroy(opts.cPtr)
     opts.cPtr = nil
 
+    autoCloseNonNil(opts.cache)
+    autoCloseNonNil(opts.filterPolicy)
+
 template opt(nname, ntyp, ctyp: untyped) =
   proc `nname=`*(opts: TableOptionsRef, value: ntyp) =
     doAssert not opts.isClosed
@@ -76,15 +86,26 @@ opt wholeKeyFiltering, bool, uint8
 opt formatVersion, int, cint
 
 proc `blockCache=`*(opts: TableOptionsRef, cache: CacheRef) =
+  doAssert not opts.isClosed()
+  doAssert opts.cache.isNil()
+    # don't allow overwriting an existing cache which could leak memory
+
   rocksdb_block_based_options_set_block_cache(opts.cPtr, cache.cPtr)
+  opts.cache = cache
 
 proc `filterPolicy=`*(opts: TableOptionsRef, policy: FilterPolicyRef) =
-  rocksdb_block_based_options_set_filter_policy(opts.cPtr, policy.cPtr)
+  doAssert not opts.isClosed()
+  doAssert opts.filterPolicy.isNil()
+    # don't allow overwriting an existing policy which could leak memory
 
-proc defaultTableOptions*(): TableOptionsRef =
+  rocksdb_block_based_options_set_filter_policy(opts.cPtr, policy.cPtr)
+  opts.filterPolicy = policy
+
+proc defaultTableOptions*(autoClose = false): TableOptionsRef =
   # https://github.com/facebook/rocksdb/wiki/Setup-Options-and-Basic-Tuning#other-general-options
-  let opts = createTableOptions()
+  let opts = createTableOptions(autoClose)
   opts.blockSize = 16 * 1024
   opts.cacheIndexAndFilterBlocks = true
   opts.pinL0FilterAndIndexBlocksInCache = true
+
   opts

rocksdb/rocksdb.nim

@@ -77,7 +77,8 @@ proc listColumnFamilies*(path: string): RocksDBResult[seq[string]] =
     cfList = rocksdb_list_column_families(
       dbOpts.cPtr, path.cstring, addr cfLen, cast[cstringArray](errors.addr)
     )
-  bailOnErrors(errors, dbOpts)
+  bailOnErrorsWithCleanup(errors):
+    autoCloseNonNil(dbOpts)
 
   if cfList.isNil or cfLen == 0:
     return ok(newSeqOfCap[string](0))
@@ -127,7 +128,11 @@ proc openRocksDb*(
     cfHandles[0].addr,
     cast[cstringArray](errors.addr),
   )
-  bailOnErrors(errors, dbOpts, readOpts, writeOpts, cfDescriptors = cfs)
+  bailOnErrorsWithCleanup(errors):
+    autoCloseNonNil(dbOpts)
+    autoCloseNonNil(readOpts)
+    autoCloseNonNil(writeOpts)
+    autoCloseAll(cfs)
 
   let
     cfTable = newColFamilyTable(cfNames.mapIt($it), cfHandles)
@@ -181,7 +186,10 @@ proc openRocksDbReadOnly*(
     errorIfWalFileExists.uint8,
     cast[cstringArray](errors.addr),
   )
-  bailOnErrors(errors, dbOpts, readOpts, cfDescriptors = cfs)
+  bailOnErrorsWithCleanup(errors):
+    autoCloseNonNil(dbOpts)
+    autoCloseNonNil(readOpts)
+    autoCloseAll(cfs)
 
   let
     cfTable = newColFamilyTable(cfNames.mapIt($it), cfHandles)
@@ -407,10 +415,8 @@ proc close*(db: RocksDbRef) =
       db.cPtr = nil
 
       # opts should be closed after the database is closed
-      if db.dbOpts.autoClose:
-        db.dbOpts.close()
-      if db.readOpts.autoClose:
-        db.readOpts.close()
+      autoCloseNonNil(db.dbOpts)
+      autoCloseNonNil(db.readOpts)
 
       for cfDesc in db.cfDescriptors:
         if cfDesc.autoClose:
@@ -418,8 +424,7 @@ proc close*(db: RocksDbRef) =
 
       if db of RocksDbReadWriteRef:
         let db = RocksDbReadWriteRef(db)
-        if db.writeOpts.autoClose:
-          db.writeOpts.close()
+        autoCloseNonNil(db.writeOpts)
 
         rocksdb_ingestexternalfileoptions_destroy(db.ingestOptsPtr)
         db.ingestOptsPtr = nil

rocksdb/sstfilewriter.nim

@@ -44,7 +44,8 @@ proc openSstFileWriter*(
   rocksdb_sstfilewriter_open(
     writer.cPtr, filePath.cstring, cast[cstringArray](errors.addr)
   )
-  bailOnErrors(errors, dbOpts)
+  bailOnErrorsWithCleanup(errors):
+    autoCloseNonNil(dbOpts)
 
   ok(writer)
 
@@ -102,5 +103,4 @@ proc close*(writer: SstFileWriterRef) =
     rocksdb_sstfilewriter_destroy(writer.cPtr)
     writer.cPtr = nil
 
-    if writer.dbOpts.autoClose:
-      writer.dbOpts.close()
+    autoCloseNonNil(writer.dbOpts)

rocksdb/transactiondb.nim

@@ -72,7 +72,10 @@ proc openTransactionDb*(
     cfHandles[0].addr,
     cast[cstringArray](errors.addr),
   )
-  bailOnErrors(errors, dbOpts, txDbOpts = txDbOpts, cfDescriptors = cfs)
+  bailOnErrorsWithCleanup(errors):
+    autoCloseNonNil(dbOpts)
+    autoCloseNonNil(txDbOpts)
+    autoCloseAll(cfs)
 
   let
     cfTable = newColFamilyTable(cfNames.mapIt($it), cfHandles)
@@ -131,11 +134,6 @@ proc close*(db: TransactionDbRef) =
       db.cPtr = nil
 
       # opts should be closed after the database is closed
-      if db.dbOpts.autoClose:
-        db.dbOpts.close()
-      if db.txDbOpts.autoClose:
-        db.txDbOpts.close()
-
-      for cfDesc in db.cfDescriptors:
-        if cfDesc.autoClose:
-          cfDesc.close()
+      autoCloseNonNil(db.dbOpts)
+      autoCloseNonNil(db.txDbOpts)
+      autoCloseAll(db.cfDescriptors)

rocksdb/transactions/transaction.nim

@@ -179,9 +179,6 @@ proc close*(tx: TransactionRef) =
     tx.cPtr = nil
 
     # opts should be closed after the transaction is closed
-    if tx.readOpts.autoClose:
-      tx.readOpts.close()
-    if tx.writeOpts.autoClose:
-      tx.writeOpts.close()
-    if tx.txOpts.autoClose:
-      tx.txOpts.close()
+    autoCloseNonNil(tx.readOpts)
+    autoCloseNonNil(tx.writeOpts)
+    autoCloseNonNil(tx.txOpts)

tests/test_rocksdb.nim

@@ -350,21 +350,60 @@ suite "RocksDbRef Tests":
     db.close()
     check db.isClosed()
 
-  test "Test auto close":
+  test "Test auto close enabled":
     let
-      dbPath = mkdtemp() / "autoclose"
-      dbOpts = defaultDbOptions(autoClose = false)
+      dbPath = mkdtemp() / "autoclose-enabled"
+      dbOpts = defaultDbOptions(autoClose = true)
       readOpts = defaultReadOptions(autoClose = true)
-      db = openRocksDb(dbPath, dbOpts, readOpts).get()
+      writeOpts = defaultWriteOptions(autoClose = true)
+      columnFamilies =
+        @[
+          initColFamilyDescriptor(CF_DEFAULT, defaultColFamilyOptions(autoClose = true))
+        ]
+      db = openRocksDb(dbPath, dbOpts, readOpts, writeOpts, columnFamilies).get()
 
     check:
       dbOpts.isClosed() == false
       readOpts.isClosed() == false
+      writeOpts.isClosed() == false
+      columnFamilies[0].isClosed() == false
+      db.isClosed() == false
+
+    db.close()
+
+    check:
+      dbOpts.isClosed() == true
+      readOpts.isClosed() == true
+      writeOpts.isClosed() == true
+      columnFamilies[0].isClosed() == true
+      db.isClosed() == true
+
+  test "Test auto close disabled":
+    let
+      dbPath = mkdtemp() / "autoclose-enabled"
+      dbOpts = defaultDbOptions(autoClose = false)
+      readOpts = defaultReadOptions(autoClose = false)
+      writeOpts = defaultWriteOptions(autoClose = false)
+      columnFamilies =
+        @[
+          initColFamilyDescriptor(
+            CF_DEFAULT, defaultColFamilyOptions(autoClose = false)
+          )
+        ]
+      db = openRocksDb(dbPath, dbOpts, readOpts, writeOpts, columnFamilies).get()
+
+    check:
+      dbOpts.isClosed() == false
+      readOpts.isClosed() == false
+      writeOpts.isClosed() == false
+      columnFamilies[0].isClosed() == false
       db.isClosed() == false
 
     db.close()
 
     check:
       dbOpts.isClosed() == false
-      readOpts.isClosed() == true
+      readOpts.isClosed() == false
+      writeOpts.isClosed() == false
+      columnFamilies[0].isClosed() == false
       db.isClosed() == true