Go: [BREAKING] explicit destruction for futures and transactions #12052
Conversation
| return | ||
| } | ||
|
|
||
| kv = ri.kvs[ri.index] |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
I ended up switching from Advance()<
10000
/code>/GetSlice() to NextBatch()/Get()
| @@ -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. | |||
There was a problem hiding this comment.
Should we document what happens if the transaction is closed before all the associated futures have been closed?
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
I will make some attempts and submit a change; I am thinking to return a "closer" function that caller can defer.
There was a problem hiding this comment.
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.
20a73a5 to
1e768c3
Compare
Result of foundationdb-pr-clang-ide on Linux CentOS 7
|
Result of foundationdb-pr-clang on Linux CentOS 7
|
Result of foundationdb-pr-macos-m1 on macOS Ventura 13.x
|
Result of foundationdb-pr on Linux CentOS 7
|
Result of foundationdb-pr-cluster-tests on Linux CentOS 7
|
Result of foundationdb-pr-clang-arm on Linux CentOS 7
|
Result of foundationdb-pr-macos on macOS Ventura 13.x
|
|
@johscheuer I have fixed the build errors in the directory layer; please trigger CI again |
Result of foundationdb-pr-clang-ide on Linux CentOS 7
|
Result of foundationdb-pr-macos-m1 on macOS Ventura 13.x
|
Result of foundationdb-pr-clang-arm on Linux CentOS 7
|
Result of foundationdb-pr-clang on Linux CentOS 7
|
Result of foundationdb-pr on Linux CentOS 7
|
Result of foundationdb-pr-cluster-tests on Linux CentOS 7
|
Result of foundationdb-pr-macos on macOS Ventura 13.x
|
|
Thanks for triggering the CI @jzhou77. I guess next step is to discuss the changes |
Yes, I reached the same conclusion last week.
thanks for the feedback @Semisol!
This seems doable, I will try to add it to this PR.
Not sure about this one; will try to find out whether it's the only option or not, as I change more code.
I think you mean Close and Cancel; however, if you look at the C API there are specific behaviors associated to them. Will eventually remove
Yes, this is doable thanks to the callback. I will instrument it further.
I think the current pattern with Will submit some changes later as I rewrite part of the current ones and add |
I just submitted the first part of this rewrite:
What's left to be done:
|
3f302af to
5d8b534
Compare
|
I've completed my changes; a couple methods have been removed from the By the way, the C FDB API implementation does indeed treat destroy and cancel as almost the same: extern "C" DLLEXPORT void fdb_future_cancel(FDBFuture* f) {
CATCH_AND_DIE(TSAVB(f)->addref(); TSAVB(f)->cancel(););
}
extern "C" DLLEXPORT void fdb_future_destroy(FDBFuture* f) {
CATC
F438
H_AND_DIE(TSAVB(f)->cancel(););
} |
|
If we want to further simplify this:
Pros:
Cons:
I do not like this direction because it would be wasteful by design; if we really want to pursue it, it would be best to do it only for the scalar types (not the KV or K arrays) |
|
I have done some of my own changes here, with some opiniation: https://github.com/semisol/foundationdb I'll look into the changes here and get back with feedback soon. (I have removed contexts from |
I noticed that, but rather than an even bigger change I opted for adding context without pulling that apart. There is no performance/correctness concern other than it does not look elegant. |
I checked your branch; aside from undoing cancellation and context I think it introduces transaction leaks (with no finalizer and no closer, the transaction will not be closed). I saw you wanted to counteract that by closing the transaction in the Looking forward to your feedback! |
|
Let me write what is (at least for me) an interesting problem regarding closing transactions: there are basically two use cases to support:
In (2), closing the transaction causes all futures to be cancelled, thus it can only be done after user is done using the futures. Note: the lifecycle is similar for both transactions that will be committed and transactions that will not be committed: all transactions must be destroyed, even those which are not committed, the only variation is the commit happening before destruction. When doing read-only operations, transactions are always destroyed but never committed. |
This should be avoided in most cases, as you cannot retry once you are outside of the transaction, except with application logic. At that point, the user should use the I am not against supporting such an API though, but I believe it should be distinct. |
It is not correct IMO because closing a transaction implicitly cancels all associated futures, which can be used after transaction commit.
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. |
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.
Result of foundationdb-pr-clang-ide on Linux RHEL 9
|
Result of foundationdb-pr-macos-m1 on macOS Ventura 13.x
|
Result of foundationdb-pr-clang on Linux RHEL 9
|
Result of foundationdb-pr-cluster-tests on Linux RHEL 9
|
Result of foundationdb-pr on Linux RHEL 9
|
Result of foundationdb-pr-clang-arm on Linux CentOS 7
|
Result of foundationdb-pr-macos on macOS Ventura 13.x
|
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.Contextis 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
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:
release-branchormainif this is the youngest branch)