bindings/go: use keepalive(f) on futures when they're used #1451
Conversation
bindings/go: keep futures alive on use
|
@fdb-build, ok to test |
|
@alexmiller-apple any idea what failed here? |
|
I'm not sure which version of Go we're using to build, but it doesn't recognize |
|
Oh, I never actually posted. We're running go 1.3 in our CI, and runtime.KeepAlive was introduced in 1.7. Tangentially, @AlvinMooreSr is working on getting the image used in the CI updated, so this PR will be stranded for sad reasons for a little while until that happens, but that should hopefully happen sometime soon. |
|
I think Alvin mentioned our images got changed and upgraded, so let's try again. @fdb-build, test this please |
|
Was the go version updated in the build image? |
|
In any case, this change would require also bumping our minimum required go version, which isn't the end of the world, but it would be important to note. |
|
The version of go in the new build image is "go version go1.11.5 linux/amd64". This PR also successfully compiles all go targets via cmake within the new docker image: foundationdb/foundationdb-build:0.1.2 which is already published to docker.com and awaiting integration into our current master branch. |
|
test this please |
alecgrieser
left a comment
alecgrieser
left a comment
There was a problem hiding this comment.
Okay, well, this seems reasonable enough to me, though I guess I'm somewhat confused of the mechanism. Is the specific thing that the future was somehow being dereferenced in the C specific stuff? I'm not super familiar with the low-level particulars of go like that.
It also looks like the go community doesn't support anything older than 1.11 if I'm reading this page correctly: https://golang.org/doc/devel/release.html#policy So, I'm fine with bumping the required go version, though we need to update our documentation with this change: https://github.com/apple/foundationdb/blob/master/documentation/sphinx/source/downloads.rst#go-11
|
It seems the basic idea is that from the perspective of Go, there are cases where we were done using a Go level future object while we still had an outstanding native call using the pointer that the future object held. Go would destruct the future object, triggering the finalizer which decremented the reference count of the object at the native level. It must be the case that were deleting the last reference count (either because the operations didn't hold their own reference or due to a race, not sure), and then that left us accessing a deleted object. The I think we can go ahead and merge this and update the documentation afterward. |
5 participants
Hey,
While investigating a panic in the bindings we found that futures do not use
runtime.KeepAlive(), this can cause the future to be destroyed before it is used.See an example here: https://play.golang.org/p/Lv19twqhDEU
We patched segmentio/foundationdb with this fix, deployed and we no longer see any panics in production.
Worth nothing that we found that other structures that use
SetFinalizer()do not useruntime.KeepAlive()as well, but we opted to make minimal changes to the*Futurethat shows up in the trace.