search

shekohex / allo-isolate

Run Multithreaded Rust along with Dart VM (in isolate) 🌀
Apache License 2.0
120 stars 18 forks source link

Fixed a bug with `zero-copy` cargo feature #41

Closed temeddix closed 1 year ago

temeddix commented 1 year ago

I am sorry to tell that I left out one of the most important changes when I was making the previous #39 PR.

self should be passed to ManuallyDrop instead of self.0, because this one doesn't have a ZeroCopyBuffer wrapper. I apologize for the mistake.

temeddix commented 1 year ago

I made a vec_to_dart_native_external_typed_data function to avoid code duplication. Also, I added CI tests with cargo features enabled, which uses cargo fmt and clippy, that also tells you about an error.

shekohex commented 1 year ago

Could we take a moment to think about how would be able to test something like that in our Mocked DartVM vm.rs?

@fzyzcjy did you figure out a way to do End to End testing with Memory checks using real Dart VM And Rust?

fzyzcjy commented 1 year ago

@fzyzcjy did you figure out a way to do End to End testing with Memory checks using real Dart VM And Rust?

Check the valgrind section in my ci.yaml. However, I just checked memory leaks, and do not check invalid memory accesses, because Dart seems to violate so many valgrind operations...

Related: https://github.com/dart-lang/sdk/issues/47346

shekohex commented 1 year ago

@fzyzcjy did you figure out a way to do End to End testing with Memory checks using real Dart VM And Rust?

Check the valgrind section in my ci.yaml. However, I just checked memory leaks, and do not check invalid memory accesses, because Dart seems to violate so many valgrind operations...

Related: dart-lang/sdk#47346

Fair enough. @temeddix are you open to make a PR that introduce these kind of checks into our code and then we merge it here into your PR? I want to make sure that we do not introduce any other memory bugs because of a new feature we added.

temeddix commented 1 year ago

I'm not familiar with valgrind tests, but I'll try to apply those in a few days :) I'll refer to flutter_rust_bridge repo.

temeddix commented 1 year ago

I dug into the valgrind test code of flutter_rust_bridge for some time, and unfortunately, it is too complicated for me to make that work here.

In flutter_rust_bridge, Github CI calls a justfile command, and that justfile command calls many other justfile commands, some of which call the .sh bash commands in the Dart folder, which calls a Python code, which executes valgrind. This logic is not simple enough for me to fully understand properly.

Apart from this 'deep call stack', there's no Dart example like flutter_rust_bridge in this repo. There are only Rust files here, which means many Dart codes for testing the library's features should be added. I'm not sure if I can understand all the features of this library and organize it into a testing Dart file. This complexity deserves a whole new PR :|

In my opinion, we could focus on fixing the bug and adding CI for Rust linting error in this PR.

shekohex commented 1 year ago

Fair enough. Okay I guess I will go an merge this PR.