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

feat: impl LongRefFromWasmAbi for T where T: Tsify + FromWasmAbi #35

Draft
wants to merge 2 commits into
base: main
Choose a base branch
from

Conversation

Pantamis
Copy link

@Pantamis Pantamis commented Apr 25, 2024

Implement #34

I tried to make a test of the behavior by building a test very similar to #33 using async function for validate and it fails 😧...

which is expected because Rust must be responsible for dropping the reference to the closure here so you cannot invoke the closure twice and we have no way to check it in Rust according to rustwasm/wasm-bindgen#3178 so this suggest my implementation is right actually 😃 ...

But it also fails if I use the same implementation as RefFromWasmAbi for the LongRefFromWasmAbi 😧, which normally does not drop the reference at the end of the first Rust invocation and so allow the second invocation to be processed.

In summary, I am not sure how to test that my implementation is the correct one 😕

I leave this PR here for people who are maybe more knowledgeable building a test for this. My main issue is that we cannot take an async closure as function argument to reproduce a test like in aaaa398 where validation is async

@siefkenj
Copy link
Owner

It looks like the expansion tests are the only ones that are failing? Or did you remove a different failing test?

@Pantamis
Copy link
Author

Yeah I forgot to change expansion results in test, I am on it !

@Pantamis
Copy link
Author

Pantamis commented Apr 25, 2024

All good for the CI now

I did not include the test using an async function for now because I am not really sure about what it is really testing 😅 (so tests did not change at all, I just completed expansion results)

If my implementation is bad, the worst case should be that we leak memory in the ABI heap because Rust may not clean the imported JsValue when dropping it (obviously, that's not what I have done and I want to test I don't do it), this may happen if I copy-paste the implementation of RefFromWasmAbi in the implementation of LongRefFromWasmAbi but I am not able to make a test that show this kind of mistake clearly.

Leaking memory is safe but ... yeah we should avoid it so it would be nice to test that a correct implementation avoid it 😒

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants