feat(InstanceContext): Allow any type of user data #85
Conversation
Previously the given unsafe.Pointer to user data was passed directly across the CGo FFI resulting in a panic for any reference types or types that included any reference types or other Go pointers. This commit avoids a panic by casting the unsafe.Pointer to a uintptr, which is not analyzed by the runtime. In order for compatability with the existing CGo API and bridge code, which expects (void*), a *uintptr is passed. The memory for the user provided unsafe.Pointer and the *uintptr is cached in the instance to protect it from garbage collection. The tests are expanded to ensure that reference types do not cause panics. re wasmerio#44 wasmerio#83
|
I have added a test for using reference types as well that fails on the master branch but succeeds after this patch. @losfair Thanks for the suggestion. This is much simpler than the previous approach, and has the advantage of not at all changing the API. However the downside of not changing the API is that when users who implemented some other work around update their dependencies they will not have anything in particular that would alert them to the new functionality, especially given that the documentation for Instance.SetContextData was already misleading. Perhaps the documentation should be updated to be more explicit that any type can be used. This would help signal to users that the package has been improved. Changing the functions to use an interface{} instead of an unsafe.Pointer might be a friendlier API anyway so that users don't have to import unsafe. Let me know if you would like to see the documentation for SetContextData to be updated or this API change and I will push those commits on this PR. |
Hywan
added
🎉 enhancement
|
I will double-check with @losfair, but it looks good to me, thanks! I agree the documentation must be improved. Do you feel brave enough to update it in this PR, or do you prefer to postpone (by opening a new issue for instance)? |
|
Happy to update the documentation. I'll get that pushed today in an additional commit to this PR. Any thoughts about changing the API to use an |
…for Data
Instead of an unsafe.Pointer, use an interface{} to allow stronger type
assertions and safer code. Additionally update the documentation for
Instance.SetContextData to explicitly allow data types that include Go
references.
BREAKING CHANGE: Changes the API of Instance.SetContextData and
InstanceContext.Data to use interface{}.
|
I added a commit that changes to use an |
|
I'm fine with breaking the compatibility for the moment. We are still on the 0.x versions. So changing for |
|
Thank you very much! |
Previously an uintptr to user data was passed across the CGo FFI which would become invalid if any stack frame occurred. This commit avoids this by never passing any pointers across the CGo FFI. A registry map is used instead to keep Go data on the Go side, and only an int index into the map is passed across to the C side. fix wasmerio#93 re wasmerio#44 wasmerio#85 wasmerio#83
Previously the given unsafe.Pointer to user data was passed directly
across the CGo FFI resulting in a panic for any reference types or types
that included any reference types or other Go pointers.
This commit avoids a panic by casting the unsafe.Pointer to a uintptr,
which is not analyzed by the runtime. In order for compatability with
the existing CGo API and bridge code, which expects (void*), a *uintptr
is passed. The memory for the user provided unsafe.Pointer and the
*uintptr is cached in the instance to protect it from garbage
collection.
The tests are expanded to ensure that reference types do not cause
panics.
re #44 #83