search

apple / foundationdb

FoundationDB - the open source, distributed, transactional key-value store
https://apple.github.io/foundationdb/
Apache License 2.0
14.19k stars 1.29k forks source link

Go binding: do not use pointer receiver for Close() #11383

Closed gm42 closed 1 month ago

gm42 commented 1 month ago

A mix of pointer/non-pointer receivers makes it impossible to use interfaces for the fdb.Database type; I propose to change the Close() method for consistency.

See https://go.dev/play/p/PQ_FDXajpnB for an example of the problem (needs to be run locally):

# command-line-arguments
./main.go:18:22: cannot use fdb.Database{} (value of type fdb.Database) as FoundationDB value in variable declaration: fdb.Database does not implement FoundationDB (method Close has pointer receiver)

Custom interfaces can still be used by using a pointer instead of directly the fdb.Database type:

var _ FoundationDB = &fdb.Database{}

But that's less efficient.

NOTE: no test was updated

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.

For Release-Branches

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

gm42 commented 1 month ago

Cc @johscheuer

foundationdb-ci commented 1 month ago

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

foundationdb-ci commented 1 month ago

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

foundationdb-ci commented 1 month ago

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

foundationdb-ci commented 1 month ago

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

foundationdb-ci commented 1 month ago

Result of foundationdb-pr-clang on Linux CentOS 7

foundationdb-ci commented 1 month ago

Result of foundationdb-pr on Linux CentOS 7

gm42 commented 1 month ago

The error seems to be, while building a Docker image:

#23 ERROR: "/YCSB" not found: not found

Unrelated?

gm42 commented 1 month ago

Please hold merging this PR, I have found an unrelated issue in main (cannot call Close() because destruction happens already via finalizer) and I am looking into it.

gm42 commented 1 month ago

I have verified that calling fdb_database_destroy multiple times eventually crashes with a SIGSEGV, so what is written in the C API documentation is correct:

It must be called exactly once for each successful call to fdb_create_database().

However for unknown reasons it does not crash if it is called 1-2 extra times, while it certainly crashes if we do that in a loop:

--- a/bindings/go/src/fdb/fdb_test.go
+++ b/bindings/go/src/fdb/fdb_test.go
@@ -344,6 +344,11 @@ func TestDatabaseCloseRemovesResources(t *testing.T) {
        if db == newDB {
                t.Fatalf("Expected a different database object, got: %v and %v\n", db, newDB)
        }
+
+       for i := 1; i < 1000; i++ {
+               fmt.Printf("closed %d times so far, going to close again\n", i)
+               db.Close()
+       }
 }

 func ExampleOpenWithConnectionString() {

The above change will crash:

closed 1 times so far, going to close again
closed 2 times so far, going to close again
SIGSEGV: segmentation violation
PC=0x764c33e5583d m=0 sigcode=128 addr=0x0
signal arrived during cgo execution

goroutine 6 gp=0xc000007dc0 m=0 mp=0x675580 [syscall]:
runtime.cgocall(0x51b860, 0xc00006ddc8)
    /usr/local/go/src/runtime/cgocall.go:157 +0x4b fp=0xc00006dda0 sp=0xc00006dd68 pc=0x40844b
github.com/apple/foundationdb/bindings/go/src/fdb._Cfunc_fdb_database_destroy(0x1b2d650)
    _cgo_gotypes.go:197 +0x3f fp=0xc00006ddc8 sp=0xc00006dda0 pc=0x50fedf
github.com/apple/foundationdb/bindings/go/src/fdb.(*database).destroy.func1(0x58fae0?)
    /home/user/source/foundationdb/bindings/go/src/fdb/database.go:89 +0x3e fp=0xc00006de08 sp=0xc00006ddc8 pc=0x5119de
github.com/apple/foundationdb/bindings/go/src/fdb.(*database).destroy(0xc000014448)
    /home/user/source/foundationdb/bindings/go/src/fdb/database.go:89 +0x7c fp=0xc00006de80 sp=0xc00006de08 pc=0x51191c
github.com/apple/foundationdb/bindings/go/src/fdb.(*Database).Close(...)
    /home/user/source/foundationdb/bindings/go/src/fdb/database.go:73
github.com/apple/foundationdb/bindings/go/src/fdb_test.TestDatabaseCloseRemovesResources(0xc0001224e0)
    /home/user/source/foundationdb/bindings/go/src/fdb/fdb_test.go:350 +0x2a9 fp=0xc00006df70 sp=0xc00006de80 pc=0x519f29
testing.tRunner(0xc0001224e0, 0x566970)
    /usr/local/go/src/testing/testing.go:1689 +0xfb fp=0xc00006dfc0 sp=0xc00006df70 pc=0x4d0fbb
testing.(*T).Run.gowrap1()
    /usr/local/go/src/testing/testing.go:1742 +0x25 fp=0xc00006dfe0 sp=0xc00006dfc0 pc=0x4d1fe5
runtime.goexit({})
    /usr/local/go/src/runtime/asm_amd64.s:1695 +0x1 fp=0xc00006dfe8 sp=0xc00006dfe0 pc=0x473f01
created by testing.(*T).Run in goroutine 1
    /usr/local/go/src/testing/testing.go:1742 +0x390

goroutine 1 gp=0xc0000061c0 m=nil [chan receive]:
runtime.gopark(0xc000108820?, 0xc0001088a8?, 0x20?, 0x88?, 0x539c00?)
    /usr/local/go/src/runtime/proc.go:402 +0xce fp=0xc0001359c0 sp=0xc0001359a0 pc=0x43face
runtime.chanrecv(0xc00001e230, 0xc000135aa7, 0x1)
    /usr/local/go/src/runtime/chan.go:583 +0x3bf fp=0xc000135a38 sp=0xc0001359c0 pc=0x40aa3f
runtime.chanrecv1(0x674900?, 0x52c680?)
    /usr/local/go/src/runtime/chan.go:442 +0x12 fp=0xc000135a60 sp=0xc000135a38 pc=0x40a672
testing.(*T).Run(0xc000122340, {0x560e86?, 0x0?}, 0x566970)
    /usr/local/go/src/testing/testing.go:1750 +0x3ab fp=0xc000135b20 sp=0xc000135a60 pc=0x4d1e8b
testing.runTests.func1(0xc000122340)
    /usr/local/go/src/testing/testing.go:2161 +0x37 fp=0xc000135b60 sp=0xc000135b20 pc=0x4d3f77
testing.tRunner(0xc000122340, 0xc000135c70)
    /usr/local/go/src/testing/testing.go:1689 +0xfb fp=0xc000135bb0 sp=0xc000135b60 pc=0x4d0fbb
testing.runTests(0xc0000120c0, {0x65d220, 0x9, 0x9}, {0x1?, 0x4b97ce?, 0x674ae0?})
    /usr/local/go/src/testing/testing.go:2159 +0x445 fp=0xc000135ca0 sp=0xc000135bb0 pc=0x4d3e65
testing.(*M).Run(0xc00010a0a0)
    /usr/local/go/src/testing/testing.go:2027 +0x68b fp=0xc000135ed0 sp=0xc000135ca0 pc=0x4d286b
main.main()
    _testmain.go:79 +0x16c fp=0xc000135f50 sp=0xc000135ed0 pc=0x51b74c
runtime.main()
    /usr/local/go/src/runtime/proc.go:271 +0x29d fp=0xc000135fe0 sp=0xc000135f50 pc=0x43f67d
runtime.goexit({})
    /usr/local/go/src/runtime/asm_amd64.s:1695 +0x1 fp=0xc000135fe8 sp=0xc000135fe0 pc=0x473f01

The reason I am looking into this is because I experience, randomly, a crash when using the Close() method and I believe it is caused by this Close() method being always called during GC.

@johscheuer no blocker to merge this PR but I might want to submit another one to fix this issue e.g. GC's call to destroy() and user-initiated Close() should not cause SIGSEGVs. I believe it can be done using an atomic value, or by removing the finalizer and asking user to always call Close() explicitly.

I think the latter is the more idiomatic approach.

gm42 commented 1 month ago

Created PR here: https://github.com/apple/foundationdb/pull/11394