gm42 commented 1 month ago

Setting the finalizer prevents user from calling Close(), as it would randomly result in SIGSEGV or some other silent memory corruption.

See https://github.com/apple/foundationdb/pull/11383#issuecomment-2110368596 for an example.

The change consists in setting no finalizer for the database and then expecting user to call Close() to avoid memory leaks.

NOTE: this must be considered a breaking change, since clients who do not update their usage of the binding will automatically upgrade to a memory leak if they open many databases without closing any.

Code-Reviewer Section

The general pull request guidelines can be found here.

Please check each of the following things and check all boxes before accepting a PR.

[x] The PR has a description, explaining both the problem and the solution.
[ ] The description mentions which forms of testing were done and the testing seems reasonable.
[x] Every function/class/actor that was touched is reasonably well documented.

For Release-Branches

If this PR is made against a release-branch, please also check the following:

[ ] This change/bugfix is a cherry-pick from the next younger branch (younger release-branch or main if this is the youngest branch)
[ ] There is a good reason why this PR needs to go into a release branch and this reason is documented (either in the description above or in a linked GitHub issue)

gm42 commented 1 month ago

Cc @johscheuer

Alternative approach would be: to use an atomic.Value or a mutex to make sure that calling destroy() multiple times is safe. I can change the PR for that approach, if it's preferable.

In the Go binding there's only 3 objects which have a finalizer:

transactions
futures
database

And currently only database has an user-reachable Close(), thus the other two use-cases are unaffected by this issue. Since databases are (usually) not created with high frequency introducing a mutex or atomic.Value is acceptable as the little performance cost is not expected to crop up.

gm42 commented 1 month ago

I have to take some time to go over the changes.

Understood, thanks for the info; I would prefer as well that you take all the time needed as rush can only favor the introduction of bugs.

In general I would prefer if all structs like Database, Future and Transaction would implement a Close() method. That would give a user more control over the life cycle of the structs and would fit well into the go model where you just define a defer method to clean up the resources.

I think that's precisely the more idiomatic Go way e.g. instead of setting finalizers letting user call Close() on defer. If in future the binding would expose Close() methods for transactions and futures as well, the same issue I raised here would appear e.g. the function called by finalizer and the function called by user (Close()) must be safe to call concurrently and multiple times.

Given that constraint I can see only two possible approaches:

removing the finalizer (as done in this PR)
using a mutex or atomic.Value to make sure that multiple calls are safe

Let me know if you prefer to retool for (2), after you have time to look into the changes.