Skip to content

Commit

Permalink
Cleanup callback code and test callback lifetimes
Browse files Browse the repository at this point in the history
1) Test callback lifetime. Equivalent of
mozilla/uniffi-rs#2230

The code looks like it should have the equivalent issue as
NordSecurity/uniffi-bindgen-cs#79, but because
pointers were taken to local variables, each pointer was unique, and
received a unique handle.

2) Store generic type directly in concurrentHandleMap, not via pointer.
Remove `rightMap`, as it actually doesn't do anything, and only creates
issues. (NordSecurity/uniffi-bindgen-cs#79)
  • Loading branch information
arg0d committed Sep 13, 2024
1 parent 034f4ff commit fb12940
Show file tree
Hide file tree
Showing 4 changed files with 62 additions and 21 deletions.
11 changes: 11 additions & 0 deletions Cargo.lock

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

30 changes: 10 additions & 20 deletions bindgen/templates/CallbackInterfaceRuntime.go
Original file line number Diff line number Diff line change
@@ -1,48 +1,38 @@
{% if self.include_once_check("CallbackHelpers.go") %}{% include "CallbackHelpers.go" %}{% endif %}

type concurrentHandleMap[T any] struct {
leftMap map[uint64]*T
rightMap map[*T]uint64
handles map[uint64]T
currentHandle uint64
lock sync.RWMutex
}

func newConcurrentHandleMap[T any]() *concurrentHandleMap[T] {
return &concurrentHandleMap[T]{
leftMap: map[uint64]*T{},
rightMap: map[*T]uint64{},
handles: map[uint64]T{},
}
}

func (cm *concurrentHandleMap[T]) insert(obj *T) uint64 {
func (cm *concurrentHandleMap[T]) insert(obj T) uint64 {
cm.lock.Lock()
defer cm.lock.Unlock()

if existingHandle, ok := cm.rightMap[obj]; ok {
return existingHandle
}
cm.currentHandle = cm.currentHandle + 1
cm.leftMap[cm.currentHandle] = obj
cm.rightMap[obj] = cm.currentHandle
cm.handles[cm.currentHandle] = obj
return cm.currentHandle
}

func (cm *concurrentHandleMap[T]) remove(handle uint64) bool {
func (cm *concurrentHandleMap[T]) remove(handle uint64) {
cm.lock.Lock()
defer cm.lock.Unlock()

if val, ok := cm.leftMap[handle]; ok {
delete(cm.leftMap, handle)
delete(cm.rightMap, val)
}
return false
delete(cm.handles, handle)
}

func (cm *concurrentHandleMap[T]) tryGet(handle uint64) (*T, bool) {
func (cm *concurrentHandleMap[T]) tryGet(handle uint64) (T, bool) {
cm.lock.RLock()
defer cm.lock.RUnlock()

val, ok := cm.leftMap[handle]
val, ok := cm.handles[handle]
return val, ok
}

Expand All @@ -60,15 +50,15 @@ func (c *FfiConverterCallbackInterface[CallbackInterface]) Lift(handle uint64) C
if !ok {
panic(fmt.Errorf("no callback in handle map: %d", handle))
}
return *val
return val
}

func (c *FfiConverterCallbackInterface[CallbackInterface]) Read(reader io.Reader) CallbackInterface {
return c.Lift(readUint64(reader))
}

func (c *FfiConverterCallbackInterface[CallbackInterface]) Lower(value CallbackInterface) C.uint64_t {
return C.uint64_t(c.handleMap.insert(&value))
return C.uint64_t(c.handleMap.insert(value))
}

func (c *FfiConverterCallbackInterface[CallbackInterface]) Write(writer io.Writer, value CallbackInterface) {
Expand Down
40 changes: 40 additions & 0 deletions binding_tests/fixture_callbacks_test.go
Original file line number Diff line number Diff line change
@@ -1,6 +1,8 @@
package binding_tests

import (
"fmt"
"strings"
"testing"

"github.com/NordSecurity/uniffi-bindgen-go/binding_tests/generated/fixture_callbacks"
Expand Down Expand Up @@ -73,6 +75,29 @@ func (invalidGetters) GetNothing(v string) *fixture_callbacks.SimpleError {
return fixture_callbacks.NewSimpleErrorBadArgument()
}

type goStringifier struct{}

func (goStringifier) FromSimpleType(value int32) string {
return fmt.Sprintf("Go: %d", value)
}

func (goStringifier) FromComplexType(values *[]*float64) string {
if values == nil {
return "Go: nil"
}

var strNumbers []string
for _, num := range *values {
if num != nil {
strNumbers = append(strNumbers, fmt.Sprintf("%f", *num))
} else {
strNumbers = append(strNumbers, "nil")
}
}

return fmt.Sprintf("Go: %s", strings.Join(strNumbers, " "))
}

type testGetterInput[T any] struct {
name string
value T
Expand Down Expand Up @@ -312,3 +337,18 @@ func TestRustGetters_GetStringOptionalCallback(t *testing.T) {
})
}
}

func TestRustGetters_CallbackReferenceDoesNotInvalidateOtherReferences(t *testing.T) {
stringifier := goStringifier{}
rustStringifier1 := fixture_callbacks.NewRustStringifier(stringifier)

{
rustStringifier2 := fixture_callbacks.NewRustStringifier(stringifier)
assert.Equal(t, "Go: 123", rustStringifier2.FromSimpleType(123))
rustStringifier2.Destroy()
// `stringifier` must remain valid after `rustStringifier2` drops the reference
}

assert.Equal(t, "Go: 321", rustStringifier1.FromSimpleType(321))
rustStringifier1.Destroy()
}
2 changes: 1 addition & 1 deletion binding_tests/issue45_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -6,8 +6,8 @@ package binding_tests

import (
"github.com/NordSecurity/uniffi-bindgen-go/binding_tests/generated/issue45"
"testing"
"github.com/stretchr/testify/assert"
"testing"
)

// Ensure multiple futures packages work fine together, the other one being
Expand Down

0 comments on commit fb12940

Please sign in to comment.