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

gm42 · 2025-03-26T10:02:19Z

Explicit destruction for futures and transactions.
Introduction of context for transactions and futures.
Simplification of Future interface and internal value caching

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 · 2025-03-26T12:26:41Z

bindings/go/src/fdb/range.go

-		return
-	}
-
-	kv = ri.kvs[ri.index]


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.

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

johscheuer

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/directory/directory_layer.go

bindings/go/src/fdb/snapshot.go

johscheuer · 2025-03-26T14:47:54Z

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.


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

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.

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

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.

foundationdb-ci · 2025-04-01T11:55:48Z

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 · 2025-04-01T12:05:17Z

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 · 2025-04-01T12:09:23Z

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 · 2025-04-01T12:13:04Z

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 · 2025-04-01T12:13:34Z

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 · 2025-04-01T12:18:41Z

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 · 2025-04-01T12:34:50Z

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 · 2025-04-01T16:35:03Z

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

foundationdb-ci · 2025-04-02T05:10:59Z

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 · 2025-04-02T05:29:11Z

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 · 2025-04-02T05:37:02Z

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 · 2025-04-02T05:43:48Z

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 · 2025-04-02T05:48:46Z

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 · 2025-04-02T05:49:41Z

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 · 2025-04-02T05:54:32Z

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 · 2025-04-04T07:23:48Z

Thanks for triggering the CI @jzhou77.

I guess next step is to discuss the changes

foundationdb-ci · 2025-05-15T08:52:06Z

Result of foundationdb-pr-cluster-tests on Linux RHEL 9

Commit ID: e242faa
Duration 0:41:17
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 · 2025-05-15T08:53:12Z

Result of foundationdb-pr on Linux RHEL 9

Commit ID: e242faa
Duration 0:42:25
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 · 2025-05-15T08:55:21Z

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

Commit ID: e242faa
Duration 0:44:36
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 · 2025-05-15T09:05:08Z

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

Commit ID: e242faa
Duration 0:54:24
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 · 2025-05-19T16:36:48Z

The failure seems to be:

clang: warning: argument unused during compilation: '-L/codebuild/output/src516627942/src/github.com/apple/foundationdb/build_output/fdbclient/awssdk-build/install/external-install/zlib/lib' [-Wunused-command-line-argument]
/codebuild/output/src516627942/src/github.com/apple/foundationdb/build_output/fdbclient/awssdk-build/curl/CMake/CurlTests.c:322:24: error: call to undeclared function 'ioctlsocket'; ISO C99 and later do not support implicit function declarations [-Wimplicit-function-declaration]
  322 |  unsigned long flags = ioctlsocket(socket, FIONBIO, &flags);
      |                        ^
/codebuild/output/src516627942/src/github.com/apple/foundationdb/build_output/fdbclient/awssdk-build/curl/CMake/CurlTests.c:322:44: error: use of undeclared identifier 'FIONBIO'
  322 |  unsigned long flags = ioctlsocket(socket, FIONBIO, &flags);
      |                                            ^
2 errors generated.
ninja: build stopped: subcommand failed.

Unrelated to this PR? I will rebase it just in case it helps.

Semisol · 2025-05-19T16:39:03Z

It is not correct IMO because closing a transaction implicitly cancels all associated futures, which can be used after transaction commit.

I believe that all effects of a transaction in Transact should remain in Transact. The user can use CreateTransaction for more advanced applications.

Futures are perfectly valid to be used after transaction has been committed; this is currently supported when using the C binding and when using the Go binding. There is no need to retry because resolving a future doesn't involve any future-specific retry.

What I meant was if after the TX has been committed, the future fails for some other reason, the app may want to retry it. The only futures I think that might matter is versionstamp.

foundationdb-ci · 2025-05-19T17:05:56Z

Result of foundationdb-pr-clang-ide on Linux RHEL 9

Commit ID: 64e4b53
Duration 0:28:07
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 · 2025-05-19T17:13:38Z

Result of foundationdb-pr-clang on Linux RHEL 9

Commit ID: 64e4b53
Duration 0:35:45
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 · 2025-05-19T17:19:21Z

Result of foundationdb-pr-cluster-tests on Linux RHEL 9

Commit ID: 64e4b53
Duration 0:41:32
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 · 2025-05-19T17:19:47Z

Result of foundationdb-pr on Linux RHEL 9

Commit ID: 64e4b53
Duration 0:41:56
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 · 2025-05-19T17:24:07Z

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

Commit ID: 64e4b53
Duration 0:46:20
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)

gm42 · 2025-05-26T13:28:39Z

@jzhou77 can you please help me figure out whether the CI failure is legit or a false positive?

sabey · 2025-05-30T00:54:59Z

Hey, if this is going to get merged in sometime could you fix snapshot options? Thanks

#12174

gm42 · 2025-05-30T09:59:03Z

Hey, if this is going to get merged in sometime could you fix snapshot options? Thanks

#12174

I can, but I think it would be best to make a separate PR for that.

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.

Other functions renamed: * GetSliceWithError() -> Get() * GetSliceOrPanic() -> MustGet()

Using a channel allows an easier integration with context.Context

Use a goroutine to automatically cancel transaction when context has an error. Correctly close transactions by returning a function to caller.

Futures will be cancelled in case of context cancellation Explicitly mention that BlockUntilReady is a reimplementation of fdb_future_block_until_ready

Only a few futures are currently being cached; this change make all futures be cached and adds the missing check for fdb_future_get_error in the previously cached futures.

Simplification for the usage of futures: * if Get() is called on a future, all further calls will return a cached value and the future will be automatically closed * if Get() is never called then future will be closed when user calls Close() Cancel() is no more necessary: user can call Close() Similarly, BlockUntilReady() is removed because a call to Get() (ignoring return value) is equivalent.

foundationdb-ci · 2025-05-30T10:25:52Z

Result of foundationdb-pr-clang-ide on Linux RHEL 9

Commit ID: 5cb1040
Duration 0:24:49
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 · 2025-05-30T10:34:59Z

Result of foundationdb-pr-clang on Linux RHEL 9

Commit ID: 5cb1040
Duration 0:33:53
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 · 2025-05-30T10:36:19Z

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

Commit ID: 5cb1040
Duration 0:35: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} /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 · 2025-05-30T10:42:11Z

Result of foundationdb-pr-cluster-tests on Linux RHEL 9

Commit ID: 5cb1040
Duration 0:41:06
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 · 2025-05-30T10:42:54Z

Result of foundationdb-pr on Linux RHEL 9

Commit ID: 5cb1040
Duration 0:41:53
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 · 2025-05-30T10:46:27Z

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

Commit ID: 5cb1040
Duration 0:45:24
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 · 2025-05-30T10:58:30Z

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

Commit ID: 5cb1040
Duration 0:57:28
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 requested a review from johscheuer March 26, 2025 10:02

gm42 mentioned this pull request Mar 26, 2025

Go (golang bindings): 8 database connections at once causes CGO signal and panic #11856

Open

gm42 commented Mar 26, 2025

View reviewed changes

johscheuer reviewed Mar 26, 2025

View reviewed changes

gm42 force-pushed the main branch 3 times, most recently from 20a73a5 to 1e768c3 Compare March 31, 2025 07:58

johscheuer closed this Apr 1, 2025

johscheuer reopened this Apr 1, 2025

gm42 force-pushed the main branch from 1e768c3 to b68c700 Compare April 1, 2025 16:34

gm42 requested a review from johscheuer April 1, 2025 16:35

jzhou77 closed this Apr 2, 2025

jzhou77 reopened this Apr 2, 2025

johscheuer requested a review from vishesh April 4, 2025 15:43

gm42 force-pushed the main branch from e242faa to 64e4b53 Compare May 19, 2025 16:37

gm42 added 7 commits May 30, 2025 12:00

Go: rename Advance() as NextBatch(), remove RangeIterator.Get()

0e8406d

Other functions renamed: * GetSliceWithError() -> Get() * GetSliceOrPanic() -> MustGet()

Go: use a channel instead of a Mutex to wait for future readiness

60f5add

Using a channel allows an easier integration with context.Context

Go: introduce context for Transact/ReadTransact

1eea6c7

Use a goroutine to automatically cancel transaction when context has an error. Correctly close transactions by returning a function to caller.

Go: add context to futures' Get()

3a6ccfd

Futures will be cancelled in case of context cancellation Explicitly mention that BlockUntilReady is a reimplementation of fdb_future_block_until_ready

Go: cache resolved futures for all types

8e54435

Only a few futures are currently being cached; this change make all futures be cached and adds the missing check for fdb_future_get_error in the previously cached futures.

gm42 force-pushed the main branch from 64e4b53 to 5cb1040 Compare May 30, 2025 10:00

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

Are you sure you want to change the base?

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

Uh oh!

Conversation

gm42 commented Mar 26, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Code-Reviewer Section

For Release-Branches

Uh oh!

gm42 Mar 26, 2025

Choose a reason for hiding this comment

Uh oh!

gm42 Apr 1, 2025

Choose a reason for hiding this comment

Uh oh!

johscheuer left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

johscheuer Mar 26, 2025

Choose a reason for hiding this comment

Uh oh!

gm42 Mar 27, 2025

Choose a reason for hiding this comment

Uh oh!

gm42 Mar 27, 2025

Choose a reason for hiding this comment

Uh oh!

gm42 Mar 27, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

foundationdb-ci commented Apr 1, 2025

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

Uh oh!

foundationdb-ci commented Apr 1, 2025

Result of foundationdb-pr-clang on Linux CentOS 7

Uh oh!

foundationdb-ci commented Apr 1, 2025

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

Uh oh!

foundationdb-ci commented Apr 1, 2025

Result of foundationdb-pr on Linux CentOS 7

Uh oh!

foundationdb-ci commented Apr 1, 2025

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

Uh oh!

foundationdb-ci commented Apr 1, 2025

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

Uh oh!

foundationdb-ci commented Apr 1, 2025

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

Uh oh!

gm42 commented Apr 1, 2025

Uh oh!

foundationdb-ci commented Apr 2, 2025

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

Uh oh!

foundationdb-ci commented Apr 2, 2025

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

Uh oh!

foundationdb-ci commented Apr 2, 2025

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

Uh oh!

foundationdb-ci commented Apr 2, 2025

Result of foundationdb-pr-clang on Linux CentOS 7

Uh oh!

foundationdb-ci commented Apr 2, 2025

Result of foundationdb-pr on Linux CentOS 7

Uh oh!

foundationdb-ci commented Apr 2, 2025

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

Uh oh!

foundationdb-ci commented Apr 2, 2025

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

Uh oh!

gm42 commented Apr 4, 2025

Uh oh!

gm42 commented Mar 26, 2025 •

edited

Loading

gm42 Mar 27, 2025 •

edited

Loading