fatal error: found pointer to free object #2
Comments
|
(moving discussion here @bradfitz @josharian) Josh, I'm not sure I follow the sequence of events in your hypothesis that lead to a dangling pointer. Get and the related finalizer are interlocked by mu. If the finalizer runs first, Get will miss in the cache table and therefore create a new pointer. If Get runs first, the finalizer will have the wrong gen version and turn into a no-op. In both cases, it seems to me that we cannot reach a point in execution where we materialize a pointer from a uintptr that has already ceased to exist - the process of making the pointer cease to exist should be destroying the uintptr. What sequence of operations am I missing that the runtime is allowed to do? |
|
One piece of knowledge I have holds the above together, and I realize I may be wrong about that: is a finalizer allowed to create new references to the object being finalized? That is, does the runtime have to correctly account for "I was about to delete this object, but the finalizer made it exist again" ? Or is it allowed to assume that, once the finalizer returns, the object is collectable? |
|
Just saw this issue too. I'd added this test locally too, like yours above, but the allocs make it trigger quicker: func TestStress(t *testing.T) {
iters := 1000000
if testing.Short() {
iters = 1000
}
var sink []byte
for i := 0; i < iters; i++ {
_ = Get("foo")
sink = make([]byte, 1<<20)
}
_ = sink
} |
|
Sent #4 that no longer changes a finalizer. It now only choose to re-add a finalizer in the finalizer. https://golang.org/pkg/runtime/#SetFinalizer says:
(which makes it very clear that it's okay to re-add a finalizer in the finalizer) |
|
I have questions, because I'm still not understanding where the problem was in the first place. The mutex should have made everything completely consistent, no? |
|
Specifically: I'm worried that I don't understand why the previous code was incorrect, and as a result don't know if this is fixed merely by lowering the probability of triggering in this specific test case. |
|
It seems like our "changing the finalizer" by clearing it & re-setting it was bogus somehow. Perhaps the intermediate In any case, PR #4 follows the documented rules super explicitly rather than wandering off into any unknown areas where we have to guess how the runtime might be implemented. |
|
Oooh, of course, clearing the finalizer lets a concurrent GC do bad things. So, for sequence of events, something like:
IOW, clearing a finalizer from an object gives the runtime immediate leave to delete that object without further ceremony. Normally, the reference that was being held to pass to SetFinalizer would prevent that, but the reference we're using was freshly conjured out of thin air and it may be one more GC cycle until the runtime is aware of its existence, for keeping-alive purposes. Sound right? The gap in my knowledge is where precisely finalizers get considered and scheduled in the GC cycle. I'm assuming there's a gap between "okay, this object needs collecting" and "time to collect, or is there a finalizer attached?" |
|
Yup, that all sounds right. As for when they're scheduled, the runtime docs also say:
|
|
That piece of runtime docs doesn't exactly answer my question, which is: how far apart in time are the mark phases and the check to see if a finalizer exists? That's the danger window in which we hold a pointer that the GC is unaware of, and so the finalizer is the only thing preventing the object from being GC'd. Empirically the answer seems to be "whatever, it's fine", because the new code unconditionally runs the finalizer at least once, ensuring that the Value will survive until the next GC cycle - at which point the newly materialized Value pointer will be seen in mark. It's notionally equivalent to the finalizer creating a new permanent reference to the object (e.g. in a global map) and thus preventing its collection, which I would hope the runtime handles gracefully. |
This package has a bug of some kind that causes crashes in the Go runtime under heavy use.
To reproduce, add the following to intern_test.go:
Under
go test(after a variable amount of time/attempts), this will crash with something like:The text was updated successfully, but these errors were encountered: