diff --git a/rocksdb/backup.nim b/rocksdb/backup.nim index 7c88bc5..98301b4 100644 --- a/rocksdb/backup.nim +++ b/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) diff --git a/rocksdb/columnfamily/cfopts.nim b/rocksdb/columnfamily/cfopts.nim index ae7e237..f73c8f6 100644 --- a/rocksdb/columnfamily/cfopts.nim +++ b/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*( diff --git a/rocksdb/internal/utils.nim b/rocksdb/internal/utils.nim index 2de5923..e4ba78a 100644 --- a/rocksdb/internal/utils.nim +++ b/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 diff --git a/rocksdb/options/cache.nim b/rocksdb/options/cache.nim index 9d53b3a..2024852 100644 --- a/rocksdb/options/cache.nim +++ b/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: diff --git a/rocksdb/options/dbopts.nim b/rocksdb/options/dbopts.nim index e3f9c0f..36fc08a 100644 --- a/rocksdb/options/dbopts.nim +++ b/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) diff --git a/rocksdb/options/tableopts.nim b/rocksdb/options/tableopts.nim index e8bdba8..ec8e31d 100644 --- a/rocksdb/options/tableopts.nim +++ b/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 diff --git a/rocksdb/rocksdb.nim b/rocksdb/rocksdb.nim index a5443d7..e757313 100644 --- a/rocksdb/rocksdb.nim +++ b/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 diff --git a/rocksdb/sstfilewriter.nim b/rocksdb/sstfilewriter.nim index 4160850..107cd48 100644 --- a/rocksdb/sstfilewriter.nim +++ b/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) diff --git a/rocksdb/transactiondb.nim b/rocksdb/transactiondb.nim index f449781..ba591d5 100644 --- a/rocksdb/transactiondb.nim +++ b/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) diff --git a/rocksdb/transactions/transaction.nim b/rocksdb/transactions/transaction.nim index f268c9a..51cc222 100644 --- a/rocksdb/transactions/transaction.nim +++ b/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) diff --git a/tests/test_rocksdb.nim b/tests/test_rocksdb.nim index 7c3480d..2d58052 100644 --- a/tests/test_rocksdb.nim +++ b/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