fzyzcjy / flutter_rust_bridge

Flutter/Dart <-> Rust binding generator, feature-rich, but seamless and simple.
https://fzyzcjy.github.io/flutter_rust_bridge/
MIT License
3.61k stars 254 forks source link

Should OpaqueObject use read/write instead of try_read/try_write ? #1883

Closed alanlzhang closed 1 week ago

alanlzhang commented 1 month ago

Often rust code is called in a flutter event callback, it's common that user click another buttons while processing.

It is quite easy to call methods of the same opaque object a second time, and rust just panick.

Surely we can restrict calls at flutter side, but it's fragile.

Usually it's ok waiting previous call to complete, and read/write is just what users want.

So I think use read / write is better than currently try_read / try_write.

Of course it's even better if we can config this.

What do you think ? Thanks

fzyzcjy commented 1 month ago

Good question!

IIRC it is currently try_read/try_write partially because to avoid deadlock problems. As you know, suppose we have 2 opaque objects, it is really easy to create deadlock, especially under the scenarios you mentioned.

Brainstorms:

alanlzhang commented 1 month ago

I think maybe deadlock is ok because pure rust also cannot prevent it. (Best practices should be documented)

If there is some deadlock detection, then I think it's perfect Ok.

If we model application state as opaque object, the current multi-thread model locks everywhere to prevent data race and does not benefit much from it.

Maybe it's better to multi-threading stateless calls and event-loop stateful(opaque object) calls.

fzyzcjy commented 1 month ago

I will reconsider it a bit later and see what to implement!

aran commented 2 weeks ago

You can't get deadlocks in simple rust code that just uses ownership, &mut ref / &ref. You also can't get deadlocks in plain Flutter. Given the focus on safety, I would advocate that any changes here would aim to avoid introducing runtime errors like deadlocks or runtime borrow failures unless the developer explicitly opts in to using a less safe feature set.

$0.02, I think it's ok for a developer to risk a deadlock if they call lock.read() or lock.write() in their own code. I don't think it's ok for the framework to introduce a behind-the-scenes lock if its usage is not 100% safe.