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

support native async calls introduced in uniffi 0.24 #5

Open
8 tasks
Tracked by #214
typfel opened this issue Jul 24, 2023 · 12 comments
Open
8 tasks
Tracked by #214

support native async calls introduced in uniffi 0.24 #5

typfel opened this issue Jul 24, 2023 · 12 comments

Comments

@fweissberg
Copy link

I created a branch of the kotlin multiplatform bindings that supports both the new callback interface and the ForeignExecutor. It was developed before unfffi 0.24.0 was released and arbitrarily targets mozilla/uniffi-rs@22b8c21

It should cover at least
mozilla/uniffi-rs@323a497 and
mozilla/uniffi-rs#1497

I was going to contribute it to https://gitlab.com/trixnity/uniffi-kotlin-multiplatform-bindings. However, the changes are quite massive, @benkuly doesn't have a whole lot of time to spare for the project and I'm not quite ready to merge this without a second pair of eyeballs going over it.

Maybe we could team up on this (after I clean up my branch a little bit)?

@typfel
Copy link
Member Author

typfel commented Jul 26, 2023

Great! I started to look into this so I'm happy to pair up with you.

@benkuly
Copy link
Contributor

benkuly commented Jul 26, 2023

Glad to hear that uniffi-kotlin-multiplatform-bindings is making progress! 🙂

I'm open to add code owners to the original project, if you like to.

@fweissberg
Copy link

Here is what I currently have: https://gitlab.com/fweissberg/uniffi-kotlin-multiplatform-bindings/-/tree/target_22b8c_w_async?ref_type=heads

I am going to port the futures test from upstream uniffi but it's not finished, yet.

Also, the callbacks tests are currently in disrepair because I originally just put some async tests there. Then, I discovered an issue with the JVM version of either the callbacks or async implementation when used together (see the comments in the test file).

@typfel
Copy link
Member Author

typfel commented Jul 27, 2023

Cool, I'll try to dedicate some time to review it tomorrow.

@fweissberg
Copy link

I finished porting the futures tests. Good thing I did, because it uncovered some bugs.

@fweissberg
Copy link

I fixed the bug preventing the callbacks test from passing on the JVM and pushed the commit to the branch mentioned above.

@typfel
Copy link
Member Author

typfel commented Jul 28, 2023

I fixed the bug preventing the callbacks test from passing on the JVM and pushed the commit to the branch mentioned above.

Nice!

Not only the test method you commented was failing for me though but also all the regular callback tests like callbackAsArgument() so maybe we don't need a dedicated regression test.

I see you added many async methods to the callback test. Was that for debugging purposes and do you intend to remove them again?

@fweissberg
Copy link

Yes, I think much of the code can be removed.

However, the bug itself was present before any of my patches, it was just not triggered.

I'll try building a minimal regression test maybe by manually triggering the GC.

@fweissberg
Copy link

I wasn't able to quickly come up with something that triggers the issue without using the async callbacks. At least, I reduced the async stuff in the callbacks test to a call to a single function and removed the rest of the clutter. It should all be covered by the futures test.

Feel free to give it a try, though.


I was going to give you permissions on my Gitlab fork of the project. But it seems that doesn't work because you logged into that service using your Github account? If it's more convenient, I could also move my fork to Github?

@typfel
Copy link
Member Author

typfel commented Jul 28, 2023

I wasn't able to quickly come up with something that triggers the issue without using the async callbacks. At least, I reduced the async stuff in the callbacks test to a call to a single function and removed the rest of the clutter. It should all be covered by the futures test.

Feel free to give it a try, though.

Hmm interesting, I ran the tests on a Mac.

I was going to give you permissions on my Gitlab fork of the project. But it seems that doesn't work because you logged into that service using your Github account? If it's more convenient, I could also move my fork to Github?

I'm @sunkig at GitLabs (https://gitlab.com/sunkig), was that the user you were trying to add?

It would helpful be able to comment on code in your changes, maybe open a (draft) PR?

@typfel
Copy link
Member Author

typfel commented Jul 28, 2023

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

No branches or pull requests

3 participants