Skip to content

Commit

Permalink
Aristo+Kvt: Let destructor crash when nil argument is given (#2080)
Browse files Browse the repository at this point in the history
why:
  Ignoring `nil` objects was handy for a while but eventually led to
  lazy programming which in turn led to double destructor calls for
  the rocks-db.
  • Loading branch information
mjfh authored Mar 15, 2024
1 parent 90622c0 commit 5379302
Show file tree
Hide file tree
Showing 5 changed files with 29 additions and 26 deletions.
7 changes: 3 additions & 4 deletions nimbus/db/aristo/aristo_init/memory_only.nim
Original file line number Diff line number Diff line change
Expand Up @@ -98,10 +98,9 @@ proc finish*(db: AristoDbRef; flush = false) =
##
## This distructor may be used on already *destructed* descriptors.
##
if not db.isNil:
if not db.backend.isNil:
db.backend.closeFn flush
discard db.getCentre.forgetOthers()
if not db.backend.isNil:
db.backend.closeFn flush
discard db.getCentre.forgetOthers()

# ------------------------------------------------------------------------------
# End
Expand Down
7 changes: 3 additions & 4 deletions nimbus/db/kvt/kvt_init/memory_only.nim
Original file line number Diff line number Diff line change
Expand Up @@ -73,10 +73,9 @@ proc finish*(db: KvtDbRef; flush = false) =
##
## This distructor may be used on already *destructed* descriptors.
##
if not db.isNil:
if not db.backend.isNil:
db.backend.closeFn flush
discard db.getCentre.forgetOthers()
if not db.backend.isNil:
db.backend.closeFn flush
discard db.getCentre.forgetOthers()

# ------------------------------------------------------------------------------
# End
Expand Down
4 changes: 2 additions & 2 deletions tests/test_aristo.nim
Original file line number Diff line number Diff line change
Expand Up @@ -183,8 +183,8 @@ proc storagesRunner(

proc aristoMain*(noisy = defined(debug)) =
noisy.miscRunner()
noisy.accountsRunner(persistent=false)
noisy.storagesRunner(persistent=false)
noisy.accountsRunner()
noisy.storagesRunner()

when isMainModule:
const
Expand Down
10 changes: 6 additions & 4 deletions tests/test_aristo/test_filter.nim
Original file line number Diff line number Diff line change
Expand Up @@ -202,8 +202,10 @@ proc dbTriplet(w: LeafQuartet; rdbPath: string): Result[DbTriplet,AristoError] =

# ----------------------

proc cleanUp(dx: DbTriplet) =
dx[0].finish(flush=true)
proc cleanUp(dx: var DbTriplet) =
if not dx[0].isNil:
dx[0].finish(flush=true)
dx.reset

proc isDbEq(a, b: FilterRef; db: AristoDbRef; noisy = true): bool =
## Verify that argument filter `a` has the same effect on the
Expand Down Expand Up @@ -559,7 +561,7 @@ proc testDistributedAccess*(
block:

# Clause (8) from `aristo/README.md` example
let
var
dx = block:
let rc = dbTriplet(w, rdbPath)
xCheckRc rc.error == 0
Expand Down Expand Up @@ -612,7 +614,7 @@ proc testDistributedAccess*(

# Work through clauses (12)..(15) from `aristo/README.md` example
block:
let
var
dy = block:
let rc = dbTriplet(w, rdbPath)
xCheckRc rc.error == 0
Expand Down
27 changes: 15 additions & 12 deletions tests/test_aristo/test_tx.nim
Original file line number Diff line number Diff line change
Expand Up @@ -108,13 +108,15 @@ proc randomisedLeafs(
lvp2[n].swap lvp2[r]
ok lvp2

proc innerCleanUp(db: AristoDbRef): bool {.discardable.} =
proc innerCleanUp(db: var AristoDbRef): bool {.discardable.} =
## Defer action
let rx = db.txTop()
if rx.isOk:
let rc = rx.value.collapse(commit=false)
xCheckRc rc.error == 0
db.finish(flush=true)
if not db.isNil:
let rx = db.txTop()
if rx.isOk:
let rc = rx.value.collapse(commit=false)
xCheckRc rc.error == 0
db.finish(flush=true)
db = AristoDbRef(nil)

proc schedStow(
db: AristoDbRef; # Database
Expand Down Expand Up @@ -334,10 +336,11 @@ proc testTxMergeAndDeleteOneByOne*(
): bool =
var
prng = PrngDesc.init 42
db = AristoDbRef()
db = AristoDbRef(nil)
fwdRevVfyToggle = true
defer:
db.finish(flush=true)
if not db.isNil:
db.finish(flush=true)

for n,w in list:
# Start with brand new persistent database.
Expand Down Expand Up @@ -439,10 +442,10 @@ proc testTxMergeAndDeleteSubTree*(
): bool =
var
prng = PrngDesc.init 42
db = AristoDbRef()

db = AristoDbRef(nil)
defer:
db.finish(flush=true)
if not db.isNil:
db.finish(flush=true)

for n,w in list:
# Start with brand new persistent database.
Expand Down Expand Up @@ -524,7 +527,7 @@ proc testTxMergeProofAndKvpList*(
let
oopsTab = oops.toTable
var
db = AristoDbRef()
db = AristoDbRef(nil)
tx = AristoTxRef(nil)
rootKey: Hash256
count = 0
Expand Down

0 comments on commit 5379302

Please sign in to comment.