IntersectMBO / IntersectMBO/ouroboros-consensus

Revisit use of `ResourceRegistry` in `ChainDB`

Open
#764 0 comments 0 reactions 0 assignees View on GitHub
technical debt
Dominant language
Haskell
Stars
67
Forks
43
Avg merge
5d 13h
Merged PRs (30d)
43

Description

The design of the `ChainDB` predates the redesign of the `ResourceRegistry`. This is visible in a number of ways. For example, internally the `ChainDB` has some registry-like environments such as `cdbBgThreads :: TVar m [Thread m ()]`, `cdbIterators :: TVar m (Map IteratorId (m ()))`; we can probably use a standard registry for these.

More importantly, some of the `ChainDB` functions take a `ResourceRegistry` as argument (specifically, `streamBlocks`, `newHeaderReader`/`newBlockReader`). I think this is _mostly_ just an artifact of the previous restriction that a `ResourceRegistry` could only be used from a single thread. However, the new registry still imposes _some_ restrictions; specifically, the thread using it must be "known" to the registry (implying that its lifetime is limited by the lifetime of that registry). In other words, if thread `T` uses registry `R` and calls `streamBlocks` with `R`, then that registry will throw a runtime exception if `T` wasn't spawned using `R`. This is not unreasonable, since that registry might disappear before `T` does.

In some cases however (https://github.com/input-output-hk/cardano-byron-proxy/pull/37 being one example), we can't even use the same registry as the one that created the `ChainDB` in the first place. But this doesn't really make sense: if `streamBlocks` refuses to allocate something in a registry `R` because that registry might die before the thread does, _but the chain database itself is allocated in that same registry_`, surely the whole chain DB disappearing would be an even greater problem? In a way, the ChainDB deals with this internally, "out of band" (for example, the `ChainDbClosed` exception).

I already relaxed the deallocation code of an iterator so that an iterator can be passed to another thread and when it deallocates itself from its registry, it skips the "is known thread check". Perhaps we should do this for allocation as well, and remove the `ResourceRegistry` arguments altogether from the `ChainDB` API. If not, we should at least document how these arguments should be chosen, and how that affects the caller.

Contributor guide

Open the contributing guide

Assessment

This issue has not been assessed yet.

Get new issues in your inbox

A short digest of beginner-friendly GitHub issues.