Go binding: add StopNetwork() #11393

gm42 · 2024-05-14T12:59:54Z

This PR contains the following notable changes:

make sure that database creation is happening only while network thread is running
panic if the call to fdb_run_network() fails, instead of logging an error only
StartNetwork(), already deprecated, effectively becomes a no-op
expose a new StopNetwork() method which allows to follow spec for issues like Consider making shutting down without joining the network thread safe in the client #2978 and Not all binding implementations follow the requirements of fdb_stop_network #3015

Any advice for test coverage?

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.

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.

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 · 2024-05-14T13:04:37Z

Cc @vishesh @ajbeamon, as I saw you were involved in discussing #3015

foundationdb-ci · 2024-05-14T17:25:52Z

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

Commit ID: 0f04b8d
Duration 0:48:54
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 · 2024-05-14T17:41:46Z

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

Commit ID: 0f04b8d
Duration 1:04:48
Result: ❌ FAILED
Error: Error while executing command: docker build --label "org.foundationdb.version=${FDB_VERSION}" --label "org.foundationdb.build_date=${BUILD_DATE}" --label "org.foundationdb.commit=${COMMIT_SHA}" --progress plain --build-arg FDB_VERSION="${FDB_VERSION}" --build-arg FDB_LIBRARY_VERSIONS="${FDB_VERSION}" --build-arg FDB_WEBSITE="${FDB_WEBSITE}" --tag foundationdb/ycsb:${FDB_VERSION}-${COMMIT_SHA}-debug --file Dockerfile.eks --target ycsb .. 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 · 2024-05-14T17:58:04Z

Result of foundationdb-pr-clang on Linux CentOS 7

Commit ID: 0f04b8d
Duration 1:21:06
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 · 2024-05-14T18:01:27Z

Result of foundationdb-pr on Linux CentOS 7

Commit ID: 0f04b8d
Duration 1:24:27
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)

bindings/go/src/fdb/fdb.go

johscheuer · 2024-05-27T15:44:34Z

bindings/go/src/fdb/fdb.go

-func startNetwork() error {
+// ensureNetworkIsStarted ensures that the API version is set and then starts the internal network event loop, if not already done.
+// The provided function is guaranteed to be run while network thread is running.
+func ensureNetworkIsStarted(f func()) error {


Suggested change

func ensureNetworkIsStarted(f func()) error {

func executeWithRunningNetworkThread(f func()) error {

I don't want to be nitpicking about the method name but executeWithRunningNetworkThread might be a better name for what this method is doing now.

Should we add a check in this method if the network was stopped? And if so return an according error? Right now we would still execute f() which would probably result in a panic?

Sure; made some changes, please review?

Note: I could not use a FoundationDB error code for these, as they do not exist.

bindings/go/src/fdb/fdb.go

johscheuer · 2024-05-27T15:51:05Z

bindings/go/src/fdb/fdb.go

+// This function does nothing if the network has not yet started.
+// Once network is stopped it cannot be started again.
+// See also: https://github.com/apple/foundationdb/issues/3015
+func StopNetwork() {


Suggested change

func StopNetwork() {

func StopNetwork() error {

We probably should return an error if the network was started and was already stopped? I'm not sure what will happen, but a user-friendly error like "network was already stopped" might be better than having the code panic or return nothing.

Sure, but what about the case of network not yet started? Should it return an error or do nothing?

I made it return an error also in such case.

gm42 · 2024-05-28T22:29:50Z

bindings/go/src/fdb/errors.go

-	errNetworkNotSetup = Error{2008}
+	errNetworkNotSetup          = Error{2008}
+	errNetworkAlreadySetup      = Error{2009}
+	errNetworkCannotBeRestarted = Error{2025}


These are really not used in this PR; I could remove them, I thought to add them and leave them here for completeness.

…d is running All API calls which create a database are guaranteed to happen while the network thread is running; there is no performance penalty for calls happening after network thread has already been started. Notable change: if the call to run network thread fails, a panic will be generated instead of a logged error.

Add a method which allows to a safe explicit stop of the network thread. This method waits for the network thread to exit before returning to caller.

Specify which functions are safe to be called from multiple goroutines

johscheuer

One comment, otherwise LGTM 👍

johscheuer · 2024-06-06T13:54:28Z

bindings/go/src/fdb/fdb.go

+	if networkStopped {
+		networkMutex.RUnlock()
+
+		return ErrNetworkIsStopped


Suggested change

if networkStopped {

networkMutex.RUnlock()

return ErrNetworkIsStopped

if networkStopped {

return ErrNetworkIsStopped

In this case we have a write mutex and the defer will already unlock it. Calling it again will result in a runtime error.

gm42 mentioned this pull request May 14, 2024

Not all binding implementations follow the requirements of fdb_stop_network #3015

Open

jzhou77 closed this May 14, 2024

jzhou77 reopened this May 14, 2024

gm42 force-pushed the fix/add-stop-network branch from 0f04b8d to 3565780 Compare May 15, 2024 05:54

johscheuer reviewed May 27, 2024

View reviewed changes

gm42 force-pushed the fix/add-stop-network branch from 4561886 to 970c54c Compare May 27, 2024 17:55

gm42 requested a review from johscheuer May 28, 2024 22:29

gm42 commented May 28, 2024

View reviewed changes

gm42 added 3 commits June 6, 2024 12:12

Go: add StopNetwork method

fd7b68b

Add a method which allows to a safe explicit stop of the network thread. This method waits for the network thread to exit before returning to caller.

Go: return an error when calling StopNetwork() multiple times

a89df68

Specify which functions are safe to be called from multiple goroutines

gm42 force-pushed the fix/add-stop-network branch from 970c54c to a89df68 Compare June 6, 2024 10:14

johscheuer approved these changes Jun 6, 2024

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Go binding: add StopNetwork() #11393

Go binding: add StopNetwork() #11393

gm42 commented May 14, 2024

gm42 commented May 14, 2024

foundationdb-ci commented May 14, 2024

foundationdb-ci commented May 14, 2024

foundationdb-ci commented May 14, 2024

foundationdb-ci commented May 14, 2024

johscheuer May 27, 2024

gm42 May 27, 2024

gm42 May 27, 2024

johscheuer May 27, 2024

gm42 May 27, 2024

gm42 May 27, 2024

gm42 May 28, 2024

johscheuer left a comment

johscheuer Jun 6, 2024

	func ensureNetworkIsStarted(f func()) error {
	func executeWithRunningNetworkThread(f func()) error {

Go binding: add StopNetwork() #11393

Are you sure you want to change the base?

Go binding: add StopNetwork() #11393

Conversation

gm42 commented May 14, 2024

Code-Reviewer Section

For Release-Branches

gm42 commented May 14, 2024

foundationdb-ci commented May 14, 2024

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

foundationdb-ci commented May 14, 2024

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

foundationdb-ci commented May 14, 2024

Result of foundationdb-pr-clang on Linux CentOS 7

foundationdb-ci commented May 14, 2024

Result of foundationdb-pr on Linux CentOS 7

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

johscheuer left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment