Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

runtime: defer runtime.KeepAlive bug #21402

Closed
bramp opened this issue Aug 11, 2017 · 7 comments

Comments

Projects
None yet
6 participants
@bramp
Copy link

commented Aug 11, 2017

What version of Go are you using (go version)?

go version go1.8.3 darwin/amd64
go1.8 (go playground)

What operating system and processor architecture are you using (go env)?

GOARCH="amd64"
GOBIN=""
GOEXE=""
GOHOSTARCH="amd64"
GOHOSTOS="darwin"
GOOS="darwin"
GOPATH="/Users/bramp/go"
GORACE=""
GOROOT="/Users/bramp/homebrew/Cellar/go/1.8.3/libexec"
GOTOOLDIR="/Users/bramp/homebrew/Cellar/go/1.8.3/libexec/pkg/tool/darwin_amd64"
GCCGO="gccgo"
CC="/usr/bin/clang"
GOGCCFLAGS="-fPIC -m64 -pthread -fno-caret-diagnostics -Qunused-arguments -fmessage-length=0 -fdebug-prefix-map=/var/folders/vw/07nbdcm16wn7mnzd252c44s0008gng/T/go-build574481541=/tmp/go-build -gno-record-gcc-switches -fno-common"
CXX="/usr/bin/clang++"
CGO_ENABLED="1"
PKG_CONFIG="pkg-config"
CGO_CFLAGS="-g -O2"
CGO_CPPFLAGS=""
CGO_CXXFLAGS="-g -O2"
CGO_FFLAGS="-g -O2"
CGO_LDFLAGS="-g -O2"

What did you do?

package main

import (
	"fmt"
	"runtime"
	"time"
)

type Test struct {
	data string
}

func main() {
	fmt.Println("A")

	o := &Test{"some data"}
	runtime.SetFinalizer(o, func(o *Test) {
		fmt.Printf("Finalized %p\n", o)
	})
	
	defer runtime.KeepAlive(o)
	
	runtime.GC()
	time.Sleep(1 * time.Second)

	fmt.Println("B")
}

https://play.golang.org/p/165ZsI_FUf

What did you expect to see?

I expected the defer runtime.KeepAlive to keep o reachable until the end of the main function, and to be finalised afterwards.

A
B
Finalized 0x1040c118

What did you see instead?

However, o was finalised before the main function ended, and before the defer statement was called.

A
Finalized 0x1040c118
B
@ALTree

This comment has been minimized.

Copy link
Member

commented Aug 11, 2017

Note that (from the spec)

deferred functions are invoked immediately before the surrounding function returns

so runtime.KeepAlive(o) is not called before the runtime.GC() call, but after when it's "too late".*

*edited

@cespare

This comment has been minimized.

Copy link
Contributor

commented Aug 11, 2017

@ALTree Isn't that the idea? You want to call runtime.KeepAlive at the last point where you want the object to be live.

@ALTree

This comment has been minimized.

Copy link
Member

commented Aug 11, 2017

at the last point

@cespare I see your point. But I wonder if a defer argument qualifies as a "physical point" in the code or at that point it's already "too late". I don't know if this makes sense.

EDIT: nevermind, I just noticed it happens without defer too.

@ALTree ALTree changed the title defer runtime.KeepAlive bug runtime: defer runtime.KeepAlive bug Aug 11, 2017

@randall77 randall77 self-assigned this Aug 11, 2017

@randall77

This comment has been minimized.

Copy link
Contributor

commented Aug 11, 2017

I think I see what is going on here. Replace defer runtime.KeepAlive(o) with defer f(o) with various fs.

func f1(x *Test) {}
func f2(x *Test) { _ = x }
func f3(x *Test) { _ = x.data }

The finalizer runs early with f1 and f2, but not f3.
It all comes down to how the garbage collector scans a defer. It basically uses the live map for the entry point for the deferred function. In f1 and f2 (and runtime.KeepAlive), the input argument is dead at entry, so the input argument is not live, so the input can get GCd early. In f3, we use x in a nil pointer check, so it is live at entry, so x gets retained.

This all looks correct for these fs. We just need to introduce enough use of its argument in runtime.KeepAlive to convince the compiler that its argument is live at entry.

@randall77 randall77 added this to the Go1.10 milestone Aug 11, 2017

@randall77 randall77 added the NeedsFix label Aug 11, 2017

@gopherbot

This comment has been minimized.

Copy link

commented Aug 11, 2017

Change https://golang.org/cl/55150 mentions this issue: runtime: add a use of runtime.KeepAlive's argument

@randall77

This comment has been minimized.

Copy link
Contributor

commented Aug 11, 2017

I don't think this should be a release blocker.
The bug is also in 1.7 and 1.8, and there is an easy workaround.

@ianlancetaylor

This comment has been minimized.

Copy link
Contributor

commented Aug 11, 2017

I'm marking it a release-blocker for 1.10.

@gopherbot gopherbot closed this in 2c990f4 Aug 14, 2017

@golang golang locked and limited conversation to collaborators Aug 14, 2018

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
You can’t perform that action at this time.