Auto close feature to improve memory management of option types. (#57)

* Revert some changes from prior PR #48 which enable memory leak of options when not cleaned up manually.

* Clean up, add more tests and use correct free function for listColumnFamilies.

* Close opt types when opening database fails.

* Add autoClose flag to each opt type.

* Finish autoClose changes to prevent memory leaks.

rocksdb/backup.nim

@@ -25,17 +25,21 @@ type
     backupOpts: BackupEngineOptionsRef
 
 proc openBackupEngine*(
-    path: string, backupOpts = defaultBackupEngineOptions()
+    path: string, backupOpts = defaultBackupEngineOptions(autoClose = true)
 ): RocksDBResult[BackupEngineRef] =
   ## Create a new backup engine. The `path` parameter is the path of the backup
   ## directory. Note that the same directory should not be used for both backups
   ## and the database itself.
+  ##
+  ## If no `backupOpts` are provided, the default options will be used. These
+  ## default backup options will be closed when the backup engine is closed.
+  ## If `backupOpts` are provided, they will need to be closed manually.
 
   var errors: cstring
   let backupEnginePtr = rocksdb_backup_engine_open(
     backupOpts.cPtr, path.cstring, cast[cstringArray](errors.addr)
   )
-  bailOnErrors(errors)
+  bailOnErrors(errors, backupOpts = backupOpts)
 
   let engine =
     BackupEngineRef(cPtr: backupEnginePtr, path: path, backupOpts: backupOpts)
@@ -88,3 +92,6 @@ proc close*(backupEngine: BackupEngineRef) =
   if not backupEngine.isClosed():
     rocksdb_backup_engine_close(backupEngine.cPtr)
     backupEngine.cPtr = nil
+
+    if backupEngine.backupOpts.autoClose:
+      backupEngine.backupOpts.close()

rocksdb/columnfamily/cfdescriptor.nim

@@ -9,16 +9,18 @@
 
 {.push raises: [].}
 
-import ../internal/utils, ./cfopts
+import ./cfopts
 
 export cfopts
 
+const DEFAULT_COLUMN_FAMILY_NAME* = "default"
+
 type ColFamilyDescriptor* = object
   name: string
   options: ColFamilyOptionsRef
 
 proc initColFamilyDescriptor*(
-    name: string, options = defaultColFamilyOptions()
+    name: string, options: ColFamilyOptionsRef
 ): ColFamilyDescriptor =
   ColFamilyDescriptor(name: name, options: options)
 
@@ -28,11 +30,16 @@ proc name*(descriptor: ColFamilyDescriptor): string {.inline.} =
 proc options*(descriptor: ColFamilyDescriptor): ColFamilyOptionsRef {.inline.} =
   descriptor.options
 
+proc autoClose*(descriptor: ColFamilyDescriptor): bool {.inline.} =
+  descriptor.options.autoClose
+
 proc isDefault*(descriptor: ColFamilyDescriptor): bool {.inline.} =
   descriptor.name == DEFAULT_COLUMN_FAMILY_NAME
 
-proc defaultColFamilyDescriptor*(): ColFamilyDescriptor {.inline.} =
-  initColFamilyDescriptor(DEFAULT_COLUMN_FAMILY_NAME)
+proc defaultColFamilyDescriptor*(autoClose = false): ColFamilyDescriptor {.inline.} =
+  initColFamilyDescriptor(
+    DEFAULT_COLUMN_FAMILY_NAME, defaultColFamilyOptions(autoClose = autoClose)
+  )
 
 proc isClosed*(descriptor: ColFamilyDescriptor): bool {.inline.} =
   descriptor.options.isClosed()

rocksdb/columnfamily/cfopts.nim

@@ -23,6 +23,7 @@ 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
+    autoClose*: bool # if true then close will be called when the database is closed
 
   Compression* {.pure.} = enum
     # Use a slightly clunky name here to avoid global symbol conflicts
@@ -50,8 +51,8 @@ proc close*(s: SlicetransformRef) =
     rocksdb_slicetransform_destroy(s.cPtr)
     s.cPtr = nil
 
-proc newColFamilyOptions*(): ColFamilyOptionsRef =
-  ColFamilyOptionsRef(cPtr: rocksdb_options_create())
+proc newColFamilyOptions*(autoClose = false): ColFamilyOptionsRef =
+  ColFamilyOptionsRef(cPtr: rocksdb_options_create(), autoClose: autoClose)
 
 proc isClosed*(cfOpts: ColFamilyOptionsRef): bool {.inline.} =
   cfOpts.cPtr.isNil()
@@ -114,8 +115,8 @@ opt blobGCForceThreshold, float, cdouble
 opt blobCompactionReadaheadSize, int, uint64
 opt blobFileStartingLevel, int, cint
 
-proc defaultColFamilyOptions*(): ColFamilyOptionsRef =
-  newColFamilyOptions()
+proc defaultColFamilyOptions*(autoClose = false): ColFamilyOptionsRef =
+  newColFamilyOptions(autoClose)
 
 # proc setFixedPrefixExtractor*(dbOpts: ColFamilyOptionsRef, length: int) =
 #   doAssert not dbOpts.isClosed()

rocksdb/internal/utils.nim

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

rocksdb/options/backupopts.nim

@@ -16,9 +16,10 @@ type
 
   BackupEngineOptionsRef* = ref object
     cPtr: BackupEngineOptionsPtr
+    autoClose*: bool # if true then close will be called when the backup engine is closed
 
-proc newBackupEngineOptions*(): BackupEngineOptionsRef =
-  BackupEngineOptionsRef(cPtr: rocksdb_options_create())
+proc newBackupEngineOptions*(autoClose = false): BackupEngineOptionsRef =
+  BackupEngineOptionsRef(cPtr: rocksdb_options_create(), autoClose: autoClose)
 
 proc isClosed*(engineOpts: BackupEngineOptionsRef): bool {.inline.} =
   engineOpts.cPtr.isNil()
@@ -29,8 +30,11 @@ proc cPtr*(engineOpts: BackupEngineOptionsRef): BackupEngineOptionsPtr =
 
 # TODO: Add setters and getters for backup options properties.
 
-proc defaultBackupEngineOptions*(): BackupEngineOptionsRef {.inline.} =
-  let opts = newBackupEngineOptions()
+proc defaultBackupEngineOptions*(autoClose = false): BackupEngineOptionsRef {.inline.} =
+  let opts = newBackupEngineOptions(autoClose)
+
+  # TODO: set defaults here
+
   opts
 
 proc close*(engineOpts: BackupEngineOptionsRef) =

rocksdb/options/dbopts.nim

@@ -18,9 +18,10 @@ type
 
   DbOptionsRef* = ref object
     cPtr: DbOptionsPtr
+    autoClose*: bool # if true then close will be called when the database is closed
 
-proc newDbOptions*(): DbOptionsRef =
-  DbOptionsRef(cPtr: rocksdb_options_create())
+proc newDbOptions*(autoClose = false): DbOptionsRef =
+  DbOptionsRef(cPtr: rocksdb_options_create(), autoClose: autoClose)
 
 proc isClosed*(dbOpts: DbOptionsRef): bool {.inline.} =
   dbOpts.cPtr.isNil()
@@ -95,8 +96,8 @@ proc `rowCache=`*(dbOpts: DbOptionsRef, cache: CacheRef) =
   doAssert not dbOpts.isClosed()
   rocksdb_options_set_row_cache(dbOpts.cPtr, cache.cPtr)
 
-proc defaultDbOptions*(): DbOptionsRef =
-  let opts: DbOptionsRef = newDbOptions()
+proc defaultDbOptions*(autoClose = false): DbOptionsRef =
+  let opts: DbOptionsRef = newDbOptions(autoClose)
 
   # Optimize RocksDB. This is the easiest way to get RocksDB to perform well:
   opts.increaseParallelism(countProcessors())

rocksdb/options/readopts.nim

@@ -16,9 +16,10 @@ type
 
   ReadOptionsRef* = ref object
     cPtr: ReadOptionsPtr
+    autoClose*: bool # if true then close will be called when the database is closed
 
-proc newReadOptions*(): ReadOptionsRef =
-  ReadOptionsRef(cPtr: rocksdb_readoptions_create())
+proc newReadOptions*(autoClose = false): ReadOptionsRef =
+  ReadOptionsRef(cPtr: rocksdb_readoptions_create(), autoClose: autoClose)
 
 proc isClosed*(readOpts: ReadOptionsRef): bool {.inline.} =
   readOpts.cPtr.isNil()
@@ -29,8 +30,8 @@ proc cPtr*(readOpts: ReadOptionsRef): ReadOptionsPtr =
 
 # TODO: Add setters and getters for read options properties.
 
-proc defaultReadOptions*(): ReadOptionsRef {.inline.} =
-  newReadOptions()
+proc defaultReadOptions*(autoClose = false): ReadOptionsRef {.inline.} =
+  newReadOptions(autoClose)
   # TODO: set prefered defaults
 
 proc close*(readOpts: ReadOptionsRef) =

rocksdb/options/writeopts.nim

@@ -16,9 +16,10 @@ type
 
   WriteOptionsRef* = ref object
     cPtr: WriteOptionsPtr
+    autoClose*: bool # if true then close will be called when the database is closed
 
-proc newWriteOptions*(): WriteOptionsRef =
-  WriteOptionsRef(cPtr: rocksdb_writeoptions_create())
+proc newWriteOptions*(autoClose = false): WriteOptionsRef =
+  WriteOptionsRef(cPtr: rocksdb_writeoptions_create(), autoClose: autoClose)
 
 proc isClosed*(writeOpts: WriteOptionsRef): bool {.inline.} =
   writeOpts.cPtr.isNil()
@@ -29,8 +30,8 @@ proc cPtr*(writeOpts: WriteOptionsRef): WriteOptionsPtr =
 
 # TODO: Add setters and getters for write options properties.
 
-proc defaultWriteOptions*(): WriteOptionsRef {.inline.} =
-  newWriteOptions()
+proc defaultWriteOptions*(autoClose = false): WriteOptionsRef {.inline.} =
+  newWriteOptions(autoClose)
   # TODO: set prefered defaults
 
 proc close*(writeOpts: WriteOptionsRef) =