Add tests to cover autoClose.

rocksdb/columnfamily/cfopts.nim

@@ -11,12 +11,13 @@
 
 import ../lib/librocksdb, ../internal/utils, ../options/tableopts
 
+export 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
 
@@ -25,7 +26,6 @@ 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
 
@@ -40,11 +40,8 @@ type
     xpressCompression = rocksdb_xpress_compression
     zstdCompression = rocksdb_zstd_compression
 
-proc createFixedPrefix*(value: int, autoClose = false): SlicetransformRef =
-  SlicetransformRef(
-    cPtr: rocksdb_slicetransform_create_fixed_prefix(value.csize_t),
-    autoClose: autoClose,
-  )
+proc createFixedPrefix*(value: int): SlicetransformRef =
+  SlicetransformRef(cPtr: rocksdb_slicetransform_create_fixed_prefix(value.csize_t))
 
 proc isClosed*(s: SlicetransformRef): bool {.inline.} =
   s.cPtr.isNil()
@@ -73,7 +70,6 @@ proc close*(cfOpts: ColFamilyOptionsRef) =
     rocksdb_options_destroy(cfOpts.cPtr)
     cfOpts.cPtr = nil
 
-    autoCloseNonNil(cfOpts.sliceTransform)
     autoCloseNonNil(cfOpts.tableOpts)
 
 template opt(nname, ntyp, ctyp: untyped) =
@@ -135,11 +131,14 @@ 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
 
+  # Destroys the existing slice transform if there is one attached to the column
+  # family options and takes ownership of the passed slice transform. After this call,
+  # the ColFamilyOptionsRef is responsible for cleaning up the policy when it is no
+  # longer needed so we set the filter policy to nil so that isClosed() will return true
+  # and prevent the filter policy from being double freed which was causing a seg fault.
   rocksdb_options_set_prefix_extractor(cfOpts.cPtr, value.cPtr)
-  cfOpts.sliceTransform = value
+  value.cPtr = nil
 
 proc `blockBasedTableFactory=`*(
     cfOpts: ColFamilyOptionsRef, tableOpts: TableOptionsRef

rocksdb/options/cache.nim

@@ -10,6 +10,9 @@ type
 proc cacheCreateLRU*(size: int, autoClose = false): CacheRef =
   CacheRef(cPtr: rocksdb_cache_create_lru(size.csize_t), autoClose: autoClose)
 
+proc isClosed*(cache: CacheRef): bool =
+  isNil(cache.cPtr)
+
 proc close*(cache: CacheRef) =
   if cache.cPtr != nil:
     rocksdb_cache_destroy(cache.cPtr)

rocksdb/options/tableopts.nim

@@ -1,5 +1,7 @@
 import ../lib/librocksdb, ../internal/utils, ./cache
 
+export cache
+
 type
   # TODO might eventually wrap this
   TableOptionsPtr* = ptr rocksdb_block_based_table_options_t

tests/columnfamily/test_cfopts.nim

@@ -20,3 +20,45 @@ suite "ColFamilyOptionsRef Tests":
     check cfOpts.isClosed()
     cfOpts.close()
     check cfOpts.isClosed()
+
+  test "Test auto close enabled":
+    let
+      cfOpts = defaultColFamilyOptions()
+      tableOpts = defaultTableOptions(autoClose = true)
+      sliceTransform = createFixedPrefix(1000)
+
+    cfOpts.blockBasedTableFactory = tableOpts
+    cfOpts.setPrefixExtractor(sliceTransform)
+
+    check:
+      cfOpts.isClosed() == false
+      tableOpts.isClosed() == false
+      sliceTransform.isClosed() == true # closed because tableopts takes ownership
+
+    cfOpts.close()
+
+    check:
+      cfOpts.isClosed() == true
+      tableOpts.isClosed() == true
+      sliceTransform.isClosed() == true
+
+  test "Test auto close disabled":
+    let
+      cfOpts = defaultColFamilyOptions()
+      tableOpts = defaultTableOptions(autoClose = false)
+      sliceTransform = createFixedPrefix(1000)
+
+    cfOpts.blockBasedTableFactory = tableOpts
+    cfOpts.setPrefixExtractor(sliceTransform)
+
+    check:
+      cfOpts.isClosed() == false
+      tableOpts.isClosed() == false
+      sliceTransform.isClosed() == true # closed because tableopts takes ownership
+
+    cfOpts.close()
+
+    check:
+      cfOpts.isClosed() == true
+      tableOpts.isClosed() == false
+      sliceTransform.isClosed() == true

tests/options/test_dbopts.nim

@@ -13,7 +13,7 @@ import unittest2, ../../rocksdb/options/dbopts
 
 suite "DbOptionsRef Tests":
   test "Test newDbOptions":
-    var dbOpts = newDbOptions()
+    let dbOpts = newDbOptions()
 
     check not dbOpts.cPtr.isNil()
 
@@ -28,10 +28,27 @@ suite "DbOptionsRef Tests":
     dbOpts.close()
 
   test "Test close":
-    var dbOpts = defaultDbOptions()
+    let dbOpts = defaultDbOptions()
 
     check not dbOpts.isClosed()
     dbOpts.close()
     check dbOpts.isClosed()
     dbOpts.close()
     check dbOpts.isClosed()
+
+  test "Test auto close enabled":
+    let
+      dbOpts = defaultDbOptions()
+      cache = cacheCreateLRU(1000, autoClose = true)
+
+    dbOpts.rowCache = cache
+
+    check:
+      dbOpts.isClosed() == false
+      cache.isClosed() == false
+
+    dbOpts.close()
+
+    check:
+      dbOpts.isClosed() == true
+      cache.isClosed() == true

tests/options/test_tableopts.nim

@@ -0,0 +1,80 @@
+# Nim-RocksDB
+# Copyright 2024 Status Research & Development GmbH
+# Licensed under either of
+#
+#  * Apache License, version 2.0, ([LICENSE-APACHE](LICENSE-APACHE) or http://www.apache.org/licenses/LICENSE-2.0)
+#  * GPL license, version 2.0, ([LICENSE-GPLv2](LICENSE-GPLv2) or https://www.gnu.org/licenses/old-licenses/gpl-2.0.en.html)
+#
+# at your option. This file may not be copied, modified, or distributed except according to those terms.
+
+{.used.}
+
+import unittest2, ../../rocksdb/options/tableopts
+
+suite "TableOptionsRef Tests":
+  test "Test createTableOptions":
+    let tableOpts = createTableOptions()
+
+    check not tableOpts.cPtr.isNil()
+
+    tableOpts.close()
+
+  test "Test defaultTableOptions":
+    let tableOpts = defaultTableOptions()
+
+    check not tableOpts.cPtr.isNil()
+
+    tableOpts.close()
+
+  test "Test close":
+    let tableOpts = defaultTableOptions()
+
+    check not tableOpts.isClosed()
+    tableOpts.close()
+    check tableOpts.isClosed()
+    tableOpts.close()
+    check tableOpts.isClosed()
+
+  test "Test auto close enabled":
+    # TODO: enable filter policy once creating updated DLL build
+    let
+      tableOpts = defaultTableOptions()
+      cache = cacheCreateLRU(1000, autoClose = true)
+      # filter = createRibbon(9.9, autoClose = true)
+
+    tableOpts.blockCache = cache
+    # tableOpts.filterPolicy = filter
+
+    check:
+      tableOpts.isClosed() == false
+      cache.isClosed() == false
+      # filter.isClosed() == true # closed because tableopts takes ownership
+
+    tableOpts.close()
+
+    check:
+      tableOpts.isClosed() == true
+      cache.isClosed() == true
+      # filter.isClosed() == true
+
+  test "Test auto close disabled":
+    # TODO: enable filter policy once creating updated DLL build
+    let
+      tableOpts = defaultTableOptions()
+      cache = cacheCreateLRU(1000, autoClose = false)
+      # filter = createRibbon(9.9, autoClose = true)
+
+    tableOpts.blockCache = cache
+    # tableOpts.filterPolicy = filter
+
+    check:
+      tableOpts.isClosed() == false
+      cache.isClosed() == false
+      # filter.isClosed() == true # closed because tableopts takes ownership
+
+    tableOpts.close()
+
+    check:
+      tableOpts.isClosed() == true
+      cache.isClosed() == false
+      # filter.isClosed() == true

tests/test_all.nim

@@ -17,6 +17,7 @@ import
   ./options/test_dbopts,
   ./options/test_readopts,
   ./options/test_writeopts,
+  ./options/test_tableopts,
   ./transactions/test_txdbopts,
   ./transactions/test_txopts,
   ./test_backup,

tests/test_backup.nim

@@ -59,3 +59,33 @@ suite "BackupEngineRef Tests":
     check engine.isClosed()
     engine.close()
     check engine.isClosed()
+
+  test "Test auto close enabled":
+    let
+      backupOpts = defaultBackupEngineOptions(autoClose = true)
+      backupEngine = openBackupEngine(dbPath, backupOpts).get()
+
+    check:
+      backupOpts.isClosed() == false
+      backupEngine.isClosed() == false
+
+    backupEngine.close()
+
+    check:
+      backupOpts.isClosed() == true
+      backupEngine.isClosed() == true
+
+  test "Test auto close disabled":
+    let
+      backupOpts = defaultBackupEngineOptions(autoClose = false)
+      backupEngine = openBackupEngine(dbPath, backupOpts).get()
+
+    check:
+      backupOpts.isClosed() == false
+      backupEngine.isClosed() == false
+
+    backupEngine.close()
+
+    check:
+      backupOpts.isClosed() == false
+      backupEngine.isClosed() == true

tests/test_rocksdb.nim

@@ -380,7 +380,7 @@ suite "RocksDbRef Tests":
 
   test "Test auto close disabled":
     let
-      dbPath = mkdtemp() / "autoclose-enabled"
+      dbPath = mkdtemp() / "autoclose-disabled"
       dbOpts = defaultDbOptions(autoClose = false)
       readOpts = defaultReadOptions(autoClose = false)
       writeOpts = defaultWriteOptions(autoClose = false)

tests/test_sstfilewriter.nim

@@ -86,3 +86,33 @@ suite "SstFileWriterRef Tests":
     check writer.isClosed()
     writer.close()
     check writer.isClosed()
+
+  test "Test auto close enabled":
+    let
+      dbOpts = defaultDbOptions(autoClose = true)
+      writer = openSstFileWriter(sstFilePath, dbOpts).get()
+
+    check:
+      dbOpts.isClosed() == false
+      writer.isClosed() == false
+
+    writer.close()
+
+    check:
+      dbOpts.isClosed() == true
+      writer.isClosed() == true
+
+  test "Test auto close disabled":
+    let
+      dbOpts = defaultDbOptions(autoClose = false)
+      writer = openSstFileWriter(sstFilePath, dbOpts).get()
+
+    check:
+      dbOpts.isClosed() == false
+      writer.isClosed() == false
+
+    writer.close()
+
+    check:
+      dbOpts.isClosed() == false
+      writer.isClosed() == true