Close iterators

[elichai]

We use goleveldb for our database, and it explictly says that all iterators must be released:

The iterator must be released after use, by calling Release method.

https://pkg.go.dev/github.com/syndtr/goleveldb/leveldb#Transaction.NewIterator

I'm not sure if in practice this means we're leaking memory, because goleveldb uses runtime.SetFinalizer to call the Release, but we don't want to trust it works correctly, and also goleveldb never promised it's doing that, so they can always stop doing that.

I did not explictly invalidated the iterators, so some iterators could still be "used" after closing, and closing won't free their memory (assigning nil to all fields), but I'm open to discuss that with or without an explicit isClosed flag in all iterators.

[codecov]

Codecov Report

Merging #1542 (85097c5) into v0.9.0-dev (63b1d2a) will decrease coverage by 0.09%.
The diff coverage is 47.26%.

@@              Coverage Diff               @@
##           v0.9.0-dev    #1542      +/-   ##
==============================================
- Coverage       61.65%   61.56%   -0.10%     
==============================================
  Files             512      512              
  Lines           19499    19644     +145     
==============================================
+ Hits            12023    12093      +70     
- Misses           5757     5795      +38     
- Partials         1719     1756      +37     

Impacted Files	Coverage Δ
domain/consensus/test_consensus_render_to_dot.go	0.00% <0.00%> (ø)
domain/consensus/utils/utxo/utxo_iterator.go	47.05% <23.07%> (-14.85%)	⬇️
.../consensus/datastructures/blockstore/blockstore.go	52.06% <28.57%> (-3.08%)	⬇️
domain/consensus/database/cursor.go	48.14% <33.33%> (-21.86%)	⬇️
...astructures/pruningstore/imported_pruning_point.go	48.21% <41.17%> (-1.26%)	⬇️
...in/consensus/utils/utxo/utxo_iterator_with_diff.go	66.66% <42.30%> (-18.63%)	⬇️
...nsensus/datastructures/consensusstatestore/utxo.go	43.18% <44.44%> (+0.19%)	⬆️
...ses/dagtraversalmanager/selected_child_iterator.go	55.10% <52.94%> (-1.15%)	⬇️
domain/consensus/processes/syncmanager/antipast.go	41.17% <66.66%> (+0.57%)	⬆️
...onsensus/database/serialization/utxo_collection.go	71.42% <100.00%> (+1.42%)	⬆️
... and 10 more

---

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 63b1d2a...7b776ae. Read the comment docs.

[svarogg]

I think we want to make sure nobody uses iterators after Close, at the very least for the sanity of the thing, and later down the road to maybe release some resources.

Even just setting iterator.isClosed = true, and then erroring out on any function that returns an error can be a good start.

[elichai]

I think we want to make sure nobody uses iterators after Close, at the very least for the sanity of the thing, and later down the road to maybe release some resources.

Even just setting iterator.isClosed = true, and then erroring out on any function that returns an error can be a good start.

So do we want to add an isClosed boolean to all the iterators? (note that if you then call Next() we can only return false/true not an error)

[svarogg]

So do we want to add an isClosed boolean to all the iterators?

I believe we do.
"to all iterators" might be an exageration, some iterators might be able to signify "isClosed" in other ways, but in general - no problem with that.
In addition we might release any resources held, but less critical at this point IMHO.

(note that if you then call Next() we can only return false/true not an error)

Yeah, I thought about that and didn't really come up with a solution I'm 100% satisfied with.
I guess we should just return false when calling Next() on a closed iterator.

[elichai]

I guess we should just return false when calling Next() on a closed iterator.

If the Get() of that iterator returns an error we can return true and then set the error (We already do that in different cases when there's an error inside Next())

[svarogg]

If the Get() of that iterator returns an error we can return true and then set the error (We already do that in different cases when there's an error inside Next())

Yeah, I though about this, it indeed will achieve the desired behavior, at the price of correctness (Next returning true while there's no actual Next).
I'm starting to see the merits of this solution in a broader light, so I think I'm convinced. Let's do this as you suggested:
.Next() on closed iterator always returns true, then caller will call .Get(), that always returns an error.