Fix double allocate of resources in getStateRef

[jasagredo]

Description

The duplicate function for LSM trees is already allocating the tables in a resource registry. We were then allocating this resource again in the registry and we would only free this second allocation if we never committed the forker thus not replacing foeResourcesToRelease. If we replaced it with closeDiscarded, we would point to the close of the tables but forget about the resource allocated by getStateRef which would hold a reference to the tables forever.

[bladyjoker]

Nice catch! Do you think it makes sense to add some assertions on the registry itself. For example if we don't expect more than 3k items in there, we could provide an assertion in a similar manner we do with NoThunks for detecting thunks.

[jasagredo]

@bladyjoker I think the process of finding that something is retained by the resource registry is "easy" enough to find that something is retained:

1. A -l -hi run shows that some type is being retained.
2. Using ghc-debug we save a reference to that object we are retaining.
3. With a very quick findRetainersOf on a snapshot of the heap we can see that the resource registry is the one responsible for retaining it.

Another issue is that the problem is not allocating a new resource but instead that the previous resources still remain, so such an assertion might flag a false positive. It reminds me of limiting the allocation size of the program in the RTS in the hopes that you hit exactly the allocation that goes over the limit, which is a brittle approach.

[jasagredo]

The problem is not exactly the one you described.

1. We duplicate the handle, adding it to the lgrRegistry, with a release function that will call closeTable and a close function that will call release on lgrRegistry.
2. We then allocate (again!) this duplicated handle in the registry used to create the forker (let's call it forkerRegistry), with a release function that will
   1. call close on the LedgerTablesHandle which will
   2. call release on lgrRegistry which will
   3. call closeTable.
3. If we don't commit the forker and close it, we will run the release function from step 2, which will
   1. release it from the forkerRegistry which will
   2. call close on the LedgerTablesHandle which will
   3. call release on lgrRegistry which will
   4. call closeTable.
4. If we commit the forker, we will:
   1. call close on the LedgerTablesHandle which will
   2. call release on lgrRegistry which will
   3. call closeTable.

Notice how in option 4 we are missing the release from the forkerRegistry. That is where the handle was being retained (as it will hold a reference to it to be able to call close on it).

The way I solved it was by not allocating again the LedgerTablesHandle in the forkerRegistry, but it probably has some downsides like the one you mentioned about losing again exception safety...

I think we could solve this by passing around the key that needs to be released upon commit, so that the line

close $ tables $ AS.anchor lseq 

instead releases such key.

---

Re-doing this in a more obviously correct way sounds like a good idea, but perhaps we could defer it to a later PR? I actually think this PR could be merged without all the LSM stuff.

[amesgen]

That's how I understood the issue, maybe

In this PR, you fix this by using close in both cases.

was misleading in #1702 (comment); I edited it to

In this PR, you fix this by using close in both cases and no longer registering the LedgerTablesHandle in the forker-specific registry.

for clarity.

---

Also, the fact that the LSM V2 backend happens to use ResourceRegistrys internally isn't relevant here, right? Eg even if it didn't use a ResourceRegistry internally somehow, we would still have the exact same resource leak.

---

I think we could solve this by passing around the key that needs to be released upon commit, so that the line

close $ tables $ AS.anchor lseq 

instead releases such key.

FTR that's exactly what I also mentioned in #1702 (comment), see "This would also be the exact place where we would need to close via ResourceKey in the dual approach mentioned above."

---

Re-doing this in a more obviously correct way sounds like a good idea, but perhaps we could defer it to a later PR? I actually think this PR could be merged without all the LSM stuff.

That sounds good to me, let's open an issue for the first point 👍

[jasagredo]

The issue for simplifying this handling: #1704