IntersectMBO / IntersectMBO/ouroboros-consensus
Revisit use of `ResourceRegistry` in `ChainDB`
- 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
Assessment
This issue has not been assessed yet.