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

proposal: runtime: provide centralized facility for managing (c)go pointer handles #37033

Open
oakad opened this issue Feb 5, 2020 · 8 comments
Open
Labels
Projects
Milestone

Comments

@oakad
Copy link

@oakad oakad commented Feb 5, 2020

Any non-trivial program relying on cgo eventually will find itself in need to use callbacks and interact with go objects. The current "customary" way to deal with this situation is for package implementer to come with some sort of global map to establish correspondence between go pointers and some sort of custom cgo handle.

(I'm talking about pointers to complex go objects, which are normally disallowed by cgo call checker).

This is unfortunate, as many cgo enabled packages are required to carry boilerplate code with a lot of potential for hard to find bugs, such as:

  1. Handle assignment collisions
  2. Pointer "leaks"
  3. Concurrency issues, accessing those "pointer to handle" maps

Therefore, it will be nice if go runtime provided a small API to handle the above case cleanly, to the tune of:

// Make i non-collectable and non-movable and return a cgo handle h for it
func cgoMakeHandle(i interface{}) (h uintptr)

// Given a cgo handle, return a Go reference for the object, if valid
func cgoGetHandle(h uintptr) (i interface{}, ok bool)

// Same as "get" above, but the handle h becomes invalid, and reference i becomes collectable
// and movable again
func cgoReleaseHandle(h uintptr) (i interface{}, ok bool)

The h handle, while valid, is then supposed to be safely passable to native code and back.

@gopherbot gopherbot added this to the Proposal milestone Feb 5, 2020
@gopherbot gopherbot added the Proposal label Feb 5, 2020
@ianlancetaylor

This comment has been minimized.

Copy link
Contributor

@ianlancetaylor ianlancetaylor commented Feb 5, 2020

We aren't going to add a general purpose facility to make an arbitrary number of pointers non-collectable and non-movable. That goes against one of the main goals of https://go.googlesource.com/proposal/+/refs/heads/master/design/12416-cgo-pointers.md, which is to have a fixed known number of such pointers at any moment in time.

I don't see anything wrong with the handle part of this proposal, but I don't see any reason it has to be in the runtime package. It can be in any package, using a map.

@oakad

This comment has been minimized.

Copy link
Author

@oakad oakad commented Feb 5, 2020

Even if it stays a map, it's much better to have a truly global, pointer keyed, concurrency-safe map.

Because currently it's just one big mess. Every complex cgo package comes with some creative ways to store pointers and allocate handles. This causes proliferation of global variables with unclear semantics, contributing to non-obvious side effects.

There are a great deal of "application domain specific" libraries which will be infeasible to port to Go anytime soon. On the other hand, Go had already reached a sufficient level of popularity to proliferate into places, not conceivable earlier, and link with those libraries.

Therefore, it makes all sense to standardize the pointer passing issue a bit further. For comparison, Java had a reasonably full featured JNI spec covering all aspects of native code interactions from day one, and it helped adoption a lot in the early 00' when pure Java code was not abundant yet.

@ianlancetaylor

This comment has been minimized.

Copy link
Contributor

@ianlancetaylor ianlancetaylor commented Feb 5, 2020

I'm not objecting to the basic idea, I'm just saying that it doesn't have to be in the runtime package.

For reference, the version of this idea that I wrote for use inside Google has this API:

// H is the type of a handle to a Go value.  The underlying type may change, but
// values are guaranteed to always fit in a C.uintptr_t.
//
// The zero H is never a valid handle, and is thus safe to use as a sentinel in
// C APIs.
type H cUintptr

// New returns a handle for a Go value.  The handle is valid until the
// program calls Delete on it.  The handle uses resources, and this
// package assumes that C++ code may hold on to the handle, so the
// program must explicitly call Delete when the handle is no longer
// needed.  The intended use is to pass the returned handle to C
// code, which passes it back to Go, which calls Value.
func New(v interface{}) H

// Delete removes a handle.  This should be called when C code no
// longer has a copy of the handle, and the program no longer needs
// it.  This returns an error if the handle is not valid.
func Delete(h H) error

// MustDelete removes a handle, and calls log.Fatal if the handle is
// not valid.
func MustDelete(h H)

// Value returns the Go value for a handle.  If the handle is not
// valid, this returns an error.
func Value(h H) (interface{}, error)

// MustValue looks up a handle.  If the handle is not valid, it calls
// log.Fatal.
func MustValue(h H) interface{}
@oakad

This comment has been minimized.

Copy link
Author

@oakad oakad commented Feb 6, 2020

We've got a package like that as well. Just like many other people/companies. This was exactly my point - something like that should be standardized.

@ianlancetaylor

This comment has been minimized.

Copy link
Contributor

@ianlancetaylor ianlancetaylor commented Feb 6, 2020

Understood. Again, I'm not objecting to the basic idea, I'm just saying that it doesn't have to be in the runtime package.

The next step is to pick a package and define an API.

@rsc

This comment has been minimized.

Copy link
Contributor

@rsc rsc commented Feb 12, 2020

And you could easily publish an package outside the standard library and see how much use it gets. It's unclear that this must be in the standard library.

@bcmills

This comment has been minimized.

Copy link
Member

@bcmills bcmills commented Feb 12, 2020

it's much better to have a truly global, pointer keyed, concurrency-safe map.

Note that a single global map would likely encounter a lot of cache contention if implemented using the primitives we have in the standard library today.

(#21035 would need to be addressed in order to implement this library using sync.Map in a way that scales appropriately with CPU core count.)

@rsc

This comment has been minimized.

Copy link
Contributor

@rsc rsc commented Feb 26, 2020

If we were going to do this in the standard library, there is already a runtime/cgo package with no exported API, but we could add

type Handle uintptr

func NewHandle(v interface{}) Handle
func (h Handle) Delete() // panics if h already deleted or invalid
func (h Handle) Value() interface{} // panics if h already deleted or invalid

It does seem to come up often, and we could do a good implementation. Thoughts?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
Linked pull requests

Successfully merging a pull request may close this issue.

None yet
5 participants
You can’t perform that action at this time.