Skip to content

Go: [BREAKING] explicit destruction for futures and transactions #12052

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom
Open
wants to merge 3 commits into
base: main
Choose a base branch
from
Open

Go: [BREAKING] explicit destruction for futures and transactions #12052

wants to merge 3 commits into from

Conversation

gm42
Copy link
Collaborator

@gm42 gm42 commented Mar 26, 2025 •
edited
Loading

Explicit destruction for futures and transactions.
Introduction of context.

This is a breaking change

This change makes the Go binding stop relying on Go's GC finalizers and instead use explicit Close() calls to release resources, which is more Go-idiomatic; additionally, context.Context is added to function signatures and supported through transaction cancellation.

NOTE: once merged this will be a breaking change for users because memory leaks would start appear on their FoundationDB clients, unless Close() is called for futures, transactions and range iterators.

Code-Reviewer Section

  • 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.
  • Every function/class/actor that was touched is reasonably well documented.

I have not yet updated test code as I'd like to discuss this change first.

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 gm42 requested a review from johscheuer March 26, 2025 10:02
@gm42 gm42 mentioned this pull request Mar 26, 2025
Open
gm42
gm42 commented Mar 26, 2025
View reviewed changes
bindings/go/src/fdb/range.go
return
}

kv = ri.kvs[ri.index]
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Note: on next advance call, this slice is thrown away. I'd like to address this in this PR, or a separate one, by having Advance() return the whole slice.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I ended up switching from Advance()/GetSlice() to NextBatch()/Get()

johscheuer
johscheuer reviewed Mar 26, 2025
View reviewed changes
Copy link
Contributor

@johscheuer johscheuer left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the PR! Could we update the test cases to make use of the new close method (and directly document there how to use them)?

bindings/go/src/fdb/transaction.go
@@ -88,7 +88,9 @@ func (opt TransactionOptions) setOpt(code int, param []byte) error {
}, param)
}

func (t *transaction) destroy() {
// Close will destroy the underlying transaction.
// It must be called after all the transaction-associated futures have been closed.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should we document what happens if the transaction is closed before all the associated futures have been closed?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In fact destroying the transaction causes the transaction (and all its futures) to be cancelled, so this needs some re-thinking/re-writing as well.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I will make some attempts and submit a change; I am thinking to return a "closer" function that caller can defer.

Copy link
Collaborator Author

@gm42 gm42 Mar 27, 2025 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I've submitted a version of this; will update all tests code.

This change also introduces an implementation of context with automatic cancellation, as I thought it would be a good time to break the function signature only once.

@gm42
Go: [BREAKING] explicit destruction for futures and transactions
e0d6856 
Stop relying on Go's GC finalizers and instead use explicit Close() calls
to release resources, which is more Go-idiomatic.
The Get() method is removed from RangeIterator.

NOTE: this would be a breaking change for users because without any code breakage
they would now have huge memory leaks on their FoundationDB clients; a release management
solution must be devised for this problem.
@gm42
Go: rename Advance() as NextBatch(), remove RangeIterator.Get()
534b1fe 
Other functions renamed:
* GetSliceWithError() -> Get()
* GetSliceOrPanic() -> MustGet()
@johscheuer johscheuer closed this Apr 1, 2025
@johscheuer johscheuer reopened this Apr 1, 2025
@foundationdb-ci
Copy link
Contributor

Result of foundationdb-pr-clang-ide on Linux CentOS 7

  • Commit ID: 1e768c3
  • Duration 0:22:16
  • Result: ✅ SUCCEEDED
  • Error: N/A
  • Build Log terminal output (available for 30 days)
  • Build Workspace zip file of the working directory (available for 30 days)

@foundationdb-ci
Copy link
Contributor

Result of foundationdb-pr-clang on Linux CentOS 7

  • Commit ID: 1e768c3
  • Duration 0:31:43
  • Result: ❌ FAILED
  • Error: Error while executing command: ninja -v -C build_output -j ${NPROC} all packages strip_targets. Reason: exit status 1
  • Build Log terminal output (available for 30 days)
  • Build Workspace zip file of the working directory (available for 30 days)

@foundationdb-ci
Copy link
Contributor

Result of foundationdb-pr-macos-m1 on macOS Ventura 13.x

  • Commit ID: 1e768c3
  • Duration 0:35:50
  • Result: ❌ FAILED
  • Error: Error while executing command: ssh -o StrictHostKeyChecking=no -o UserKnownHostsFile=/dev/null -i ${HOME}/.ssh_key ec2-user@${MAC_EC2_HOST} /opt/homebrew/bin/bash --login -c ./build_pr_macos.sh. Reason: exit status 1
  • Build Log terminal output (available for 30 days)
  • Build Workspace zip file of the working directory (available for 30 days)

@foundationdb-ci
Copy link
Contributor

Result of foundationdb-pr on Linux CentOS 7

  • Commit ID: 1e768c3
  • Duration 0:39:30
  • Result: ❌ FAILED
  • Error: Error while executing command: ninja -v -C build_output -j ${NPROC} all packages strip_targets. Reason: exit status 1
  • Build Log terminal output (available for 30 days)
  • Build Workspace zip file of the working directory (available for 30 days)

@foundationdb-ci
Copy link
Contributor

Result of foundationdb-pr-cluster-tests on Linux CentOS 7

  • Commit ID: 1e768c3
  • Duration 0:40:00
  • Result: ❌ FAILED
  • Error: Error while executing command: ninja -v -C build_output -j ${NPROC} all packages strip_targets. Reason: exit status 1
  • Build Log terminal output (available for 30 days)
  • Build Workspace zip file of the working directory (available for 30 days)
  • Cluster Test Logs zip file of the test logs (available for 30 days)

@foundationdb-ci
Copy link
Contributor

Result of foundationdb-pr-clang-arm on Linux CentOS 7

  • Commit ID: 1e768c3
  • Duration 0:45:09
  • Result: ❌ FAILED
  • Error: Error while executing command: ninja -v -C build_output -j ${NPROC} all packages strip_targets. Reason: exit status 1
  • Build Log terminal output (available for 30 days)
  • Build Workspace zip file of the working directory (available for 30 days)

@foundationdb-ci
Copy link
Contributor

Result of foundationdb-pr-macos on macOS Ventura 13.x

  • Commit ID: 1e768c3
  • Duration 1:01:18
  • Result: ❌ FAILED
  • Error: Error while executing command: ssh -o StrictHostKeyChecking=no -o UserKnownHostsFile=/dev/null -i ${HOME}/.ssh_key ec2-user@${MAC_EC2_HOST} /usr/local/bin/bash --login -c ./build_pr_macos.sh. Reason: exit status 1
  • Build Log terminal output (available for 30 days)
  • Build Workspace zip file of the working directory (available for 30 days)

@gm42
Go: introduce context for Transact/ReadTransact
b68c700 
Use a goroutine to automatically cancel transaction when context has an error.
Correctly close transactions by returning a function to caller.
@gm42
Copy link
Collaborator Author

gm42 commented Apr 1, 2025

@johscheuer I have fixed the build errors in the directory layer; please trigger CI again

@gm42 gm42 requested a review from johscheuer April 1, 2025 16:35
@jzhou77 jzhou77 closed this Apr 2, 2025
@jzhou77 jzhou77 reopened this Apr 2, 2025
@foundationdb-ci
Copy link
Contributor

Result of foundationdb-pr-clang-ide on Linux CentOS 7

  • Commit ID: b68c700
  • Duration 0:22:28
  • Result: ✅ SUCCEEDED
  • Error: N/A
  • Build Log terminal output (available for 30 days)
  • Build Workspace zip file of the working directory (available for 30 days)

@foundationdb-ci
Copy link
Contributor

Result of foundationdb-pr-macos-m1 on macOS Ventura 13.x

  • Commit ID: b68c700
  • Duration 0:40:40
  • Result: ✅ SUCCEEDED
  • Error: N/A
  • Build Log terminal output (available for 30 days)
  • Build Workspace zip file of the working directory (available for 30 days)

@foundationdb-ci
Copy link
Contributor

Result of foundationdb-pr-clang-arm on Linux CentOS 7

  • Commit ID: b68c700
  • Duration 0:48:30
  • Result: ✅ SUCCEEDED
  • Error: N/A
  • Build Log terminal output (available for 30 days)
  • Build Workspace zip file of the working directory (available for 30 days)

@foundationdb-ci
Copy link
Contributor

Result of foundationdb-pr-clang on Linux CentOS 7

  • Commit ID: b68c700
  • Duration 0:55:18
  • Result: ✅ SUCCEEDED
  • Error: N/A
  • Build Log terminal output (available for 30 days)
  • Build Workspace zip file of the working directory (available for 30 days)

@foundationdb-ci
Copy link
Contributor

Result of foundationdb-pr on Linux CentOS 7

  • Commit ID: b68c700
  • Duration 1:00:12
  • Result: ✅ SUCCEEDED
  • Error: N/A
  • Build Log terminal output (available for 30 days)
  • Build Workspace zip file of the working directory (available for 30 days)

@foundationdb-ci
Copy link
Contributor

Result of foundationdb-pr-cluster-tests on Linux CentOS 7

  • Commit ID: b68c700
  • Duration 1:01:10
  • Result: ✅ SUCCEEDED
  • Error: N/A
  • Build Log terminal output (available for 30 days)
  • Build Workspace zip file of the working directory (available for 30 days)
  • Cluster Test Logs zip file of the test logs (available for 30 days)

@foundationdb-ci
Copy link
Contributor

Result of foundationdb-pr-macos on macOS Ventura 13.x

  • Commit ID: b68c700
  • Duration 1:06:03
  • Result: ✅ SUCCEEDED
  • Error: N/A
  • Build Log terminal output (available for 30 days)
  • Build Workspace zip file of the working directory (available for 30 days)

@gm42
Copy link
Collaborator Author

gm42 commented Apr 4, 2025

Thanks for triggering the CI @jzhou77.

I guess next step is to discuss the changes

@johscheuer johscheuer requested a review from vishesh April 4, 2025 15:43
johscheuer
johscheuer reviewed Apr 4, 2025
View reviewed changes
Copy link
Contributor

@johscheuer johscheuer left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks again for the PR, it's in my queue for next week!

@gm42
Copy link
Collaborator Author

gm42 commented Apr 4, 2025

Thanks again for the PR, it's in my queue for next week!

Thanks! It's a big change, looking forward to the discussion!

@gm42 gm42 requested a review from johscheuer April 10, 2025 09:24
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@vishesh vishesh Awaiting requested review from vishesh

@johscheuer johscheuer Awaiting requested review from johscheuer

At least 1 approving review is required to merge this pull request.

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@gm42 @foundationdb-ci @johscheuer @jzhou77