Brendonovich / swift-rs

Call Swift functions from Rust with ease!
Apache License 2.0
246 stars 28 forks source link

Fix memory handling, and related stuff #17

Closed amodm closed 1 year ago

amodm commented 1 year ago

Problem

The current model of managing memory is broken. As an example, replace the following snippet of example/src/main.rs:

fn main() {
    let path = "/Users";
    let thumbnail = unsafe { get_file_thumbnail_base64(path.into()) };
    println!("length of base64 encoded thumbnail: {}", thumbnail.as_str().len());

    let mounts = unsafe { get_mounts() };
    println!("First Volume Name: {}", mounts[0].name);

    let opt = unsafe { return_nullable(true) };
    println!("function returned nil: {}", opt.is_none());

    let opt = unsafe { return_nullable(false) };
    println!("function returned data: {}", opt.is_some());
}

with the following (essentially just running the exact same code on a non-main thread):

fn main() {
    let handle = std::thread::spawn(|| {
        let path = "/Users";
        let thumbnail = unsafe { get_file_thumbnail_base64(path.into()) };
        println!("length of base64 encoded thumbnail: {}", thumbnail.as_str().len());

        let mounts = unsafe { get_mounts() };
        println!("First Volume Name: {}", mounts[0].name);

        let opt = unsafe { return_nullable(true) };
        println!("function returned nil: {}", opt.is_none());

        let opt = unsafe { return_nullable(false) };
        println!("function returned data: {}", opt.is_some());
    };
    handle.join.unwrap();
}

This will break. I realised this when writing tests, and the exact code of the example wouldn't work (tests get executed on a non-main thread).

Reason

The reason is that in the current model, when we create an object on the Swift side and pass it on to Rust, we do not disconnect it from Swift's ARC. So when the Rust side drops it (thus calling a .release() on it via SRObject's drop implementation) , Swift's ARC still assumes this object needs to be dropped, eventually dropping it in autorelease. This happens only when a separate thread is used.

Fix

The recommended way to handle Swift & C-like languages interfacing, is sticking to the following rules (I've also mentioned corresponding changes done in this PR):

  1. When an object is created on Swift side, and is being sent to Rust side, assuming that Rust will now be owning that object (in the sense of managing its lifecycle), we need to disconnect the Swift object from ARC before passing it along. To do this, I've created a toRust() function which does exactly that. Every T: SRObject return from Swift needs to be wrapped in toRust().
  2. Given that Rust now owns the object, once it's done with it, it needs to invoke Unmanaged..release() to tell Swift to free it up. The current implementation of SRObject::drop() does that already, but had a bug, which has been fixed in this PR.
  3. When Rust sends a SRString object to Swift side, the only memory-safe way it can be sent, is as a reference (i.e. &value, not value). Let's take the get_file_thumbnail_base64(path: SRString) of example. If path is an SRString instead of &SRString, Rust treats it as consumed in the function call. But the function here is an extern function, so the drop on path never gets called - a memory leak!
  4. The above also means that any Swift side function which receives an SRString parameter, now needs to take in a ptr: UnsafePointer<SRString> instead, and use ptr.pointee.to_string() to get a Swift string.

Cascading effects

Along with the above, this PR includes the following:

  1. Tests for the SRString bindings - both from Rust to Swift & vice-versa. This is in test_string() of tests/test_bindings.rs. Similar tests can/should be created for other types, if this PR gets merged.
  2. Tests for memory leak detection via leaks command. See test_memory_leaks() under tests/test_binding.rs
  3. For the tests to be viable, they needed to be linked to a test swift package, which required a build script (src-rs/test-build.rs) and corresponding changes to Cargo.toml. This build script is a no-op unless specific test conditions are met, so does not create any overhead for users of this library.
  4. Changes in example to use toRust() and accept SRString as a reference on Swift side (i.e. UnsafePointer<SRString>)
  5. Documentation changes to reflect the API changes

I wish there was a non-API breaking change to do all of the above, but to the best of my knowledge, there isn't.

Brendonovich commented 1 year ago

Thanks for doing all of this, but I want to hold off merging just yet. Detailing all of this in an issue before actually implementing it would have been appreciated, since we already have a solution to the memory management problem implemented on the autorelease branch that uses a different method than what you've done here. It's not part of main because I'm not sure if it's actually sound, but I'd like to compare these two approaches in terms of performance, DX, and which actually works better. I'll detail my solution in #8 and I'd love for you to compare it, it's very possible that my solution isn't sound considering that you seem to have greater knowledge of how Swift's memory management actually works.

amodm commented 1 year ago

Holding off is perfectly alright - merging the PR is your discretion, not my right. We can conclude about it in the discussion in #8.

Brendonovich commented 1 year ago

After much discussion, I'm going to bite the bullet and go ahead with this implementation. While I don't like the API changes, the memory safety is worth the change. Thanks so much for doing all this, I definitely wouldn't have figured it out on my own!

amodm commented 1 year ago

Thanks @Brendonovich for prioritising safety. I agree that this API is uglier than the alternative we were discussing. I'm hoping this is something that we're able to macro on top off, to have a better DX.