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: update SetFinalizer docs to cover cases of package level data #17311

Closed
yaxinlx opened this issue Oct 1, 2016 · 16 comments

Comments

Projects
None yet
6 participants
@yaxinlx
Copy link

commented Oct 1, 2016

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

go1.7.1 linux/amd64

What did you see instead?

see this discussion for details: https://groups.google.com/forum/#!topic/golang-dev/JAgv8kBTgk4

summary:

  1. in the latest implementation, there is not restrictions for the argument obj.
  2. the docs should present that calling SetFinalizer for a package level data is meaningless.

@odeke-em odeke-em changed the title Make the SetFinalizer docs perfect. runtime: update SetFinalizer docs to cover cases of package level data Oct 1, 2016

@odeke-em

This comment has been minimized.

Copy link
Member

commented Oct 1, 2016

/cc some of the runtime folks @aclements @RLH.

@minux

This comment has been minimized.

Copy link
Member

commented Oct 1, 2016

The existing docs is fine as is.

"The argument obj must be a pointer to an object allocated by
calling new, by taking the address of a composite literal, or by
taking the address of a local variable."

This means no other pointers are accepted, which includes
package level variables.

We shouldn't document whether it checks the incoming obj
pointer or not, because that will limit future implementations.

@minux minux closed this Oct 1, 2016

@yaxinlx

This comment has been minimized.

Copy link
Author

commented Oct 2, 2016

@minux, "This means no other pointers are accepted, which includes package level variables."
This is not the fact. Package level variables are acceptable: https://play.golang.org/p/tH_3uwbgUw
The document is surely wrong.

If the document is intended to exclude package level variables, please also exclude package level pointers to objects allocated by calling new and by taking the address of composite literals.

@minux

This comment has been minimized.

Copy link
Member

commented Oct 2, 2016

@yaxinlx

This comment has been minimized.

Copy link
Author

commented Oct 2, 2016

But if the document doesn't forbid package level pointers to objects allocated by calling new and by taking the address of composite literals, why it forbids general package level pointers?

@yaxinlx

This comment has been minimized.

Copy link
Author

commented Oct 2, 2016

If the document has no problems, can I file an issue for the compiler implementation?
After all, older compiler versions will create a panic when argument obj is a pointer to package level data.

Whatever, the docs for SetFinalizer should be more detailed to address the differences between documented behavior and actual implemented behavior.

@minux

This comment has been minimized.

Copy link
Member

commented Oct 2, 2016

@yaxinlx

This comment has been minimized.

Copy link
Author

commented Oct 2, 2016

Good explanation, ok, I don't insist on changing the docs any more.
But I think putting a warning to using pointers to package level data in docs will be perfect.

@aclements

This comment has been minimized.

Copy link
Member

commented Oct 3, 2016

There is a slight bug in the documentation. As written, it technically guarantees that SetFinalizer will panic if passed a pointer to a package-level variable. That clearly isn't the case. In fact, this claim is nonsense even if you ignore package-level variables, since we can't tell the difference between passing, say, a pointer to a composite literal and a pointer to its first element. So we should change this to say that it "may abort the program."

However, I'm inclined to change the guarantee here as well (and also the docs) to accept pointers to package-level variables just to simplify the API and its specification. I can't see how this would unduly constrain future implementations. @RLH?

@aclements aclements reopened this Oct 3, 2016

@aclements aclements added this to the Go1.8Maybe milestone Oct 3, 2016

@gopherbot

This comment has been minimized.

Copy link

commented Oct 3, 2016

CL https://golang.org/cl/30137 mentions this issue.

gopherbot pushed a commit that referenced this issue Oct 3, 2016

runtime: weaken claim about SetFinalizer panicking
Currently the SetFinalizer documentation makes a strong claim that
SetFinalizer will panic if the pointer is not to an object allocated
by calling new, to a composite literal, or to a local variable. This
is not true. For example, it doesn't panic when passed the address of
a package-level variable. Nor can we practically make it true. For
example, we can't distinguish between passing a pointer to a composite
literal and passing a pointer to its first field.

Hence, weaken the guarantee to say that it "may" panic.

Updates #17311. (Might fix it, depending on what we want to do with
package-level variables.)

Change-Id: I1c68ea9d0a5bbd3dd1b7ce329d92b0f05e2e0877
Reviewed-on: https://go-review.googlesource.com/30137
Reviewed-by: Brad Fitzpatrick <bradfitz@golang.org>

@quentinmit quentinmit added the NeedsFix label Oct 11, 2016

@quentinmit

This comment has been minimized.

Copy link
Contributor

commented Oct 11, 2016

@aclements @RLH Can we close this issue now?

@aclements

This comment has been minimized.

Copy link
Member

commented Oct 12, 2016

@RLH and I chatted about whether to guarantee that SetFinalizer won't panic if passed a package-level variable. Neither of us have strong feelings about this either way, but the existing non-guarantee is more permissive for the implementation of finalizers and lets us change our minds later if there's more evidence. Further, it's important in general that SetFinalizer only be called for the beginning of an object, but we can't check that for package-level data, so we're leaning towards not trying to make any guarantees about pointers to package-level variables.

Hence, we're going to leave the documentation alone unless there's more compelling evidence for specifying this than the documentation being more restrictive than the implementation.

@aclements aclements closed this Oct 12, 2016

@yaxinlx

This comment has been minimized.

Copy link
Author

commented Oct 13, 2016

@aclements, @RLH,

the original title of this issue is "Make the SetFinalizer docs perfect".
I just feel the docs is some verbose.

For example, in the tip version, the docs says:

The argument obj must be a pointer to an object allocated by calling
new, by taking the address of a composite literal, or by taking the
address of a local variable.

So I think the following SetFinalizer callings for a, b, c will be always safe in later go versions:

package main

import (
    "runtime"
)

func callback1(*[]int) {}
func callback2(*int) {}

var a = &[]int{}
var b = new(int)

func main() {
    v := 123
    c := &v

    runtime.SetFinalizer(a, callback1)
    runtime.SetFinalizer(b, callback2)
    runtime.SetFinalizer(c, callback2)

    // ...

    // ....
}

However, the docs then says this:

It is not guaranteed that a finalizer will run for objects allocated
in initializers for package-level variables. Such objects may be
linker-allocated, not heap-allocated.

Now, I can't confirm if the SetFinalizer callings for a, b will be always safe or not.

@aclements

This comment has been minimized.

Copy link
Member

commented Oct 13, 2016

@yaxinlx, maybe I don't understand what you mean by SetFinalizer being "safe". To me, SetFinalizer is "safe" in situations it guarantees won't panic. But your second quote from the documentation doesn't say anything about SetFinalizer panicking. It just says the finalizer may not run for a or b (which is true in general, since finalizers are never guaranteed to be run).

@yaxinlx

This comment has been minimized.

Copy link
Author

commented Oct 14, 2016

The complaint for the docs is I don't understand what the "must" mean in the first quote.
"must be xxx to possibly trigger a finalizer calling", "must be xxx to avoid paniking in later go versions"?

Thanks for your explanation. By your explanation, I think "must" means "must be xxx to avoid paniking in later go versions".

@aclements

This comment has been minimized.

Copy link
Member

commented Oct 14, 2016

The complaint for the docs is I don't understand what the "must" mean in the first quote.
"must be xxx to possibly trigger a finalizer calling", "must be xxx to avoid paniking in later go versions"?

Either may be true. The point is that this is an intentionally underspecified contract between SetFinalizer and the caller of SetFinalizer. "The argument obj must be a pointer to an object allocated by calling new, by taking the address of a composite literal, or by taking the address of a local variable." means it's the caller's responsibility to pass an obj that satisfies these requirements. If the caller does something else, the behavior of SetFinalizer is unspecified. It may panic (a possibility suggested by the last sentence in that paragraph). It may do nothing (in which case the finalizer will never run). It may reboot your computer (though that would be rude). Who knows? And, as you point out, it may behave differently in other versions of Go or other implementations of Go (e.g., gccgo, GopherJS).

@golang golang locked and limited conversation to collaborators Oct 14, 2017

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.