garbage collector found invalid heap pointer: Go 1.4 #163
Comments
|
The newer releases of Go (at least that's where I've noticed it) can make the GC free memory from a variable in the middle of a function if the variable isn't in use in the Go code anymore. It's possible that there are some missing defensive copies. |
|
It looks like there's a timing aspect to this. Removing the prints lets the program run without crashing. |
|
I'm getting same panic, EVEN WITH empty callback, without any prints or anything else. Like this: But my tracebacks are always pointing to this: https://github.com/libgit2/git2go/blob/master/tree.go#L109 |
No it's not timing, it's actual problem with git2go code. In 1.4 invalid (contains int for example) go pointers will cause panic during GC. What we are doing here is instructing Go to treat pointer from C (with int) as it was Go's pointer (unsafe.Pointer) which is wrong. We was able get away with this in 1.3 series, but 1.4 is actually have this fixed. This is by design. If we want to operate foreign pointers uintptr should be used. Here golang/go#9191 (comment) is explained better. |
|
I got the same issue like your comments. my go version is "go version go1.4.1 linux/amd64" Thanks in advance, it is urgent!!!! |
|
@b35li use go 1.3.3, it's OK there. |
|
@kron4eg I am confused if the problem is with git2go, why don't we fix in git2go, while we downgrad the go version to suit it? |
|
I think the problem is how git2go handle C pointers. Maybe I'm wrong, but I don't have time to fix it myself :) So I just downgraded. |
|
@kron4eg if we are using ints as pointers, that's definitely a bug, but I don't know which code you're saying that is doing this. |
|
@carlosmn I can't say exactly (in other case I'd send PR with fix), but looks like an int pointer returned from libgit2 itself at some point. And Go1.4 runtime see it and crash. |
|
I am using 1.4.2 for Tree.Walk testing and it always runtime panic for "nil pointer reference" error. func CallbackGitTreeWalk(_root *C.char, _entry *C.git_tree_entry, ptr unsafe.Pointer) C.int {
root := C.GoString(_root)
entry := _entry
callback := *(*TreeWalkCallback)(ptr)
fmt.Println("callback func %v", callback)
return C.int(callback(root, newTreeEntry(entry)))
} |
|
I think the problem is that GC has freed the pointer of callback since it doesn't known that it is referenced by C code. So to prevent GC on the pointer of callback, I've add it into a map and delete it after C._go_git_treewalk call finished. The changed code on tree.go is here: func (t Tree) Walk(callback TreeWalkCallback) error {
runtime.LockOSThread()
defer runtime.UnlockOSThread()
cbmap := make(map[unsafe.Pointer]interface{}, 1)
ptr := unsafe.Pointer(&callback)
cbmap[ptr] = callback
defer func() { delete(cbmap, ptr) }()
err := C._go_git_treewalk(
t.cast_ptr,
C.GIT_TREEWALK_PRE,
ptr,
)
if err < 0 {
return MakeGitError(err)
}
return nil
}@carlosmn Do you have any comments on this? |
|
By the way, to reproduce the runtime panic on tree_walk, you should walk a larger git repository to make GC happened during walking. Maybe to reduce the percentage of GC by debug.SetGCPercent() is another way. |
|
It's easy to reproduce the error when walking a rather big tree when starting off the GC via a goroutine. The following example crashes reliably for me on the Linux kernel repository: package main
import (
"fmt"
git "github.com/libgit2/git2go"
"os"
"runtime"
"time"
)
func main() {
path := os.Args[1]
repo, _ := git.OpenRepository(path)
ref, _ := repo.Head()
commit, _ := repo.LookupCommit(ref.Target())
tree, _ := commit.Tree()
go func() {
time.Sleep(100 * time.Millisecond)
runtime.GC()
}()
if err := tree.Walk(func(dir string, entry *git.TreeEntry) int {
fmt.Println(entry.Name)
return 0
}); err != nil {
panic(err)
}
} |
|
And yes, the error does not occur anymore after the workaround by @shinningstar is applied. Unfortunately it seems as if we don't have any way of doing what we want to do that is completly reliable, as the GC of Go is allowed to copy stacks around, thus causing pointers to become invalid. So if we hand over a Go pointer to a C function and the GC kicks in, causing pointers to become invalid, we're screwed up. As of Go 1.4 handing over Go pointers is not supported (see golang/go#8310). So I guess the only thing we can do atm is to use the proposed workaround, as it at least seems to make the issue go away, even though it might not prove reliable. |
|
If the runtime is going to arbitrarily make all of our pointers invalid, we'll have to write an indirection layer, as we keep our pointers long-term. Whether this is due to the stack moving or overeager GC, we will need a more comprehensive method for converting pointers between libgit2 and the Go code. |
|
I'm currently unable to trigger this error, even when running against the linux repo. I have however created a branch |
|
@carlosmn You could see issue#10303 in golang, maybe it could help. |
|
@carlosmn: I've modified the code to use your pointer indirection branch (see pks-t/git2go/pointer-indirection. Seems to work after fixing a bug in the NewHandleList function, at least my code snippet posted above does not error out anymore. If this is the way we want to go I'll also convert other functions that use Go callbacks in C code. |
|
@pks-t @carlosmn How about using the empty(NOP) function to escape callback to heap, just like go runtime team using? It seems that cgo team will use this method in Go1.5 to escape Go world pointer when passing to C world. //go:noescape
func use(unsafe.Pointer) {}
func (t Tree) Walk(callback TreeWalkCallback) error {
runtime.LockOSThread()
defer runtime.UnlockOSThread()
err := C._go_git_treewalk(
t.cast_ptr,
C.GIT_TREEWALK_PRE,
unsafe.Pointer(&callback),
)
use(unsafe.Pointer(&callback))
if err < 0 {
return MakeGitError(err)
}
return nil
} |
|
Well, for my part I wouldn't really want to rely on workarounds that are not officially documented. For now it is explicitly stated that no Go pointers should be passed into the C world, so I'd like to adhere to that and use the pointer indirection branch of @carlosmn insetad. |
|
@pks-t OK, the pointer indirection is fine. I could patch my project locally and wish it merged into main branch soon. |
|
@shinningstar I've already converted most of the code to use pointer indirection, see #196. |
After upgrading from go1.3 to go1.4, I'd get a runtime crash most of the time. It crashes for various reasons (nil tree walker callback, invalid heap pointer...). Here is a scaled down version of the code that demonstrates the problem (runtime crashes most of the time). Run this code with an argument pointing at a git repository like git2go.
Some errors that come from the above code:
The text was updated successfully, but these errors were encountered: