[uniffi] Add more methods to CommitOutput

[mgeisler]

Issues:

Addresses #81

Description of changes:

This adds ratchet_tree and group_info methods to CommitOutput.

Testing:

None yet, I will add a test after merging #111 since that PR will make it trivial again to setup a new client.

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

[tomleavy]

@mgeisler just do you know it seems that uniffi does some not so nice things with memory management in Kotlin so unless you make things like CommitOutput a Record the users of this library will be required to "close" all handles. In my experiments I switched to just marshaling everything that doesn't have internal state.

[mgeisler]

@mgeisler just do you know it seems that uniffi does some not so nice things with memory management in Kotlin so unless you make things like CommitOutput a Record the users of this library will be required to "close" all handles. In my experiments I switched to just marshaling everything that doesn't have internal state.

Funny, we were just discussing this in a meeting here. I will be happy to follow this advice to make life easier for the developers who will end up using this.

[mgeisler]

unless you make things like CommitOutput a Record the users of this library will be required to "close" all handles.

To be more precise, I believe you're talking about the same thing as in this comment: mozilla/uniffi-rs#1869 (review).

There I tried figuring out why the Kotlin code has a big warning about memory management. My understanding is that apps can see their memory usage stay artificially high if the system is under load and the cleaner thread (and GC) doesn't run. This in turn can result in an untimely death in a system like Android.

It's not clear to me how much of a problem this is now — as far as I understand, Kotlin has a use pattern which people can use to auto-close the objects.

In this case, I think we have two choices for CommitOutput:

• CommitOutput could be a UniFFI record. This means that we eagerly marshal the commit message, the welcome messages, the ratchet tree, and the group info.
• CommitOutput could be a UniFFI object. This is the path I started down on. We could still marshal the types returned by the various methods.

Marshalling eagerly sounds more expensive, but since commits are done rarely(?), I guess paying this cost can be acceptable.

[tomleavy]

Quite honestly I think their concerns are misplaced and it's ok to deallocate native heap memory on the finalizer. Maybe good for you to look into with people who understand the Android JVM more, but SWIG for example does this for Java when wrapping C libraries and it is a non issue.

My understanding is that the finalizer is not guaranteed but typically would only be skipped if the app is shutting down or something where the deallocation doesn't matter anyway, but I could be wrong.

For now I chose record because yes it's expensive but actually the app will very likely call every getter anyway which might be more expensive overall. I'm basically using record whenever objects are returned from a function and not input to another function to start.

[codecov-commenter]

Codecov Report

Attention: Patch coverage is 0% with 31 lines in your changes are missing coverage. Please review.

Project coverage is 89.35%. Comparing base (4383d08) to head (34afdf0).

Files	Patch %	Lines
mls-rs-uniffi/src/lib.rs	0.00%	31 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main     #112      +/-   ##
==========================================
- Coverage   89.43%   89.35%   -0.08%     
==========================================
  Files         173      173              
  Lines       31334    31365      +31     
==========================================
+ Hits        28022    28027       +5     
- Misses       3312     3338      +26     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

[mgeisler]

Quite honestly I think their concerns are misplaced and it's ok to deallocate native heap memory on the finalizer.

Yeah, I have the same understanding. However, I learnt last week that the UniFFI code uses a new background thread for the deallocations. I'm not sure why this is needed, but I can see how using a thread can affect the deallocations in case the thread is starved for resources.

We'll basically keep an eye on this as we get into testing an actual Hello World program in Kotlin. Then we can adjust as needed.

For now I chose record because yes it's expensive but actually the app will very likely call every getter anyway which might be more expensive overall. I'm basically using record whenever objects are returned from a function and not input to another function to start.

Yes, I plan on following this approach as well going forward (and will convert the existing APIs into this form).