Skip to content

Commit

Permalink
fix(go): unable to reuse instances between child/parent interfaces (#…
Browse files Browse the repository at this point in the history
…3321)

The go runtime was not tracking aliases in a way that enabled it to
return an alternate proxy to an object instance when that had previously
been returned as a different, singular interface.

This change keeps tack of the "first ever" registered instance against a
given InstanceID (as this is likely the one that was natively
constructed), but also to all proxy aliases to the same InstanceID, so
that it can later return an existing proxy that can be converted to a
desired type.

Fixes #2688

---

By submitting this pull request, I confirm that my contribution is made under the terms of the [Apache 2.0 license].

[Apache 2.0 license]: https://www.apache.org/licenses/LICENSE-2.0
  • Loading branch information
Romain Marcadier authored Jan 26, 2022
1 parent 32bf95c commit 70be636
Show file tree
Hide file tree
Showing 4 changed files with 47 additions and 12 deletions.
4 changes: 2 additions & 2 deletions gh-pages/content/specification/6-compliance-report.md
Original file line number Diff line number Diff line change
Expand Up @@ -5,7 +5,7 @@
This section details the current state of each language binding with respect to our standard compliance suite.


| number | test | java (98.33%) | golang (78.33%) | Dotnet | Python |
| number | test | java (98.33%) | golang (79.17%) | Dotnet | Python |
| ------ | ---------------------------------------------------------------------------------------------------------------------------------------------------------------- | ------------- | -------------------------------------------- | ------ | ------ |
| 1 | asyncOverrides_overrideCallsSuper | 🟢 | [🔴](https://github.com/aws/jsii/issues/2670) |||
| 2 | [arrayReturnedByMethodCanBeRead]("Array created in the kernel can be queried for its elements") | 🟢 | 🟢 |||
Expand Down Expand Up @@ -42,7 +42,7 @@ This section details the current state of each language binding with respect to
| 33 | testLiteralInterface | 🟢 | 🟢 |||
| 34 | structs_nonOptionalhashCode | 🟢 ||||
| 35 | propertyOverrides_set_throws | 🟢 | 🟢 |||
| 36 | canLeverageIndirectInterfacePolymorphism | 🟢 | [🔴](https://github.com/aws/jsii/issues/2688) |||
| 36 | canLeverageIndirectInterfacePolymorphism | 🟢 | 🟢 |||
| 37 | fluentApi | 🟢 ||||
| 38 | staticListInClassCanBeReadCorrectly | 🟢 | 🟢 |||
| 39 | mapReturnedByMethodCannotBeModified | 🟢 ||||
Expand Down
1 change: 0 additions & 1 deletion packages/@jsii/go-runtime-test/project/compliance_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -1462,7 +1462,6 @@ func (suite *ComplianceSuite) TestCanLeverageIndirectInterfacePolymorphism() {
require := suite.Require()
require.Equal(float64(1337), *provider.ProvideAsClass().Value())

suite.FailTest("Unable to reuse instances between parent/child interfaces", "https://github.com/aws/jsii/issues/2688")
require.Equal(float64(1337), *provider.ProvideAsInterface().Value())
require.Equal("to implement", *provider.ProvideAsInterface().Verb())
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -64,7 +64,7 @@ func (c *Client) castAndSetToPtr(ptr reflect.Value, data reflect.Value) {
}

// If it's currently tracked, return the current instance
if object, ok := c.objects.GetObject(ref.InstanceID); ok {
if object, ok := c.objects.GetObjectAs(ref.InstanceID, ptr.Type()); ok {
ptr.Set(object)
return
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -22,11 +22,16 @@ type ObjectStore struct {
// passed to the Register method.
objectToID map[uintptr]string

// idToObject associates an instanceID with the reflect.Value that
// represents the top-level object that was registered with the instanceID
// via the Register method.
// idToObject associates an instanceID with the first reflect.Value instance
// that represents the top-level object that was registered with the
// instanceID first via the Register method.
idToObject map[string]reflect.Value

// idToObjects associates an instanceID with the reflect.Value instances that
// represent the top-level objects that were registered with the instanceID
// via the Register method.
idToObjects map[string]map[reflect.Value]struct{}

// idToInterfaces associates an instanceID with the set of interfaces that it
// is known to implement.
//
Expand All @@ -40,6 +45,7 @@ func New() *ObjectStore {
return &ObjectStore{
objectToID: make(map[uintptr]string),
idToObject: make(map[string]reflect.Value),
idToObjects: make(map[string]map[reflect.Value]struct{}),
idToInterfaces: make(map[string]stringSet),
}
}
Expand Down Expand Up @@ -73,15 +79,17 @@ func (o *ObjectStore) Register(value reflect.Value, objectRef api.ObjectRef) err

aliases := findAliases(value)

if existing, found := o.idToObject[objectRef.InstanceID]; found {
if existing == value {
if existing, found := o.idToObjects[objectRef.InstanceID]; found {
if _, found := existing[value]; found {
o.mergeInterfaces(objectRef)
return nil
}
// Value already exists (e.g: a constructor made a callback with "this"
// passed as an argument). We make the current value an alias of the new
// passed as an argument). We make the current value(s) an alias of the new
// one.
aliases = append(aliases, existing)
for existing := range existing {
aliases = append(aliases, existing)
}
}

for _, alias := range aliases {
Expand All @@ -92,7 +100,14 @@ func (o *ObjectStore) Register(value reflect.Value, objectRef api.ObjectRef) err
}

o.objectToID[ptr] = objectRef.InstanceID
o.idToObject[objectRef.InstanceID] = value
// Only add to idToObject if this is the first time this InstanceID is registered
if _, found := o.idToObject[objectRef.InstanceID]; !found {
o.idToObject[objectRef.InstanceID] = value
}
if _, found := o.idToObjects[objectRef.InstanceID]; !found {
o.idToObjects[objectRef.InstanceID] = make(map[reflect.Value]struct{})
}
o.idToObjects[objectRef.InstanceID][value] = struct{}{}
for _, alias := range aliases {
o.objectToID[alias.Pointer()] = objectRef.InstanceID
}
Expand Down Expand Up @@ -169,6 +184,27 @@ func (o *ObjectStore) GetObject(instanceID string) (value reflect.Value, found b
return
}

// GetObjectAs attempts to retrieve the object value associated with the given
// instanceID, compatible with the given type. Returns the existing value and a
// boolean informing whether a value was associated with this instanceID and
// compatible with this type or not.
//
// The GetObjectAs method is safe to call with an instanceID that was never
// registered with the ObjectStore.
func (o *ObjectStore) GetObjectAs(instanceID string, typ reflect.Type) (value reflect.Value, found bool) {
found = false
if values, exists := o.idToObjects[instanceID]; exists {
for value = range values {
if value.CanConvert(typ) {
value = value.Convert(typ)
found = true
return
}
}
}
return
}

// canonicalValue ensures the same reference is always considered for object
// identity (especially in maps), so that we don't get surprised by pointer to
// struct versus struct value versus opaque interface value, etc...
Expand Down

0 comments on commit 70be636

Please sign in to comment.