CAD-3565 split openChainDB

[jasagredo]

The ChainDB initialization no longer runs inside an allocate call but instead runs with a temporary registry that will keep track of the resources allocated through initialization until the chainDB itself is allocated together with a handle that can close all the resources into the general registry.

For the new definitions in ResourceRegistry, check the haddock comments as well as the report.

[dnadales]

Looks good. I left some comments, which we could discuss on a video call.

[nfrisby]

Excellent additions to the report!

I'm Requesting Changes because I feel uncomfortable with the additions to the ResourceRegistry API. See my very long comment below. This is a high-level course correction request/proposal.

(Sorry for the delay; the review took longer than I thought, and I couldn't finish before my flight. Today is the first day we have some unscheduled time during the vacation :).)

[nfrisby]

Javier, Damian, and I discussed the new state of the PR. We're very happy with it.

The remaining work is to introduce the following function instead of allocateInRegistryBeforeReturning and to only use runInnerWithTempRegistry in ChainDB. In particular, the ChainDB component initializations should expose a WithTempRegistry function that can be passed to runWithTempRegistry -- the ChainDB init logic can then embellish those so that they can be passed to runInnerWithTempRegistry.

Those calls from the ChainDB init to the component inits will use \_ _ -> True as the checker -- our use of encapsulation prevents the ChainDB state from having enough access to its components' private state to properly run these checks at the end of ChainDB initizliation. This is not ideal, but we're accepting it as tech debt if not indefinitely.

• We need a tech debt ticket to track this infelicity.
• It's not urgent because the risk is small: it's only the resources that arise during initialization that might be untracked. (A few handles and simple threads.)
• Also, we're confident that the ChainDB does eg contain an ImmDB handle! So if the ImmDB initializes correctly (which is still checked at run-time to the same extent it was before), then skipping the check that "the ChainDB contains the ImmDB" (etc) seems safe enough.

-- | Embed a self-contained 'WithTempRegistry' computation into a larger one.
--
-- The internal 'WithTempRegistry' is effectively passed to
-- 'runWithTempRegistry'. It therefore must have no dangling resources, for
-- example. This is the meaning of /self-contained/ above.
--
-- The key difference beyond 'runWithTempRegistry' is that the resulting
-- composite resource is also guaranteed to be registered in the outer
-- 'WithTempRegistry' computation's registry once the inner registry is closed.
-- Combined with the following assumption, this establishes the invariant that
-- all resources are (transitively) in a temporary registry.
--
-- ASSUMPTION: closing @res@ closes everything contained in @inner_st@
--
-- NOTE: In the current implementation, there will be a brief moment where the
-- inner registry still contains the inner computation's resources and also the
-- outer registry simultaneously contains the new composite resource. If an async
-- is received at that time, then the inner resources will be closed and then
-- the composite resource will be later closed. This means there's a risk of
-- /double freeing/, which can be harmless if anticipated.
runInnerWithTempRegistry
  :: forall inner_st st m res a. IOLike m
  => WithTempRegistry inner_st m (a, inner_st, res)
     -- ^ The embedded computation; see ASSUMPTION above
  -> (res -> m Bool)
     -- ^ How to release; same as for 'allocateTemp'
  -> (st -> res -> Bool)
     -- ^ How to check; same as for 'allocateTemp'
  -> WithTempRegistry st m a
runInnerWithTempRegistry inner free isTransferred = do
    outerTR <- WithTempRegistry ask

    lift $ runWithTempRegistry $ do
      (a, inner_st, res) <- inner

      -- allocate in the outer layer
      _ <-   withFixedTempRegistry outerTR
           $ allocateTemp (return res) free isTransferred

      -- TODO This point here is where an async exception could cause both the
      -- inner resources to be closed and the outer resource to be closed later
      --
      -- If we want to do better than that, we'll need a variant of
      -- 'runWithTempRegistry' that lets us perform some action with async
      -- exceptions masked "at the same time" it closes its registry.

      pure (a, inner_st)
  where
    withFixedTempRegistry env (WithTempRegistry (ReaderT f)) =
      WithTempRegistry $ ReaderT $ \_ -> f env

[jasagredo]

Rebased and commits reworded to prepare for the merge

[nfrisby]

Looks great. Again, I especially appreciate the nice write-up in the report!

I'm Requesting Changes again because I think we can simplify the openDB functions appreciably. I pushed up a commit showing how I think it should be; see the PR comments below. Please criticize it, or just squash it in if you like it. The key idea is that even the openDB functions do not need to leak the internal state type.

I also moved the free function out of the monadic computation. At the highest level, that's 1) to simplify the types and 2) to make it more strongly resemble the bracket API.

Edit: several of these comments are replies to Damian's comments -- GitHub unhelpfully does not indicate that below :(

[nfrisby]

Approving! Great PR!

I made a couple minor requests, as final touches.

Would you also change the name of my commit you cherry-picked (at least remove the WIP part)? Or squash it down to eliminate it? The PR description already looks good 👍

[jasagredo]

bors r+

[iohk-bors]

Build succeeded:

• ci/hydra:Cardano:ouroboros-network:required
• buildkite/ouroboros-network