chinedufn / swift-bridge

swift-bridge facilitates Rust and Swift interop.
https://chinedufn.github.io/swift-bridge
Apache License 2.0
838 stars 62 forks source link

already borrowed: BorrowMutError #292

Closed annethereshewent closed 1 month ago

annethereshewent commented 1 month ago

Hello,

So I have in my rust code the following struct:

pub struct Nds {
  pub bus: Rc<RefCell<Bus>>
}

In lib.rs, I have the following code:

#[swift_bridge::bridge]
mod ffi {
  extern "Rust" {
    type MobileEmulator;

    #[swift_bridge(init)]
    fn new(...) -> MobileEmulator;

    fn touch_screen(&mut self, x: u16, y: u16);
  }
}

and the relevant method:

 pub fn touch_screen(&mut self, x: u16, y: u16) {
    let ref mut bus = *self.nds.bus.borrow_mut();

    bus.touchscreen.touch_screen(x, y);
    bus.arm7.extkeyin.remove(ExternalKeyInputRegister::PEN_DOWN);
  }

finally in Swift I have this code that calls the touch_screen method:

@State private var emulator: MobileEmulator? = nil

      Image(uiImage: bottomImage)
                .frame(width: 256, height: 192)
                .onTapGesture() { location in
                    print("You touched me at \(location)")
                    emulator?.touch_screen(UInt16(location.x), UInt16(location.y))
                }

Everything works fine the first time when you tap the screen. However, if you tap the screen a second time Swift panics with the error already borrowed: BorrowMutError

From what I can tell, it seems that the bus reference that I call borrow_mut() on isn't being dropped at the end of the method call, which seems like a bug. Is there a workaround for this? Thank you!

chinedufn commented 1 month ago

Can you comment with a minimal reproduction of the error so that we can be confident about whether this issue is related to swift-bridge?

Your comment can include:

A minimal bridge module and Rust struct(s) that fail.

A minimal Swift function that calls the Rust struct's methods.


Also, what happens if you add an explicit drop(bus) to your code snippet above?

annethereshewent commented 1 month ago

Sure, no problem:

So if you do this it should throw the error:

lib.rs

use std::{rc::Rc, cell::RefCell}

#[swift_bridge::bridge]
mod ffi {
  extern "Rust" {
    type SomeStruct;

    #[swift_bridge(init)]
    fn new() -> SomeStruct;

    fn some_method(&mut self);
  }
}

pub struct SomeStruct {
  other_struct: Rc<RefCell<SomeOtherStruct>>
}

struct SomeOtherStruct {}

impl SomeStruct {
  pub fn new() -> Self {
    Self {
       other_struct: Rc::new(RefCell::new(SomeOtherStruct {}))
    }
  }
  pub fn some_method(&mut self) {
    let ref mut other_struct = *self.other_struct.borrow_mut();
  }
}

then in swift if you call some_method twice in a row it should throw the error:

class SomeClass {
  let someStruct = SomeStruct()

  func some_method() {
    someStruct.some_method()
    someStruct.some_method() // this will throw the error
  }
}

as for calling drop on bus, rust throws an error saying cannot move out *bus which is behind a mutable reference

Let me know if you have any issues reproducing.

chinedufn commented 1 month ago

Have you confirmed that this example throws an error?

I ran let _ = SomeClass().some_method() it and didn't get an error.

$ rustc -V
rustc 1.79.0-nightly (aed2187d5 2024-04-27)
annethereshewent commented 1 month ago

I forgot to mention that you will need to make SomeClass a property of a SwiftUI view and then call it twice.

chinedufn commented 1 month ago

Got it. Please provide a minimal GitHub repository with a README containing instructions on:

Then I can confirm the issue and work on a fix.

annethereshewent commented 1 month ago

Ok so I figured out how to reproduce it, the problem occurs when using multiple threads. here is the full code. I'll also create a repo soon

Just press the "touch me" button twice and you will see the error.

Rust:

use std::{cell::RefCell, rc::Rc};

#[swift_bridge::bridge]
mod ffi {
  extern "Rust" {
    type SomeStruct;

    #[swift_bridge(init)]
    fn new() -> SomeStruct;
    fn some_method(&mut self);
  }
}

struct OtherStruct {
    pub prop: bool,
    pub prop2: bool
}

pub struct SomeStruct {
    other: Rc<RefCell<OtherStruct>>
}

impl SomeStruct {
    pub fn new() -> Self {
        Self {
            other: Rc::new(RefCell::new(OtherStruct { prop: false, prop2: false }))
        }
    }
    pub fn some_method(&mut self) {
        let ref mut other = *self.other.borrow_mut();

        other.prop = !other.prop
    }
}

Swift:

//
//  ContentView.swift
//  TestApp
//

import SwiftUI
import SomeStruct

struct ContentView: View {
    private let someStruct = SomeStruct()
    var body: some View {
        VStack {
            Image(systemName: "globe")
                .imageScale(.large)
                .foregroundStyle(.tint)
            Text("Hello, world!")
        }
        Button("Touch me") {
            let workItem = DispatchWorkItem {
                while true {
                    someStruct.some_method()
                }
            }
            someStruct.some_method()

            DispatchQueue.global().async(execute: workItem)
        }
        .padding()
    }
}

#Preview {
    ContentView()
}
annethereshewent commented 1 month ago

So I figured out a way around it and the cause. If you have someStruct running some_methodin another thread and then use some_method in the main thread, it panics. However if you keep everything in the main thread it works just fine, so what I ended up doing is this:

//
//  ContentView.swift
//  TestApp
//
//  Created by Anne Castrillon on 9/17/24.
//

import SwiftUI
import SomeStruct

struct ContentView: View {
    private let someStruct = SomeStruct()

    var body: some View {

        VStack {
            Image(systemName: "globe")
                .imageScale(.large)
                .foregroundStyle(.tint)
            Text("Hello, world!")
        }
        Button("initialize") {
            let workItem = DispatchWorkItem {
                while true {
                    DispatchQueue.main.sync() {
                        someStruct.some_method()
                    }
                }
            }

            DispatchQueue.global().async(execute: workItem)
        }
        Button("Touch me") {
            someStruct.some_method()
        }
        .padding()
    }
}

#Preview {
    ContentView()
}

Not sure if it matters, but the reason for the infinite loop in a separate thread is because I need to continuously run an action on someStruct (in my real app) to update the view. however, the while loop blocks the main thread so the view never updates unless I put the code in another thread.

Hope that makes sense, feel free to close this.

chinedufn commented 1 month ago

I'm surprised that SomeStruct can be passed between threads without implementing Swift's Sendable protocol, but I'm not familiar enough with Swift concurrency to know why that's possible.


If you are indeed accessing the RefCell from multiple threads, then the problem would be stemming from the fact that RefCell is not thread safe.

You may consider replacing Rc<RefCell<Bus>> with Arc<Mutex<Bus>>.

I need to continuously run an action on someStruct (in my real app) to update the view

Without more information I would imagine that there's a better design than this.

At the very least if you're running a function to update a view (not saying that I condone this design since I don't have any information) you wouldn't need to run it more frequently than the device's refresh rate.


Closing since your issue is addressed.

I'm curious about whether DispatchQueue allows non Sendable types to be passed between threads, and if there's anything that swift-bridge can do to make this a compile time error, but we can explore that separately.

NiwakaDev commented 3 weeks ago

@chinedufn

I'm surprised that SomeStruct can be passed between threads without implementing Swift's Sendable protocol, but I'm not familiar enough with Swift concurrency to know why that's possible.

Maybe related to https://github.com/swiftlang/swift-evolution/blob/main/proposals/0414-region-based-isolation.md#motivation